-
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
Adding --rego-v1
flag to check
cmd
#6430
Adding --rego-v1
flag to check
cmd
#6430
Conversation
When enabled, checked module must be compliant with OPA 1.0 Rego. Fixes: open-policy-agent#6429 Signed-off-by: Johan Fylling <johan.dev@fylling.se>
Signed-off-by: Johan Fylling <johan.dev@fylling.se>
2b38bc7
to
2c4df5f
Compare
}`, | ||
expErrs: []string{ | ||
"test.rego:2: rego_parse_error: var cannot be used for rule name", // This error actually appears three times: once for 'p'; once for 'contains'; and once for 'x'. All are interpreted as [invalid] rule declarations with no value and body. | ||
"test.rego:2: rego_parse_error: `if` keyword is required before rule body", |
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 error seems weird 🤔
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.
Also if this policy is v1.0 compliant and rego-v1
enabled, why do we generate an error here? If the flag wasn't set then this would be an error. If the purpose of the flag is to check for Rego v1 compatibility then this should not be an error, correct? Or if this is not the expected behavior, then let's point this out in the command description.
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.
Hm, yeah, the if
error might be a bit confusing here .. I'll see if there is a way around it.
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.
The --rego-v1
flag actually doesn't check for only 1.0 compatibility. We still need the policy to also be compatible with the current 0.x version of OPA, which implies either rego.v1
or future.keywords
imports (which won't be requirements in 1.0). I'll update the description for the flag.
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. Just one comment about the v1 compatible and non-v0 compatible use-case.
Signed-off-by: Johan Fylling <johan.dev@fylling.se>
When enabled, checked module(s) must be compliant with OPA 1.0 Rego.
Fixes: #6429