From d40c6f3fe21635d0b16fe8ea15169de72f502662 Mon Sep 17 00:00:00 2001 From: Simon Baird Date: Fri, 29 Sep 2023 14:12:26 -0400 Subject: [PATCH] Show total checks passed in successes count Previously the "successes" field in the appstudio output showed just a count of each component with an overal pass. Change it to show a count of the number of passing checks. It seems more intuitive to do it this way, and I think probably the reason it's like this is because the relevant code pre-dates the ability to know how many passing checks there were. Notes: - To ensure the success count is present even when --show-success isn't used, there's an extra field added to the applicationsnapshot.Component struct. - I decide not to include the successCount value in the normal report output, hence the `json:"-"` in the struct definition. Res: https://issues.redhat.com/browse/HACBS-2694 --- cmd/validate/image.go | 6 ++- .../__snapshots__/task_validate_image.snap | 12 ++--- features/__snapshots__/validate_image.snap | 2 +- internal/applicationsnapshot/report.go | 29 +++++++----- internal/applicationsnapshot/report_test.go | 44 +++++++++++-------- 5 files changed, 54 insertions(+), 39 deletions(-) diff --git a/cmd/validate/image.go b/cmd/validate/image.go index 46e29201d..fca7550f6 100644 --- a/cmd/validate/image.go +++ b/cmd/validate/image.go @@ -292,9 +292,13 @@ func validateImageCmd(validate imageValidationFunc) *cobra.Command { res.component.Violations = out.Violations() showSuccesses, _ := cmd.Flags().GetBool("show-successes") res.component.Warnings = out.Warnings() + + successes := out.Successes() + res.component.SuccessCount = len(successes) if showSuccesses { - res.component.Successes = out.Successes() + res.component.Successes = successes } + res.component.Signatures = out.Signatures res.component.Attestations = out.Attestations res.component.ContainerImage = out.ImageURL diff --git a/features/__snapshots__/task_validate_image.snap b/features/__snapshots__/task_validate_image.snap index 581d468cf..6d5b56dd4 100755 --- a/features/__snapshots__/task_validate_image.snap +++ b/features/__snapshots__/task_validate_image.snap @@ -372,25 +372,25 @@ TUF_MIRROR not set. Skipping TUF root initialization. [Strict with warnings:results - 1] { - "TEST_OUTPUT": "{\"timestamp\":\"${TIMESTAMP}\",\"namespace\":\"\",\"successes\":1,\"failures\":0,\"warnings\":1,\"result\":\"WARNING\"}" + "TEST_OUTPUT": "{\"timestamp\":\"${TIMESTAMP}\",\"namespace\":\"\",\"successes\":3,\"failures\":0,\"warnings\":1,\"result\":\"WARNING\"}" } --- [Non strict with warnings:results - 1] { - "TEST_OUTPUT": "{\"timestamp\":\"${TIMESTAMP}\",\"namespace\":\"\",\"successes\":1,\"failures\":0,\"warnings\":1,\"result\":\"WARNING\"}" + "TEST_OUTPUT": "{\"timestamp\":\"${TIMESTAMP}\",\"namespace\":\"\",\"successes\":3,\"failures\":0,\"warnings\":1,\"result\":\"WARNING\"}" } --- [Golden container image:results - 1] { - "TEST_OUTPUT": "{\"timestamp\":\"${TIMESTAMP}\",\"namespace\":\"\",\"successes\":1,\"failures\":0,\"warnings\":0,\"result\":\"SUCCESS\"}" + "TEST_OUTPUT": "{\"timestamp\":\"${TIMESTAMP}\",\"namespace\":\"\",\"successes\":4,\"failures\":0,\"warnings\":0,\"result\":\"SUCCESS\"}" } --- [Initialize TUF succeeds:results - 1] { - "TEST_OUTPUT": "{\"timestamp\":\"${TIMESTAMP}\",\"namespace\":\"\",\"successes\":1,\"failures\":0,\"warnings\":0,\"result\":\"SUCCESS\"}" + "TEST_OUTPUT": "{\"timestamp\":\"${TIMESTAMP}\",\"namespace\":\"\",\"successes\":4,\"failures\":0,\"warnings\":0,\"result\":\"SUCCESS\"}" } --- @@ -475,7 +475,7 @@ success: true { "timestamp": "${TIMESTAMP}", "namespace": "", - "successes": 1, + "successes": 3, "failures": 0, "warnings": 0, "result": "SUCCESS" @@ -488,7 +488,7 @@ true [Outputs are there:results - 1] { - "TEST_OUTPUT": "{\"timestamp\":\"${TIMESTAMP}\",\"namespace\":\"\",\"successes\":1,\"failures\":0,\"warnings\":0,\"result\":\"SUCCESS\"}" + "TEST_OUTPUT": "{\"timestamp\":\"${TIMESTAMP}\",\"namespace\":\"\",\"successes\":3,\"failures\":0,\"warnings\":0,\"result\":\"SUCCESS\"}" } --- diff --git a/features/__snapshots__/validate_image.snap b/features/__snapshots__/validate_image.snap index b8309b603..573f90793 100755 --- a/features/__snapshots__/validate_image.snap +++ b/features/__snapshots__/validate_image.snap @@ -87,7 +87,7 @@ Error: success criteria not met { "timestamp": "${TIMESTAMP}", "namespace": "", - "successes": 0, + "successes": 4, "failures": 1, "warnings": 0, "result": "FAILURE" diff --git a/internal/applicationsnapshot/report.go b/internal/applicationsnapshot/report.go index ac49f51b0..704bd3287 100644 --- a/internal/applicationsnapshot/report.go +++ b/internal/applicationsnapshot/report.go @@ -42,6 +42,7 @@ type Component struct { Warnings []evaluator.Result `json:"warnings,omitempty"` Successes []evaluator.Result `json:"successes,omitempty"` Success bool `json:"success"` + SuccessCount int `json:"-"` Signatures []signature.EntitySignature `json:"signatures,omitempty"` Attestations []attestation.Attestation `json:"attestations,omitempty"` } @@ -198,12 +199,16 @@ func (r *Report) toSummary() summary { c := componentSummary{ TotalViolations: len(cmp.Violations), TotalWarnings: len(cmp.Warnings), - TotalSuccesses: len(cmp.Successes), - Success: cmp.Success, - Name: cmp.Name, - Violations: condensedMsg(cmp.Violations), - Warnings: condensedMsg(cmp.Warnings), - Successes: condensedMsg(cmp.Successes), + + // Because cmp.Successes does not get populated unless the --show-successes + // flag was set, cmp.SuccessCount is used here instead of len(cmp.Successes) + TotalSuccesses: cmp.SuccessCount, + + Success: cmp.Success, + Name: cmp.Name, + Violations: condensedMsg(cmp.Violations), + Warnings: condensedMsg(cmp.Warnings), + Successes: condensedMsg(cmp.Successes), } pr.Components = append(pr.Components, c) } @@ -247,12 +252,12 @@ func (r *Report) toAppstudioReport() TestReport { } hasFailures := false - for _, component := range r.Components { - result.Failures += len(component.Violations) - result.Warnings += len(component.Warnings) - if component.Success { - result.Successes += 1 - } else { + for _, component := range r.toSummary().Components { + result.Failures += component.TotalViolations + result.Warnings += component.TotalWarnings + result.Successes += component.TotalSuccesses + + if !component.Success { // It is possible, although quite unusual, that a component has no // listed violations but is still marked as not successful. hasFailures = true diff --git a/internal/applicationsnapshot/report_test.go b/internal/applicationsnapshot/report_test.go index ae388f948..ecdd391f9 100644 --- a/internal/applicationsnapshot/report_test.go +++ b/internal/applicationsnapshot/report_test.go @@ -314,7 +314,8 @@ func Test_ReportSummary(t *testing.T) { }, }, }, - Success: false, + Success: false, + SuccessCount: 1, }, want: summary{ Components: []componentSummary{ @@ -361,7 +362,8 @@ func Test_ReportSummary(t *testing.T) { }, }, }, - Success: false, + Success: false, + SuccessCount: 1, }, want: summary{ Snapshot: "snappy", @@ -409,12 +411,16 @@ func Test_ReportAppstudio(t *testing.T) { "failures": 0, "namespace": "", "result": "SUCCESS", - "successes": 3, + "successes": 42, "timestamp": "0", "warnings": 0 }`, - components: []Component{{Success: true}, {Success: true}, {Success: true}}, - success: true, + components: []Component{ + {Success: true, SuccessCount: 39}, + {Success: true, SuccessCount: 2}, + {Success: true, SuccessCount: 1}, + }, + success: true, }, { name: "warning", @@ -428,8 +434,8 @@ func Test_ReportAppstudio(t *testing.T) { "warnings": 1 }`, components: []Component{ - {Success: true}, - {Success: true, Warnings: []evaluator.Result{{Message: "this is a warning"}}}, + {Success: true, SuccessCount: 1}, + {Success: true, SuccessCount: 1, Warnings: []evaluator.Result{{Message: "this is a warning"}}}, }, success: true, }, @@ -445,7 +451,7 @@ func Test_ReportAppstudio(t *testing.T) { "warnings": 0 }`, components: []Component{ - {Success: true}, + {Success: true, SuccessCount: 1}, {Success: false, Violations: []evaluator.Result{{Message: "this is a violation"}}}, }, success: false, @@ -461,7 +467,7 @@ func Test_ReportAppstudio(t *testing.T) { "timestamp": "0", "warnings": 0 }`, - components: []Component{{Success: false}, {Success: true}}, + components: []Component{{Success: false}, {Success: true, SuccessCount: 1}}, success: false, }, { @@ -476,7 +482,7 @@ func Test_ReportAppstudio(t *testing.T) { "warnings": 1 }`, components: []Component{ - {Success: true}, + {Success: true, SuccessCount: 1}, {Success: false, Violations: []evaluator.Result{{Message: "this is a violation"}}}, {Success: false, Warnings: []evaluator.Result{{Message: "this is a warning"}}}, }, @@ -508,7 +514,7 @@ func Test_ReportAppstudio(t *testing.T) { "warnings": 0 }`, snapshot: "snappy", - components: []Component{{Success: true}, {Success: true}, {Success: true}}, + components: []Component{{Success: true, SuccessCount: 3}, {Success: true}, {Success: true}}, success: true, }, } @@ -557,11 +563,11 @@ func Test_ReportHACBS(t *testing.T) { "failures": 0, "namespace": "", "result": "SUCCESS", - "successes": 3, + "successes": 42, "timestamp": "0", "warnings": 0 }`, - components: []Component{{Success: true}, {Success: true}, {Success: true}}, + components: []Component{{Success: true, SuccessCount: 42}, {Success: true}, {Success: true}}, success: true, }, { @@ -576,8 +582,8 @@ func Test_ReportHACBS(t *testing.T) { "warnings": 1 }`, components: []Component{ - {Success: true}, - {Success: true, Warnings: []evaluator.Result{{Message: "this is a warning"}}}, + {Success: true, SuccessCount: 1}, + {Success: true, SuccessCount: 1, Warnings: []evaluator.Result{{Message: "this is a warning"}}}, }, success: true, }, @@ -593,7 +599,7 @@ func Test_ReportHACBS(t *testing.T) { "warnings": 0 }`, components: []Component{ - {Success: true}, + {Success: true, SuccessCount: 1}, {Success: false, Violations: []evaluator.Result{{Message: "this is a violation"}}}, }, success: false, @@ -609,7 +615,7 @@ func Test_ReportHACBS(t *testing.T) { "timestamp": "0", "warnings": 0 }`, - components: []Component{{Success: false}, {Success: true}}, + components: []Component{{Success: false}, {Success: true, SuccessCount: 1}}, success: false, }, { @@ -624,7 +630,7 @@ func Test_ReportHACBS(t *testing.T) { "warnings": 1 }`, components: []Component{ - {Success: true}, + {Success: true, SuccessCount: 1}, {Success: false, Violations: []evaluator.Result{{Message: "this is a violation"}}}, {Success: false, Warnings: []evaluator.Result{{Message: "this is a warning"}}}, }, @@ -656,7 +662,7 @@ func Test_ReportHACBS(t *testing.T) { "warnings": 0 }`, snapshot: "snappy", - components: []Component{{Success: true}, {Success: true}, {Success: true}}, + components: []Component{{Success: true, SuccessCount: 1}, {Success: true, SuccessCount: 1}, {Success: true, SuccessCount: 1}}, success: true, }, }