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

Split rule type format to show explicit ingestion and evaluation #758

Merged
merged 7 commits into from
Aug 28, 2023

Conversation

JAORMX
Copy link
Contributor

@JAORMX JAORMX commented Aug 25, 2023

This makes it a little bit more explicit what parts of the rule configures what functionality. Hopefully, making the rules less confusing.

Now that we've separated the rule as we have, I've also separated the code which also makes it easier to test!

jhrozek
jhrozek previously approved these changes Aug 25, 2023
Copy link
Contributor

@jhrozek jhrozek left a comment

Choose a reason for hiding this comment

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

I like this improvement

yrobla
yrobla previously approved these changes Aug 25, 2023
@JAORMX JAORMX changed the title Split rule type format to show explicit ingestion and evaluation WIP: Split rule type format to show explicit ingestion and evaluation Aug 25, 2023
@JAORMX JAORMX dismissed stale reviews from yrobla and jhrozek via decf1ad August 25, 2023 13:28
@JAORMX JAORMX requested a review from yrobla August 25, 2023 13:28
This makes it a little bit more explicit what parts of the rule configures
what functionality. Hopefully making the rules less confusing.
This refactors the functionality from ingestors and evaluators, thus making the code
easier to track and more testable.

It also implements the protobuf changes.
This makes sure that our error messages print out the assertion
that didn't match.
@JAORMX JAORMX changed the title WIP: Split rule type format to show explicit ingestion and evaluation Split rule type format to show explicit ingestion and evaluation Aug 25, 2023
@JAORMX JAORMX requested a review from jhrozek August 25, 2023 14:12
Copy link
Contributor

@jhrozek jhrozek left a comment

Choose a reason for hiding this comment

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

there's only one nit that you can as well fix in an upcoming PR

@@ -17,8 +17,8 @@ def:
enabled:
type: boolean
default: true
# Defines the configuration for both ingesting and evaluating the rule.
data_eval:
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you also fix the example of the rule_type in docs/docs/manage_policies.md? I wonder if the doc should instead just have an URL that points to an in-tree example rather than an embedded one to decrease the maintenance load.

This can be done in a separate PR, just should be done to avoid people being irritated by outdated docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the reminder! I'll do that in another PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here it is #767

@JAORMX JAORMX merged commit 5d93f85 into main Aug 28, 2023
14 checks passed
@JAORMX JAORMX deleted the rule-format-split branch August 28, 2023 05:25
JAORMX added a commit that referenced this pull request Aug 28, 2023
Now that #758 merged, we need to update
the docs to reflect the new rule type syntax.
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.

4 participants