ast: Importing rego.v1 in v0 support modules when applicable
#6698
Conversation
| return true | ||
| } | ||
| return false | ||
| }) |
There was a problem hiding this comment.
Need to create tests for this before un-drafting this PR.
There was a problem hiding this comment.
Is it even possible to retain var names such that they conflict with v1 keywords? Or are they all rewritten? Any ideas?
There was a problem hiding this comment.
@srenatus any idea about this?
There was a problem hiding this comment.
We could do something similar for imports in the generated support module, but I believe all references would be locally rewritten to complete refs anyways ..
✅ Deploy Preview for openpolicyagent ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Prioritizing generating v0 Rego with `rego.v1` import when producing support modules for non-`--v1-compatible` optimized builds. Affects `opa build` when the `-O` flag is used for optimization, and `opa eval` for partial evaluation with the `-p` flag. Fixes: open-policy-agent#6450 Signed-off-by: Johan Fylling <johan.dev@fylling.se>
Signed-off-by: Johan Fylling <johan.dev@fylling.se>
ashutosh-narkar
force-pushed
the
rego-v1/v0compat1_support_modules
branch
from
049bcc7
to
80dfbbf
Compare
| query: "data.test.p", | ||
| module: `package test | ||
|
|
||
| import rego.v1 |
There was a problem hiding this comment.
If the module had the future import would the optimized module use the rego import if regoV1ImportCapable: true?
There was a problem hiding this comment.
Correct. When we build the support module, we don't have this information retained. We could probably trace the content of a single support rule back to it's origin module and do whatever relevant imports it does, but I'm not sure that's worth the effort, given that multiple origin modules could be contributing to the same support module, and they can all have their own import scheme.
I think the important thing here is that we produce Rego that is in line with the code style we recommend, which is to use the rego.v1 import.
| input.x == 1 | ||
| }`, | ||
| }, | ||
| // rego.v1 import not used, since rule name conflicts with future keyword |
There was a problem hiding this comment.
Maybe I'm missing something but if we have regoV1ImportCapable: true, then shouldn't there be a compile error?
There was a problem hiding this comment.
No. regoV1ImportCapable is only an indication for if we have the capability to use the rego.v1 import (the rego_v1_import feature is in the capabilities file). We opt-out of using it if there are any existing rules that will conflict with that import; such ase the contains rule in this test case.
| return true | ||
| } | ||
| return false | ||
| }) |
There was a problem hiding this comment.
@srenatus any idea about this?
Signed-off-by: Johan Fylling <johan.dev@fylling.se>
Prioritizing generating v0 Rego with
rego.v1import when producing support modules for non---v1-compatibleoptimized builds.Affects
opa buildwhen the-Oflag is used for optimization, andopa evalfor partial evaluation with the-pflag.Fixes: #6450