-
Notifications
You must be signed in to change notification settings - Fork 43
Upgrade opa from 0.x to 1.x #2652
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
Conversation
|
/retest |
7920834 to
b8d3fb1
Compare
Do it consistently in each of the go.mod files. This is a big jump and likely includes some breaking changes, which I'll aim to fix in upcoming commits. Ref: https://issues.redhat.com/browse/EC-1130
The github.com/open-policy-agent/opa/cmd import is an exception, that one doesn't have a v1 subpath. Ref: https://issues.redhat.com/browse/EC-1130
Now it's a global config, rather than being attached to each node (or something like that). See open-policy-agent/opa#7281 for more details. Ref: https://issues.redhat.com/browse/EC-1130 Co-authored-by: Claude 4 Sonnet
Related to the opa upgrade. (With this change I think we have a good chance that all the CI is passing.) Ref: https://issues.redhat.com/browse/EC-1130
Since we wrap opa, all the `ec opa` command line docs are generated from the `opa` command line docs. These changes are what you get when running `make generate`. Ref: https://issues.redhat.com/browse/EC-1130
This was useful figuring out the fix in the next commit. Let's leave it in since it's pretty harmless and maybe it will be useful again one day.
Firstly, it causes the "check for uncommitted diffs" CI step to mysteriously fail in GitHub due to mismatch in the generated docs. Here's an example of the diff in docs/modules/ROOT/pages/ec_opa_test.adoc: --p, --parallel:: the number of tests that can run in parallel, defaulting to the number of CPUs (explicitly set with 0). Benchmarks are always run sequentially. (Default: 16) +-p, --parallel:: the number of tests that can run in parallel, defaulting to the number of CPUs (explicitly set with 0). Benchmarks are always run sequentially. (Default: 4) Secondly, having the number of CPUs hard-coded in static docs does not make much sense. Notes: - Originally I implemented this workaround in the template internal/documentation/asciidoc/cli/cli.tmp which worked fine, but made the template quite messy. - See relevant opa code here: https://github.com/open-policy-agent/opa/blob/d0c0ae9730b1ecb06a29c341c707c265138f0494/cmd/test.go#L569C69-L569C76 - This is was tough one to debug! Ref: https://issues.redhat.com/browse/EC-1130
Just a little cleanup I wanted to do after creating the previous commit. Note that f.Deprecated and f.ShorthandDeprecated are both strings so that's why I think we can just compare to "" rather than use len(..) == 0, and also why I think we can skip the extra len > 0 check.
|
I gave this a little test against the real policies, and it seems to work fine. |
|
|
||
| _, err = evaluator.Evaluate(ctx, EvaluationTarget{Inputs: []string{path.Join(dir, "inputs")}}) | ||
| require.Error(t, err) | ||
| assert.EqualError(t, err, `the rule "deny = true if { true }" returns an unsupported value, at no_msg.rego:5`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line got nuked by Cursor in #2639 for some reason . I put it back when rebasing.
| --ignore:: set file and directory names to ignore during loading (e.g., '.*' excludes hidden files) (Default: []) | ||
| -m, --max-errors:: set the number of errors to allow before compilation fails early (Default: 10) | ||
| -r, --run:: run only test cases matching the regular expression. | ||
| -p, --parallel:: the number of tests that can run in parallel, defaulting to the number of CPUs (explicitly set with 0). Benchmarks are always run sequentially. (Default: 16) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...notice (Default: 16) here and see later commits for the fun it caused.
|
I'll probably merge it later today FYI. |
Ref: https://issues.redhat.com/browse/EC-1130
For reviewers, I suggest looking at commits one by one.