-
Notifications
You must be signed in to change notification settings - Fork 37
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
config: Allow configuration of capabilities #423
Conversation
a920b0c
to
089d95a
Compare
Looks great @charlieegan3 👍 I think it makes sense to add a |
This is a first pass at making it possible to configure capabilities in regal. I haven't yet updated the regoArgs to use this from the config since I wanted to get some feedback on the impl first. Signed-off-by: Charlie Egan <[email protected]>
089d95a
to
cebcec1
Compare
I've added this in cebcec1, I think it makes sense to have an error when setting the engine and file. |
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.
Looks good! Just two questions, then we can have this merged and start building on top of this 👍
pkg/config/config.go
Outdated
Ignore Ignore `json:"ignore,omitempty" yaml:"ignore,omitempty"` | ||
Rules map[string]Category `json:"rules" yaml:"rules"` | ||
Ignore Ignore `json:"ignore,omitempty" yaml:"ignore,omitempty"` | ||
Capabilities ast.Capabilities `json:"capabilities,omitempty" yaml:"capabilities,omitempty"` |
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.
Weird — the formatting looks off here, but there's no linter rule violation 🤔
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.
Yeah, that doesn't look right... I have fixed it manually in eeda01e
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.
Thanks! I'll look into why the linter didn't flag this.
|
||
config.Capabilities.Builtins = append(config.Capabilities.Builtins, result.Capabilities.Plus.Builtins...) | ||
|
||
return nil |
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.
If no capabilities have been provided, should we default to latest from OPA here?
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've had a go a finishing this off a little more in 1fff993
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.
Awesome! Thanks 😃 Looks good to me.
The one thing I'm not sure about is passing the capabilities from the config to Rego's eval args. Would that not have the linter (i.e. Regal) itself be limited to whatever capabilities the target policy has? I mean, even if we're linting a Rego file with e.g. a pre-contains
OPA version, surely we should be able to use contains in Regal's linter policy?
I think the way we'll want to use the capabilities at least initially, is basically as a datasource for what built-in functions we might encounter in the policy we're linting, and have linter rules be able to declare their dependencies... like how we should skip the custom-has-key-construct if object.keys
is not in the capabilities, and so on.
But if we eval with the provided capabilities, it would mean we can't use object.keys
, no?
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.
Ahh, I see. I think I got the wrong end of the stick! I can take this part out.
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.
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.
Thanks!
cc90095
to
f3a4f11
Compare
Signed-off-by: Charlie Egan <[email protected]>
f3a4f11
to
1fff993
Compare
This reverts commit 1fff993.
Signed-off-by: Charlie Egan <[email protected]>
This is a first pass at making it possible to configure capabilities in regal. Signed-off-by: Charlie Egan <[email protected]>
This is a first pass at making it possible to configure capabilities in regal.
I haven't yet updated the regoArgs to use this from the config since I wanted to get some feedback on the impl first.
Related: #212