-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
feat: hide successful policy check results when --quiet-policy-checks is set with multiple projects #5168
base: main
Are you sure you want to change the base?
Conversation
d2b193e
to
b22a3aa
Compare
b22a3aa
to
d092a44
Compare
Thanks for this @joec4i. Can you add before and after example results to the PR description as evidence of your change. |
Thanks for reviewing this, @X-Guardian . I've uploaded two screenshots to show the difference. This was taken from our test repo with two test projects. The noise reduction will be even more noticeable in repositories with more projects, which is common for Terraform setups supporting multiple environments. Looking forward to your feedback! |
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.
Looking good. Just a few small refinement suggestions to the tests.
PolicySetResults: []models.PolicySetResult{ | ||
{ | ||
PolicySetName: "policy1", | ||
// strings.Repeat require to get wrapped result |
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 like these strings.Repeat
comments are not relevant here and below?
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.
Nice catch! I copied the setup from TestRenderProjectResults()
. I don't think the comment makes sense there either. So it's removed from both tests.
}, | ||
} | ||
|
||
r := events.NewMarkdownRenderer(false, false, false, false, false, false, "", "atlantis", false, true) |
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.
Can you copy the pattern of the other NewMarkdownRenderer
function calls in this test file and comment on each flag what it is.
@@ -60,7 +60,7 @@ func TestRenderErr(t *testing.T) { | |||
}, | |||
} | |||
|
|||
r := events.NewMarkdownRenderer(false, false, false, false, false, false, "", "atlantis", false) | |||
r := events.NewMarkdownRenderer(false, false, false, false, false, false, "", "atlantis", false, false) |
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.
r := events.NewMarkdownRenderer(false, false, false, false, false, false, "", "atlantis", false, false) | |
r := events.NewMarkdownRenderer( | |
false, // gitlabSupportsCommonMark | |
false, // disableApplyAll | |
false, // disableApply | |
false, // disableMarkdownFolding | |
false, // disableRepoLocking | |
false, // enableDiffMarkdownFormat | |
"", // MarkdownTemplateOverridesDir | |
"atlantis", // executableName | |
false, // hideUnchangedPlanComments | |
false, // quietPolicyChecks | |
) |
Can I get you to update these function calls to comment on each flag what it is, here and below, to make the tests more consistent and readable?
02a462c
to
26a07a5
Compare
…t with multiple projects Signed-off-by: Joe Cai <joe.cai@bigcommerce.com>
…sistency Signed-off-by: Joe Cai <joe.cai@bigcommerce.com>
26a07a5
to
c118111
Compare
Thanks again for your review, @X-Guardian . I've updated the test cases. Please have another look! Cheers! |
what
Improve the
--quiet-policy-checks
flag to only display unsuccessful policy check results when multiple projects are involved.why
Currently, the
--quiet-policy-checks
flag is effective for a single project or when all policy checks pass across multiple projects. However, when there are multiple projects involved, it becomes noisy when one or more checks fail, as it shows both successful and unsuccessful results. This PR hides successful check results when--quiet-policy-checks
is specified in order to reduce noise and allow users to focus on failed checks.tests
Before
Note: the bot's username is masked.
After
references