-
Notifications
You must be signed in to change notification settings - Fork 54
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
chore: update opa v1 with v0 rego compat #518
Conversation
sozercan
commented
Jan 23, 2025
- updates opa to v1
- sets rego to be v0 compat
Signed-off-by: Sertac Ozercan <[email protected]>
constraint/go.mod
Outdated
github.com/google/go-cmp v0.6.0 | ||
github.com/onsi/gomega v1.33.1 | ||
github.com/open-policy-agent/opa v0.68.0 | ||
github.com/open-policy-agent/opa v1.0.1 |
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.
OPA 1.1.0 is out - https://github.com/orgs/open-policy-agent/discussions/654, let's update to latest version if no one has any objections. @open-policy-agent/gatekeeper-maintainers
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.
updated to 1.1.0
Signed-off-by: Sertac Ozercan <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #518 +/- ##
==========================================
- Coverage 54.68% 53.44% -1.24%
==========================================
Files 71 97 +26
Lines 5241 6118 +877
==========================================
+ Hits 2866 3270 +404
- Misses 2073 2513 +440
- Partials 302 335 +33
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
LGTM
- linters: [revive] | ||
text: 'redefines-builtin-id: redefinition of the built-in function new' | ||
exclude: | ||
- 'deprecated: This package is intended for older projects transitioning from OPA v0.x and will remain for the lifetime of OPA v1.x' |
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.
@charlieegan3 @anderseknert this is from https://pkg.go.dev/github.com/open-policy-agent/opa/v1 can you please confirm if switching to github.com/open-policy-agent/opa/v1/ will allow us to continue to use v0 Rego syntax without any issue?
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.
I think this must have been disabled since the project still imports various v0 packages in this branch.
In #517 I have fixed these imports to the v1 versions.
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.
Having a closer look, it doesn't look like any of the imports have been updated to v1. As explained here int he 1.0 docs https://www.openpolicyagent.org/docs/latest/v0-upgrade/#upgrading-for-go-integrations, you can upgrade to the v1 packages and still use the v0 functionality. This is explained in more detail here:
https://www.openpolicyagent.org/docs/latest/v0-compatibility/#v0x-compatibility-mode-in-rego-package
Hey @sozercan, I'd encourage you to use the v1 packages here and disable the silencing of the warning. You can still access the v0 compatibility features in the v1 packages. What would you all like me to do about the PR here: #517? This PR does the same upgrade, but also adds support for v1 and v0 Rego transparently. Any comments there about that idea appreciated too but I can rebase it off these changes once they're in too if needs be. There are some outstanding unknowns for me in the regorewriter component, but see the PR desc for more detail there. |
Thanks @charlieegan3! |
Ok that makes sense, I can keep it around and rebase once we have the inital package upgrade in. Any more info on the regorewriter component in the meantime? |
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.
LGTM
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.
LGTM
Lets design the experience in your PR. |
@charlieegan3 as @ritazh mentioned, this is just a first step to unblock opa v1 usage (with rego v0 as is) before we can implement better design through #517 |