-
Notifications
You must be signed in to change notification settings - Fork 327
Add --group flag to override group name for stdin input #1163
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
base: master
Are you sure you want to change the base?
Conversation
- Add --group CLI flag to test command for custom group naming - Override FileName from '-' to custom group name when --group is provided - Enables better identification of policy violations in CI/CD workflows - Add unit tests for group flag functionality Fixes open-policy-agent#1095 Signed-off-by: nikos.nikolakakis <[email protected]>
a39db24 to
9f2b39f
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.
Hi, thanks for taking on this FR.
Thinking about this more, I think calling the flag "group" is a little too vague. What do you think about "file-name-override" instead?
cc @taraspos
| @@ -0,0 +1,77 @@ | |||
| package runner | |||
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.
These tests aren't actually testing the real code. You aren't using the methods on the TestRunner struct. Please add tests using the TestRunner, and add acceptance tests in acceptance.bats.
- Renamed flag from 'group' to 'file-name-override' for clarity - Rewrote tests to use TestRunner.Run() method with proper test fixtures - Added comprehensive acceptance tests for the new flag - Fixed Rego v1 syntax in test policies - Added proper error handling and edge case testing Addresses review feedback from PR open-policy-agent#1163 Signed-off-by: nikos.nikolakakis <[email protected]>
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! Just a couple of minor changes. Thanks for the PR and sorry for the slow review cycle.
| defer func() { os.Stdin = oldStdin }() | ||
|
|
||
| // Run the test | ||
| ctx := context.Background() |
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.
Prefer t.Context() in tests.
| } | ||
| }` | ||
| go func() { | ||
| defer w.Close() |
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.
nit: This defer isn't needed, you can w.Close() after the line below.
| Failures: []output.Result{{Message: "test failure"}}, | ||
| }, | ||
| // Run with a regular file (not stdin) | ||
| ctx := context.Background() |
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.
Prefer t.Context() in tests.
Fixes #1095
This PR adds a --group flag to the conftest test command that allows overriding the group name for stdin input.
Problem: When using conftest with kustomize in loops, output shows generic 'FAIL - - main - message' making it hard to identify which component failed.
Solution: Add --group flag to specify custom group name for better traceability.
Usage:
Output:
FAIL - - main - messageFAIL - my/component - main - messageChanges:
Enables better CI/CD traceability and GitHub annotations.