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

More Rego completion providers #858

Merged
merged 1 commit into from
Jun 20, 2024
Merged

More Rego completion providers #858

merged 1 commit into from
Jun 20, 2024

Conversation

anderseknert
Copy link
Member

Rules converted:

  • commonrule
  • package
  • rego.v1

Also:

  • move some common functions into utility package.
  • add Name() to provider interface to be able to easily measure performance impact of a named provider

Rules converted:
- `commonrule`
- `package`
- `rego.v1`

Also:
- move some common functions into utility package.
- add `Name()` to provider interface to be able to easily measure
  performance impact of a named provider

Signed-off-by: Anders Eknert <[email protected]>

suggested_names := {
"allow",
"authorized",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is new... I personally would not be keen for authorized appearing at the start of every line but that might just be me. I would consider result a better third candidate, but would stick to allow deny myself.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like result, as well as report (that we use in Regal) and a few others. allow is fine but deny is IMHO legacy since contains was made a requirement. How does a deny "contain" anything? It just doesn't read right to me. But either way, I'm more keen to add things than to have them removed. These are just suggestions anyway.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah fair enough, I'd never thought about deny like that...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"report contains ..." :chefs-kiss:

Copy link
Member

@charlieegan3 charlieegan3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@charlieegan3 charlieegan3 merged commit 82fd171 into main Jun 20, 2024
3 checks passed
@charlieegan3 charlieegan3 deleted the more-rego-providers branch June 20, 2024 09:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants