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

Add a minimal status option to the report generating command #418

Merged

Conversation

georgibaltiev
Copy link
Contributor

What this PR does / why we need it:
This PR adds an additional flag to the diki report generate command that specifies the minimal status of the checks that should be included in the rendered report. This flag only discards check results that have a higher status when generating a regular or merged report.

Which issue(s) this PR fixes:
Fixes #378

Special notes for your reviewer:

Release note:

`diki report generate` now has a --min-status flag that discards check results with a higher status than the argument.

@georgibaltiev georgibaltiev requested a review from a team as a code owner January 8, 2025 14:16
@gardener-robot gardener-robot added needs/review Needs review size/m Size of pull request is medium (see gardener-robot robot/bots/size.py) labels Jan 8, 2025
@georgibaltiev georgibaltiev changed the title Implement min status report Add a minimal status option to the report generating command Jan 8, 2025
@gardener-robot-ci-3 gardener-robot-ci-3 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Jan 8, 2025
Copy link
Member

@AleksandarSavchev AleksandarSavchev left a comment

Choose a reason for hiding this comment

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

Thanks!

cmd/diki/app/app.go Outdated Show resolved Hide resolved
pkg/report/report.go Outdated Show resolved Hide resolved
pkg/report/report.go Outdated Show resolved Hide resolved
@gardener-robot gardener-robot added the needs/changes Needs (more) changes label Jan 9, 2025
@gardener-robot-ci-1 gardener-robot-ci-1 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Jan 9, 2025
@gardener-robot-ci-3 gardener-robot-ci-3 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Jan 9, 2025
@@ -125,6 +125,26 @@ func (r *Report) WriteToFile(filePath string) error {
return os.WriteFile(filePath, data, 0600)
}

// SetMinStatus sets minStatus of the report. It also removes all checks with status less than the specified one.
// If the new minStatus is less that the original one, nothing is changed.
func (r *Report) SetMinStatus(minStatus rule.Status) {
Copy link
Member

Choose a reason for hiding this comment

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

Can't we implement this as a report option?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will look into it.

Copy link
Member

Choose a reason for hiding this comment

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

You can disregard my comment. I see that we already have this and you are making use of it.

@@ -144,6 +146,7 @@ func addRunFlags(cmd *cobra.Command, opts *runOptions) {
func addReportGenerateFlags(cmd *cobra.Command, opts *generateOptions) {
cmd.PersistentFlags().Var(cliflag.NewMapStringString(&opts.distinctBy), "distinct-by", "If set generates a merged report. The keys are the IDs for the providers which the merged report will include and the values are distinct metadata attributes to be used as IDs for the different reports.")
cmd.PersistentFlags().StringVar(&opts.format, "format", "html", "Format for the output report. Format can be one of 'html' or 'json'.")
cmd.PersistentFlags().StringVar(&opts.minStatus, "min-status", "Passed", "If set specifies the minimal status that will be included in the generated report. Status can be one of 'Passed', 'Skipped', 'Accepted', 'Warning', 'Failed', 'Errored' or 'NotImplemented'")
Copy link
Member

Choose a reason for hiding this comment

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

We should mention the statuses in a sorted order stating which is the highest

@@ -125,6 +125,26 @@ func (r *Report) WriteToFile(filePath string) error {
return os.WriteFile(filePath, data, 0600)
}

// SetMinStatus sets minStatus of the report. It also removes all checks with status less than the specified one.
// If the new minStatus is less that the original one, nothing is changed.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// If the new minStatus is less that the original one, nothing is changed.
// If the new minStatus is less than the original one, nothing is changed.

@@ -125,6 +125,26 @@ func (r *Report) WriteToFile(filePath string) error {
return os.WriteFile(filePath, data, 0600)
}

// SetMinStatus sets minStatus of the report. It also removes all checks with status less than the specified one.
// If the new minStatus is less that the original one, nothing is changed.
func (r *Report) SetMinStatus(minStatus rule.Status) {
Copy link
Member

Choose a reason for hiding this comment

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

You can disregard my comment. I see that we already have this and you are making use of it.

@gardener-robot-ci-3 gardener-robot-ci-3 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Jan 9, 2025
Copy link
Member

@AleksandarSavchev AleksandarSavchev left a comment

Choose a reason for hiding this comment

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

/lgtm

@gardener-robot gardener-robot added reviewed/lgtm Has approval for merging and removed needs/changes Needs (more) changes needs/review Needs review labels Jan 9, 2025
@gardener-robot-ci-3 gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jan 9, 2025
Copy link
Member

@dimityrmirchev dimityrmirchev left a comment

Choose a reason for hiding this comment

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

/lgtm

@georgibaltiev georgibaltiev merged commit f65eae4 into gardener:main Jan 9, 2025
9 checks passed
@georgibaltiev georgibaltiev deleted the implement-min-status-report branch January 9, 2025 09:43
@gardener-robot gardener-robot added the status/closed Issue is closed (either delivered or triaged) label Jan 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) reviewed/lgtm Has approval for merging reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) size/m Size of pull request is medium (see gardener-robot robot/bots/size.py) status/closed Issue is closed (either delivered or triaged)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add ability to set the minimum status of included checks in generated reports
6 participants