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

feat: hide successful policy check results when --quiet-policy-checks is set with multiple projects #5168

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

joec4i
Copy link

@joec4i joec4i commented Dec 15, 2024

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

  • Unit tests
  • E2E tests

Before

Note: the bot's username is masked.

before

After

after

references

@joec4i joec4i requested review from a team as code owners December 15, 2024 22:14
@joec4i joec4i requested review from jamengual, lukemassa and X-Guardian and removed request for a team December 15, 2024 22:14
@dosubot dosubot bot added feature New functionality/enhancement go Pull requests that update Go code labels Dec 15, 2024
@joec4i joec4i force-pushed the quiet-policy-check-improvement branch from d2b193e to b22a3aa Compare December 15, 2024 22:16
@github-actions github-actions bot added build Relating to how we build Atlantis docs Documentation dependencies PRs that update a dependency file provider/github github-actions website blog labels Dec 15, 2024
@joec4i joec4i force-pushed the quiet-policy-check-improvement branch from b22a3aa to d092a44 Compare December 15, 2024 22:17
@joec4i joec4i changed the title Hide successful policy check results when --quiet-policy-checks is set with multiple projects feat: hide successful policy check results when --quiet-policy-checks is set with multiple projects Dec 15, 2024
@X-Guardian
Copy link
Contributor

Thanks for this @joec4i. Can you add before and after example results to the PR description as evidence of your change.

@X-Guardian X-Guardian added waiting-on-response Waiting for a response from the user feature New functionality/enhancement and removed blog feature New functionality/enhancement build Relating to how we build Atlantis docs Documentation dependencies PRs that update a dependency file provider/github github-actions website labels Dec 17, 2024
@joec4i
Copy link
Author

joec4i commented Dec 17, 2024

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!

Copy link
Contributor

@X-Guardian X-Guardian left a 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
Copy link
Contributor

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?

Copy link
Author

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)
Copy link
Contributor

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)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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?

@joec4i joec4i force-pushed the quiet-policy-check-improvement branch 2 times, most recently from 02a462c to 26a07a5 Compare December 19, 2024 05:30
…t with multiple projects

Signed-off-by: Joe Cai <joe.cai@bigcommerce.com>
…sistency

Signed-off-by: Joe Cai <joe.cai@bigcommerce.com>
@joec4i joec4i force-pushed the quiet-policy-check-improvement branch from 26a07a5 to c118111 Compare December 19, 2024 05:31
@joec4i
Copy link
Author

joec4i commented Dec 19, 2024

Thanks again for your review, @X-Guardian . I've updated the test cases. Please have another look! Cheers!

@joec4i joec4i requested a review from X-Guardian December 19, 2024 22:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New functionality/enhancement go Pull requests that update Go code waiting-on-response Waiting for a response from the user
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants