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

Let policy flag accept directories. #284

Merged
merged 5 commits into from
Aug 1, 2024

Conversation

MadVikingGod
Copy link
Contributor

fixes #259

This change allows --policy flags to accept a directory and include any *.rego file found. It does not recurse into sub directories.

@MadVikingGod MadVikingGod marked this pull request as ready for review August 1, 2024 13:27
src/util.rs Fixed Show fixed Hide fixed
src/util.rs Fixed Show fixed Hide fixed
Copy link
Contributor

@lquerel lquerel left a comment

Choose a reason for hiding this comment

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

Overall, LGTM. Thank you for this contribution. I made a few suggestions. Before merging, could you also update the comment on lines 35-36 in src/registry/check.rs with the following text: ‘List policy files or directories containing policy files to verify against the semantic convention registry.’ There are other sub-commands like generate, resolve, etc., where you should also make this change. Thanks.

crates/weaver_checker/src/lib.rs Outdated Show resolved Hide resolved
crates/weaver_checker/src/lib.rs Outdated Show resolved Hide resolved
src/util.rs Outdated Show resolved Hide resolved
assert_eq!(1, engine.policy_package_count);

_ = engine.add_policy_from_file_or_dir("data/multi-policies")?;
// TODO: add_policies double counts the number of files it adds.
Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed, there is double counting in add_policies. It would be good to take advantage of this PR to fix the bug. Thank you.

crates/weaver_checker/src/lib.rs Fixed Show fixed Hide fixed
crates/weaver_checker/src/lib.rs Fixed Show fixed Hide fixed
@lquerel lquerel merged commit e3b17ad into open-telemetry:main Aug 1, 2024
22 checks passed
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.

Facilitate the import of multiple rego files in weaver registry check
2 participants