-
Notifications
You must be signed in to change notification settings - Fork 35
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: Support the defaulting of rules in config #587
Conversation
47714cb
to
0293f87
Compare
eb4ca03
to
8a54a55
Compare
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 great to me! I guess we'll probably undo some of this if we move things over to Rego later, but that goes for a lot of code in our initialization routine, so... :)
One thing I noticed is how there is no documentation to cover this. Adding a section in the configuration section of the README explaining how this works would be good.
|
||
// Defaults state is loaded from configuration under rules and so is not (un)marshalled | ||
// in the same way. | ||
Defaults Defaults `json:"-" yaml:"-"` |
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.
That's clever!
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.
🤓 all this stuff still feels painful. Let's try a rego based POC of this soon.
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.
100%
filename: "p.rego", | ||
expViolations: []string{"opa-fmt", "top-level-iteration", "rule-shadows-builtin"}, | ||
expLevels: []string{"error", "warning", "warning"}, | ||
}, |
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.
Test cases are good! I wonder if we should add something (e2e) for the case of "category disabled via config file, enabled via CLI" or similar... wouldn't need to be extensive but just something to ensure CLI args have precedence.
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 I can look into doing that since we don't have that covered elsewhere
I've done some manual testing and things work just as I'd expect 👍 |
This PR implements an approach to rule defaulting described in: #153 (comment) Signed-off-by: Charlie Egan <charlie@styra.com>
Signed-off-by: Charlie Egan <charlie@styra.com>
8a54a55
to
ecbd4d9
Compare
Signed-off-by: Charlie Egan <charlie@styra.com>
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! I just had a question, but it's not blocking, so feel free to merge when you want :)
Thanks! 👏
README.md
Outdated
If one of Regal's rules doesn't align with your team's preferences, don't worry! Regal is not meant to be the law, | ||
and some rules may not make sense for your project, or parts of it. | ||
Regal provides several different methods to ignore rules with varying precedence. | ||
The available methods are (ranked High to Lowest precedence): |
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.
Mostly just curious about the use of capitalized High and Lowest here! Also, why is it "High" and not "Highest"?
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.
Ha, oops. That went from High to Low
to High to Lowest precedence
to ranked High to Lowest precedence
and I just was too careless to change the case... not that it even needed to be Title Case in the first place! 😂
expectExitCode(t, err, 3, &stdout, &stderr) | ||
} | ||
|
||
func TestConfigDefaultingWithEnableDirective(t *testing.T) { |
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 is great!
README.md
Outdated
|
||
### Ignoring a Rule Entirely | ||
* [Inline Ignore Directives](#inline-ignore-directives) cannot be overridden by any other method. |
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.
Build is failing because the markdown linter wants dashes in place of the asterisks for unordered lists :P
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.
Caught! I am on team *
but will get that patched up.
Signed-off-by: Charlie Egan <charlie@styra.com>
7fbac66
to
21ff561
Compare
* config: Support the defaulting of rules in config This PR implements an approach to rule defaulting described in: StyraInc#153 (comment) Signed-off-by: Charlie Egan <charlie@styra.com> * Address linter and refactor Signed-off-by: Charlie Egan <charlie@styra.com> * Add e2e test for config defaulting and flag use Signed-off-by: Charlie Egan <charlie@styra.com> * Add new notes for defaulting in README Signed-off-by: Charlie Egan <charlie@styra.com> --------- Signed-off-by: Charlie Egan <charlie@styra.com>
This PR implements an approach to rule defaulting described in: #153 (comment)