-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
cmd & format: Adding rego-v1 mode to opa fmt
#6413
cmd & format: Adding rego-v1 mode to opa fmt
#6413
Conversation
Fixes: open-policy-agent#6297 Signed-off-by: Johan Fylling <johan.dev@fylling.se>
Signed-off-by: Johan Fylling <johan.dev@fylling.se>
Signed-off-by: Johan Fylling <johan.dev@fylling.se>
* duplicate imports * data/input root document overrides * deprecated built-in usage Signed-off-by: Johan Fylling <johan.dev@fylling.se>
Signed-off-by: Johan Fylling <johan.dev@fylling.se>
Signed-off-by: Johan Fylling <johan.dev@fylling.se>
✅ Deploy Preview for openpolicyagent ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Conflicts: ast/compile.go
@@ -0,0 +1,112 @@ | |||
package ast |
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.
We're now exposing internal compiler logic here. Should be put this in an internal
package instead?
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 tried that, but since we also need to call this from the compiler which lives in the ast
package which is heavily used from these functions, we get a circular import that fails compilation.
If there aren't clever ways around that, I suppose the alternative is to duplicate the functionality, so it can be used both in the compiler and in the formatter without making it public.
Perhaps the lesser evil is to eat the redundancy and not make this public .. I don't know 🤔. What do you think?
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 would say let's try to minimize the replication as much as we can. Exposing these methods does not seem like a good idea. Moving them to an internal
package will require a lot of refactoring.
Signed-off-by: Johan Fylling <johan.dev@fylling.se>
Conflicts: ast/compile.go
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. The test coverage is really good. On rewriting of the deprecated built-ins, if you could create an issue to track this work that would be great!
Adding
--rego-v1
flag toopa fmt
command.Calls to deprecated built-ins are not rewritten to equivalent v1-compliant code; we can add this separately.
Fixes: #6297