scorecard: use TestList for output, exit 1 on test failure (#3427)

changelog/fragments/scorecard-output-api.yaml

@@ -0,0 +1,15 @@
+# entries is a list of entries to include in
+# release notes and/or the migration guide
+entries:
+  - description: >
+      Changed scorecard text and json output to use a `v1alpha3.TestList`
+      instead of aggregating all test results under a single
+      `v1alpha3.Test` and set exit status to 1 when a test fails.
+    kind: "change"
+    breaking: true
+    migration:
+      header: Alpha scorecard output API updates
+      body: |
+        Update any scripts interpretting the scorecard output to
+        understand the v1alpha3 TestList format.
+        

cmd/operator-sdk/alpha/scorecard/cmd.go

@@ -86,24 +86,20 @@ If the argument holds an image tag, it must be present remotely.`,
 	return scorecardCmd
 }
 
-func (c *scorecardCmd) printOutput(output v1alpha3.Test) error {
+func (c *scorecardCmd) printOutput(output v1alpha3.TestList) error {
 	switch c.outputFormat {
 	case "text":
-		if len(output.Status.Results) == 0 {
+		if len(output.Items) == 0 {
 			fmt.Println("0 tests selected")
 			return nil
 		}
-		o, err := output.MarshalText()
-		if err != nil {
-			fmt.Println(err.Error())
-			return err
+		for _, test := range output.Items {
+			fmt.Println(test.MarshalText())
 		}
-		fmt.Printf("%s\n", o)
 	case "json":
 		bytes, err := json.MarshalIndent(output, "", "  ")
 		if err != nil {
-			fmt.Println(err.Error())
-			return err
+			return fmt.Errorf("marshal json error: %v", err)
 		}
 		fmt.Printf("%s\n", string(bytes))
 	default:
@@ -154,12 +150,9 @@ func (c *scorecardCmd) run() (err error) {
 		return fmt.Errorf("could not parse selector %w", err)
 	}
 
-	var scorecardTest v1alpha3.Test
+	var scorecardTests v1alpha3.TestList
 	if c.list {
-		scorecardTest, err = o.ListTests()
-		if err != nil {
-			return fmt.Errorf("error listing tests %w", err)
-		}
+		scorecardTests = o.List()
 	} else {
 		runner := scorecard.PodTestRunner{
 			ServiceAccount: c.serviceAccount,
@@ -178,13 +171,31 @@ func (c *scorecardCmd) run() (err error) {
 		ctx, cancel := context.WithTimeout(context.Background(), c.waitTime)
 		defer cancel()
 
-		scorecardTest, err = o.RunTests(ctx)
+		scorecardTests, err = o.Run(ctx)
 		if err != nil {
 			return fmt.Errorf("error running tests %w", err)
 		}
 	}
 
-	return c.printOutput(scorecardTest)
+	if err := c.printOutput(scorecardTests); err != nil {
+		log.Fatal(err)
+	}
+
+	if hasFailingTest(scorecardTests) {
+		os.Exit(1)
+	}
+	return nil
+}
+
+func hasFailingTest(list v1alpha3.TestList) bool {
+	for _, t := range list.Items {
+		for _, r := range t.Status.Results {
+			if r.State != v1alpha3.PassState {
+				return true
+			}
+		}
+	}
+	return false
 }
 
 func (c *scorecardCmd) validate(args []string) error {

internal/scorecard/alpha/config.go

@@ -26,12 +26,12 @@ const (
 )
 
 type Test struct {
-	Name  string `yaml:"name"`  // The container test name
-	Image string `yaml:"image"` // The container image name
-	// An list of commands and arguments passed to the test image
-	Entrypoint  []string          `yaml:"entrypoint,omitempty"`
-	Labels      map[string]string `yaml:"labels"`      // User defined labels used to filter tests
-	Description string            `yaml:"description"` // User readable test description
+	// Image is the name of the testimage
+	Image string `json:"image"`
+	// Entrypoint is list of commands and arguments passed to the test image
+	Entrypoint []string `json:"entrypoint,omitempty"`
+	// Labels that further describe the test and enable selection
+	Labels map[string]string `json:"labels,omitempty"`
 }
 
 // Config represents the set of test configurations which scorecard

internal/scorecard/alpha/examples/custom-scorecard-tests/bundle/tests/scorecard/config.yaml

@@ -1,19 +1,15 @@
 tests:
-- name: "customtest1"
-  image: quay.io/username/custom-scorecard-tests:dev
-  entrypoint: 
+- image: quay.io/username/custom-scorecard-tests:dev
+  entrypoint:
   - custom-scorecard-tests
   - customtest1
   labels:
     suite: custom
     test: customtest1
-  description: an ISV custom test
-- name: "customtest2"
-  entrypoint: 
+- image: quay.io/username/custom-scorecard-tests:dev
+  entrypoint:
   - custom-scorecard-tests
   - customtest2
-  image: quay.io/username/custom-scorecard-tests:dev
   labels:
     suite: custom
     test: customtest2
-  description: an ISV custom test

internal/scorecard/alpha/formatting.go

@@ -18,47 +18,41 @@ import (
 	"context"
 	"encoding/json"
 
-	"github.com/operator-framework/operator-sdk/pkg/apis/scorecard/v1alpha3"
 	v1 "k8s.io/api/core/v1"
+
+	"github.com/operator-framework/operator-sdk/pkg/apis/scorecard/v1alpha3"
 )
 
 // getTestResult fetches the test pod log and converts it into
 // Test format
-func (r PodTestRunner) getTestStatus(ctx context.Context, p *v1.Pod, test Test) (output *v1alpha3.TestStatus) {
-
+func (r PodTestRunner) getTestStatus(ctx context.Context, p *v1.Pod) (output *v1alpha3.TestStatus) {
 	logBytes, err := getPodLog(ctx, r.Client, p)
 	if err != nil {
-		return testStatusError(err, test)
+		return convertErrorToStatus(err, string(logBytes))
 	}
 	// marshal pod log into TestResult
 	err = json.Unmarshal(logBytes, &output)
 	if err != nil {
-		return testStatusError(err, test)
+		return convertErrorToStatus(err, string(logBytes))
 	}
 	return output
 }
 
-// ListTests lists the scorecard tests as configured that would be
+// List lists the scorecard tests as configured that would be
 // run based on user selection
-func (o Scorecard) ListTests() (output v1alpha3.Test, err error) {
+func (o Scorecard) List() v1alpha3.TestList {
+	output := v1alpha3.NewTestList()
 	tests := o.selectTests()
-	if len(tests) == 0 {
-		return output, err
-	}
 
 	for _, test := range tests {
-		output.Status.Results = append(output.Status.Results, v1alpha3.TestResult{Name: test.Name})
+		item := v1alpha3.NewTest()
+		item.Spec = v1alpha3.TestSpec{
+			Image:      test.Image,
+			Entrypoint: test.Entrypoint,
+			Labels:     test.Labels,
+		}
+		output.Items = append(output.Items, item)
 	}
 
-	return output, err
-}
-
-func testStatusError(err error, test Test) *v1alpha3.TestStatus {
-	r := v1alpha3.TestResult{}
-	r.Name = test.Name
-	r.State = v1alpha3.FailState
-	r.Errors = []string{err.Error()}
-	return &v1alpha3.TestStatus{
-		Results: []v1alpha3.TestResult{r},
-	}
+	return output
 }

internal/scorecard/alpha/formatting_test.go

@@ -18,7 +18,6 @@ import (
 	"path/filepath"
 	"testing"
 
-	"github.com/operator-framework/operator-sdk/pkg/apis/scorecard/v1alpha3"
 	"k8s.io/apimachinery/pkg/labels"
 )
 
@@ -27,10 +26,9 @@ func TestList(t *testing.T) {
 	cases := []struct {
 		bundlePathValue string
 		selector        string
-		wantError       bool
 		resultCount     int
 	}{
-		{"testdata/bundle", "suite=basic", false, 1},
+		{"testdata/bundle", "suite=basic", 1},
 	}
 
 	for _, c := range cases {
@@ -50,17 +48,8 @@ func TestList(t *testing.T) {
 			}
 			runner.BundlePath = c.bundlePathValue
 			o.TestRunner = &runner
-			var output v1alpha3.Test
-			output, err = o.ListTests()
-			if err == nil && c.wantError {
-				t.Fatalf("Wanted error but got no error")
-			} else if err != nil {
-				if !c.wantError {
-					t.Fatalf("Wanted result but got error: %v", err)
-				}
-				return
-			}
-			actualResultCount := len(output.Status.Results)
+			output := o.List()
+			actualResultCount := len(output.Items)
 			if c.resultCount != actualResultCount {
 				t.Fatalf("Wanted result count %d but got : %d", c.resultCount, actualResultCount)
 			}

internal/scorecard/alpha/labels_test.go

@@ -66,64 +66,50 @@ func TestEmptySelector(t *testing.T) {
 }
 
 const testConfig = `tests:
-- name: "customtest1"
-  image: quay.io/someuser/customtest1:v0.0.1
+- image: quay.io/someuser/customtest1:v0.0.1
   entrypoint: 
   - custom-test
   labels:
     suite: custom
     test: customtest1
-  description: an ISV custom test that does...
-- name: "customtest2"
-  image: quay.io/someuser/customtest2:v0.0.1
+- image: quay.io/someuser/customtest2:v0.0.1
   entrypoint: 
   - custom-test
   labels:
     suite: custom
     test: customtest2
-  description: an ISV custom test that does...
-- name: "basic-check-spec"
-  image: quay.io/redhat/basictests:v0.0.1
+- image: quay.io/redhat/basictests:v0.0.1
   entrypoint: 
   - scorecard-test
   - basic-check-spec
   labels:
     suite: basic
     test: basic-check-spec-test
-  description: check the spec test
-- name: "basic-check-status"
-  image: quay.io/redhat/basictests:v0.0.1
+- image: quay.io/redhat/basictests:v0.0.1
   entrypoint: 
   - scorecard-test
   - basic-check-status
   labels:
     suite: basic
     test: basic-check-status-test
-  description: check the status test
-- name: "olm-bundle-validation"
-  image: quay.io/redhat/olmtests:v0.0.1
+- image: quay.io/redhat/olmtests:v0.0.1
   entrypoint: 
   - scorecard-test
   - olm-bundle-validation
   labels:
     suite: olm
     test: olm-bundle-validation-test
-  description: validate the bundle test
-- name: "olm-crds-have-validation"
-  image: quay.io/redhat/olmtests:v0.0.1
+- image: quay.io/redhat/olmtests:v0.0.1
   entrypoint: 
   - scorecard-test
   - olm-crds-have-validation
   labels:
     suite: olm
     test: olm-crds-have-validation-test
-  description: CRDs have validation
-- name: "kuttl-tests"
-  image: quay.io/redhat/kuttltests:v0.0.1
+- image: quay.io/redhat/kuttltests:v0.0.1
   labels:
     suite: kuttl
   entrypoint:
   - kuttl-test
   - olm-status-descriptors
-  description: Kuttl tests
-`
+  `

internal/scorecard/alpha/run_test.go

@@ -20,8 +20,9 @@ import (
 	"testing"
 	"time"
 
-	"github.com/operator-framework/operator-sdk/pkg/apis/scorecard/v1alpha3"
 	"k8s.io/apimachinery/pkg/labels"
+
+	"github.com/operator-framework/operator-sdk/pkg/apis/scorecard/v1alpha3"
 )
 
 func TestRunTests(t *testing.T) {
@@ -79,11 +80,11 @@ func TestRunTests(t *testing.T) {
 
 			ctx, cancel := context.WithTimeout(context.Background(), time.Duration(7*time.Second))
 			defer cancel()
-			var scorecardOutput v1alpha3.Test
-			scorecardOutput, err = o.RunTests(ctx)
+			var scorecardOutput v1alpha3.TestList
+			scorecardOutput, err = o.Run(ctx)
 
-			if scorecardOutput.Status.Results[0].State != c.expectedState {
-				t.Fatalf("Wanted state %v, got %v", c.expectedState, scorecardOutput.Status.Results[0].State)
+			if scorecardOutput.Items[0].Status.Results[0].State != c.expectedState {
+				t.Fatalf("Wanted state %v, got %v", c.expectedState, scorecardOutput.Items[0].Status.Results[0].State)
 			}
 
 			if err == nil && c.wantError {