Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

allow clients to provide decision_id #1465

Closed
wants to merge 5 commits into from

Conversation

omerlh
Copy link
Contributor

@omerlh omerlh commented Jun 3, 2019

Signed-off-by: omerlh omerl@soluto.com

close #1451

omerlh added 2 commits June 3, 2019 17:11
Signed-off-by: omerlh <omerl@soluto.com>
Signed-off-by: omerlh <omerl@soluto.com>
Copy link
Contributor

@patrick-east patrick-east left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like it isn't passing the make check tests https://travis-ci.org/open-policy-agent/opa/builds/540773146?utm_source=github_status&utm_medium=notification

./build/check-fmt.sh
./build/check-vet.sh
./build/check-lint.sh
/home/travis/gopath/src/github.com/open-policy-agent/opa/server/server_test.go:2466:2: don't use underscores in Go names; var decision_id should be decisionID
/home/travis/gopath/src/github.com/open-policy-agent/opa/server/types/types.go:444:2: const ParamDecisionIdV1 should be ParamDecisionIDV1
make: *** [check-lint] Error 1
The command "make" exited with 2.

@@ -1049,6 +1049,7 @@ The request body contains an object that specifies a value for [The input Docume
- **metrics** - Return query performance metrics in addition to result. See [Performance Metrics](#performance-metrics) for more detail.
- **instrument** - Instrument query evaluation and return a superset of performance metrics in addition to result. See [Performance Metrics](#performance-metrics) for more detail.
- **watch** - Set a watch on the data reference if the parameter is present. See [Watches](#watches) for more detail.
- **decision_id** - The decision id to use for logging. Allow clients to supply it, instead of auto generated number.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe reword this to:

Client supplied decision ID to use for tracing. If not specified a random ID is generated automatically.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

omerlh added 2 commits June 4, 2019 07:58
Signed-off-by: omerlh <omerl@soluto.com>
Signed-off-by: omerlh <omerl@soluto.com>
srenatus
srenatus previously approved these changes Jun 4, 2019
server/server.go Outdated
@@ -2083,6 +2084,22 @@ func validateParsedQuery(body ast.Body) ([]string, error) {
return unsafeOperators, nil
}

func getStringParam(url *url.URL, name string, ifEmpty string) string {

p, ok := url.Query()[name]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] It's not a deep change, but we could use (Values).Get, too:

if p := url.Query().Get(name); p != "" {
    return p
}
return ifEmpty

It doesn't do much, so it also doesn't matter if we use it or not, I suppose 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! It make the code a bit cleaner :)

Signed-off-by: omerlh <omerl@soluto.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow clients to supply decision id
3 participants