-
Notifications
You must be signed in to change notification settings - Fork 905
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
Refactor rules loading result #2098
Merged
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
3845b3f
Add a load result interface for use in new load_rules methods
mstemm 1e1960e
Falco engine changes to support load_rules result class
mstemm 561ecaf
Change filter_warning_resolver to use warning codes
mstemm 14e58f9
Modify rule reader to use a result struct
mstemm fd194dd
Rule loader changes to support result objects
mstemm fc19149
Falco application changes to support rule loading result struct
mstemm 766f2e6
Update tests to use result struct + json-based validation
mstemm File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
So basically
message
andmessage_contains
seem to be mandatory, right? Should we make it optional? Should we document this somewhere?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.
message_contains is only used for the test engine_version_mismatch, where the error message will be dependent on the current embedded falco engine version e.g. "Rules require engine version 9999999, but engine version is 14". If it's a big deal I could switch everything to be a substring match, but I thought the distinction was useful.
I believe all of the other error messages are static--they used to contain "in rule xxx" snippets but all of that info is in the context instead. The engine version is a strange dynamic thing that doesn't really fit into a context.