Merge pull request #1060 from simonbaird/success-count-refactor-alt

Success count refactor
enterprise-contract · Oct 3, 2023 · b56e7c8 · b56e7c8
2 parents beb9c41 + d40c6f3
commit b56e7c8
Show file tree

Hide file tree

Showing 6 changed files with 72 additions and 41 deletions.
diff --git 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
@@ -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
@@ -75,15 +75,28 @@
 
 ---
 
-[JUnit output format:stdout - 1]
+[JUnit and AppStudio output format:stdout - 1]
 <testsuites tests="5" failures="1"><testsuite name="Unnamed (${REGISTRY}/acceptance/image@sha256:${REGISTRY_acceptance/image:latest_DIGEST})" tests="5" failures="1" errors="0" id="0" time="" timestamp="${TIMESTAMP}"><properties><property name="image" value="${REGISTRY}/acceptance/image@sha256:${REGISTRY_acceptance/image:latest_DIGEST}"></property><property name="key" value="${known_PUBLIC_KEY_XML}"></property><property name="success" value="false"></property><property name="keyId" value=""></property><property name="signature" value="${IMAGE_SIGNATURE_acceptance/image}"></property></properties><testcase name="builtin.attestation.signature_check: Pass" classname="builtin.attestation.signature_check: Pass"></testcase><testcase name="builtin.attestation.syntax_check: Pass" classname="builtin.attestation.syntax_check: Pass"></testcase><testcase name="builtin.image.signature_check: Pass" classname="builtin.image.signature_check: Pass"></testcase><testcase name="main.acceptor: Pass" classname="main.acceptor: Pass"></testcase><testcase name="main.rejector: Fails always" classname="main.rejector: Fails always"><failure message="Fails always"><![CDATA[Fails always]]></failure></testcase></testsuite></testsuites>
 ---
 
-[JUnit output format:stderr - 1]
+[JUnit and AppStudio output format:stderr - 1]
 Error: success criteria not met
 
 ---
+[JUnit and AppStudio output format:stdout - 2]
+{
+  "timestamp": "${TIMESTAMP}",
+  "namespace": "",
+  "successes": 4,
+  "failures": 1,
+  "warnings": 0,
+  "result": "FAILURE"
+}
+---
+[JUnit and AppStudio output format:stderr - 2]
+Error: success criteria not met
 
+---
 [policy and data sources:stdout - 1]
 {
   "success": false,

diff --git a/features/validate_image.feature b/features/validate_image.feature
@@ -475,7 +475,7 @@ Feature: evaluate enterprise contract
     Then the exit status should be 0
     Then the output should match the snapshot
 
-  Scenario: JUnit output format
+  Scenario: JUnit and AppStudio output format
     Given a key pair named "known"
     Given an image named "acceptance/image"
     Given a valid image signature of "acceptance/image" image signed by the "known" key
@@ -500,6 +500,9 @@ Feature: evaluate enterprise contract
     When ec command is run with "validate image --image ${REGISTRY}/acceptance/image --policy acceptance/ec-policy --rekor-url ${REKOR} --public-key ${known_PUBLIC_KEY} --output junit --show-successes"
     Then the exit status should be 1
     Then the output should match the snapshot
+    When ec command is run with "validate image --image ${REGISTRY}/acceptance/image --policy acceptance/ec-policy --rekor-url ${REKOR} --public-key ${known_PUBLIC_KEY} --output appstudio"
+    Then the exit status should be 1
+    Then the output should match the snapshot
 
   Scenario: Using OCI bundles
     Given a key pair named "known"

diff --git 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
@@ -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,
 		},
 	}