sdk: Allow use of a query tracer #5447
Conversation
✅ Deploy Preview for openpolicyagent ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
| @@ -501,6 +507,7 @@ func evaluate(ctx context.Context, args evalArgs) (interface{}, ast.Value, map[s | |||
| rego.EvalMetrics(args.m), | |||
| rego.EvalInterQueryBuiltinCache(args.interQueryCache), | |||
| rego.EvalNDBuiltinCache(args.ndbcache), | |||
| rego.EvalQueryTracer(args.tracer), | |||
There was a problem hiding this comment.
I had tried to set this on the rego built above but newEvalContext clears the queryTracers:
https://github.com/open-policy-agent/opa/blob/a738dfcc5539c4b015c8a1e7c4577edf2acac1c0/rego/rego.go#L325
https://github.com/charlieegan3/opa/blob/a738dfcc5539c4b015c8a1e7c4577edf2acac1c0/rego/rego.go#L325
I think that this must be correct, but confirmation appreciated :)
sdk/opa_test.go
Outdated
| }`, | ||
| allow { | ||
| erroring_function(1) | ||
| }`, |
There was a problem hiding this comment.
Some adjustments to match rest of file indentation, missed in #5438
There was a problem hiding this comment.
Interesting. I'd have gone the other way, removing the spacing before package example. But it's fine either way, and I don't believe we've got a convention.
There was a problem hiding this comment.
😄 me too, but I just wanted to match what was already there in the rest of the file. I can unindent them all since that does seem to be more correct.
There was a problem hiding this comment.
815d5b75 updates the rego examples in the sdk tests to use this convention.
sdk/opa_test.go
Outdated
| t.Errorf("expected Enter event, got %v", events[1].Op) | ||
| } | ||
| if events[2].Op != "Note" { | ||
| t.Errorf("expected Enter event, got %v", events[2].Op) |
There was a problem hiding this comment.
I believe that I could make these tests look nicer with testify assert. I see we have this elsewhere, but it's not used here in this file. Wdyt? I could reformat the other tests here for consistency too if you'd like.
There was a problem hiding this comment.
So historically, we've refrained from using anything else but pure golang for our tests; maybe with a bit of reflect.DeepEqual when it's convenient.
Any use of testify you find should be limited to "manually vendored" (i.e. copied) third-party code.
A pattern I personally like is this
if exp, act := "Note", events[2].Op; exp != act {
t.Errorf("event 2, expected %v, got %v", exp, act)
}There was a problem hiding this comment.
Ok, I can switch it to look more like that where appropriate.
Any use of testify you find should be limited to "manually vendored"
Ahh, I can see that now! Thanks.
There was a problem hiding this comment.
b7270d26 alters the new tests to this format.
There was a problem hiding this comment.
LGTM. Some comments on the tests, but since our tests are a hotchpotch of different styles, it's OK to keep them as-is.
I'm trying to remember the rationale for not using a testing lib; but I think it's mostly been us trying to keep it as simple as possible. 🤔
sdk/opa_test.go
Outdated
| }`, | ||
| allow { | ||
| erroring_function(1) | ||
| }`, |
There was a problem hiding this comment.
Interesting. I'd have gone the other way, removing the spacing before package example. But it's fine either way, and I don't believe we've got a convention.
sdk/opa_test.go
Outdated
| t.Errorf("expected Enter event, got %v", events[1].Op) | ||
| } | ||
| if events[2].Op != "Note" { | ||
| t.Errorf("expected Enter event, got %v", events[2].Op) |
There was a problem hiding this comment.
So historically, we've refrained from using anything else but pure golang for our tests; maybe with a bit of reflect.DeepEqual when it's convenient.
Any use of testify you find should be limited to "manually vendored" (i.e. copied) third-party code.
A pattern I personally like is this
if exp, act := "Note", events[2].Op; exp != act {
t.Errorf("event 2, expected %v, got %v", exp, act)
}
✅ Deploy Preview for openpolicyagent ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
This PR exposes functionality to the callers of opa.Decision & opa.Partial allowing a tracer implementing topdown.QueryTracer to be passed. Signed-off-by: Charlie Egan <charlieegan3@users.noreply.github.com>
This is intended to make them easier to read and formatted more consistently. Signed-off-by: Charlie Egan <charlieegan3@users.noreply.github.com>
Refactored tests based on comment here: #5447 (comment) Signed-off-by: Charlie Egan <charlieegan3@users.noreply.github.com>
We have testify in our go.mod. This sometimes misleads contributors (including myself!) to think that we can use testify in test code. Since testify is a common testing package, many go developers default to using it. I'm not sure if we want to merge this as we might rather leave these vendored packages untouched, but it was 10 minutes of work and I was interested to see if we can avoid issues like this in future: * #5753 (comment) * #5447 (comment) * #3952 (review) Signed-off-by: Charlie Egan <charlie@styra.com>
This PR exposes functionality to the callers of
opa.Decision&opa.Partialallowing a tracer implementingtopdown.QueryTracerto be passed.Related to #5176
Signed-off-by: Charlie Egan charlieegan3@users.noreply.github.com