Skip to content
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

Make linter rules of strict compile time checks? #2

Open
anderseknert opened this issue Jan 24, 2023 · 4 comments
Open

Make linter rules of strict compile time checks? #2

anderseknert opened this issue Jan 24, 2023 · 4 comments
Labels

Comments

@anderseknert
Copy link
Member

Since we'll be depending on OPA anyway, it would be useful to include all of the strictness checks from OPA but in the form of linter checks. These would naturally not be described in Rego, but would be displayed as other linter violations without requiring the use of another command (opa check, opa eval, ...) and would have links back to our documentation portal like the other linter rules do.

@anderseknert
Copy link
Member Author

This is "blocked" by open-policy-agent/opa#5815

Most of these should be easy to implement using the AST alone, so will probably go with that rather than relying on the compiler. First one already submitted: #76

@anderseknert
Copy link
Member Author

anderseknert commented Apr 13, 2023

Current status:

  • Duplicate imports
  • Unused local assignments
  • Unused imports
  • input and data reserved keywords
  • Use of deprecated built-ins

Rather than having a "strict" category, I think we can place these in the existing categories, with references pointing to strict mode in the OPA docs.

@anderseknert
Copy link
Member Author

Given how most of the strict checks will be made default in OPA 1.0, I don't think there's any point in having them included in Regal at this point. The two rules that will remain checked only in strict mode are "unused imports" and "unused local assignment". While I would like to have both of those included as Regal linter checks, the API for the OPA compiler wrt strictness checks currently leaves a lot to be desired (as previously mentioned).

I doubt that these two rules have dependencies among them (i.e. that unused local assignment depends on the unused imports check) so perhaps they could be made independent checks exposed via some API. But until that day, this is on hold.

@anderseknert
Copy link
Member Author

Reading this blog helped me understand that making compilation mandatory / opt-out would mean Regal would fail on many policies we're currently able to lint.

Finally, regarding the linter, although a syntax check command called opa check is provided as standard, there are issues with the Rego code executed with Conftest provided by OPA , such as errors occurring when using Conftest's own parse_config function because the syntax cannot be understood.

The reason opa check fails here is of course the custom built-in the command is unaware of. regal lint won't care about that though as long as we're able to parse the file. Both commands allow providing capabilities to help with this, but that's not likely an option new users will be aware of.

This makes me think compilation should be opt-in for regal lint. The language server OTOH should probably make it opt-out, as these are errors most would likely want to have surfaced in their editor without having to run external commands, "check on save", or whatnot.

Either way, in order for us to provide compilation even as an optional feature, we'll most likely need to look into configuration options for defining multiple bundle roots, rather than assuming that we can treat the whole workspace as a bundle.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: TODO
Development

No branches or pull requests

1 participant