V1alpha2 api - scorecard output changes for v1alpha2 version #1995

jmccormick2001 · 2019-09-30T19:11:37Z

Description of the change:

This changes adds a new output format to the scorecard subcommand when you specify
a version=v1alpha2 in the configuration or command line. The following shows the v1alpha1 output on the left and the v1alpha2 format on the right:

Motivation for the change:

These changes are part of the larger effort of creating a new v1alpha2 format for the
scorecard. Additional work remains and would be added in with separate PRs. This
PR mostly impacts the output format that an end user sees in the text format along with
changes to the json format specifically for v1alpha2.

jmccormick2001 · 2019-09-30T20:23:33Z

/test e2e-aws-helm

joelanford

Just some initial comments/questions. Looking forward to trying this out.

joelanford · 2019-09-30T19:20:18Z

internal/pkg/scorecard/scorecard_output.go

+						fmt.Printf("\t%-35s: ", result.Name)
+
+						if result.State == scapiv1alpha1.PassState {
+							fmt.Printf(passColor, scapiv1alpha1.PassState)


It might be nice to check if the process is running in a terminal before using color. That way it'll be easier to read in CI if using text-formatted output.

yep, will add that new dependency and the code looks simple enough to add a check for a terminal, if no terminal is found then no colorization will be done.

added a check for the terminal, if its not a terminal then no colorization is performed.

joelanford · 2019-09-30T19:50:51Z

internal/pkg/scorecard/scorecard_output.go

+	passColor           = "\033[1;32m%s\033[0m\n"
+)
+
+func printPluginOutputs(pluginOutputs []scapiv1alpha1.ScorecardOutput) error {


Nit: Is it possible/would it make sense to declare an interface for output formats that has separate implementations in thev1alpha1 and v1alpha2 packages (possibly by ScorecardOutputList) so that they are decoupled (making it easier to remove v1alpha1 later). Maybe something like:

type ScorecardFormatter interface { MarshalText() (string, error) MarshalJSON() ([]byte, error) }

Then in the main scorecard runner function, we could do all the version and format checks and make the appropriate conversions and format the outputs. That way we can limit the places we have logic that cares about format and version.

I took a shot at this today, creating the interface and the v1/v2 implementations of that interface....seems better in the long term for sure.

I apologize, but looking at your new implementation (which looks great!), I realize that the ScorecardOutputList seems to be for a list of separate scorecard runs? If so, I don't think we have a use case for that type right now (we could probably remove it from v1alpha2).

I was envisioning that whatever type captures the results of a single run would be the one that implements this interface.

yeah, I was wondering about the usefulness of the OutputList, I couldn't see it being used anywhere but was unsure if that was there for some specific future purpose. I'll remove it today and replace it with the ScorecardOutput type.

removed in the latest commit.

joelanford · 2019-09-30T19:52:50Z

internal/pkg/scorecard/helpers/helpers.go

@@ -139,6 +139,15 @@ func UpdateState(res scapiv1alpha1.ScorecardTestResult) scapiv1alpha1.ScorecardT
 	if res.State == scapiv1alpha1.ErrorState {
 		return res
 	}
+
+	if IsV1alpha2() {


Could we move this logic to the ConvertTestResultV1ToV2 function?

yep, will move it.

joelanford · 2019-09-30T19:56:58Z

pkg/apis/scorecard/v1alpha2/types.go

+
+const (
+	// UnsetState is the default state for a TestResult. It must be updated by UpdateState or by the Test.
+	UnsetState State = "unset"


Not sure I understand the need for this to be "unset" vs. just "", but I see it was that way in v1alpha1. Maybe worth changing since "" is the default value of a string?

Do we use UnsetState?

I'll remove that from v1alpha2, I could not find it being used anywhere either.

joelanford · 2019-09-30T19:59:34Z

pkg/apis/scorecard/v1alpha2/types.go

+
+// ScorecardSuiteResult contains the combined results of a suite of tests.
+// +k8s:openapi-gen=true
+type ScorecardSuiteResult struct {


Do we want to continue with the idea of a suite in v1alpha2 (e.g. "Basic", "OLM", etc.), or would a flat list of tests be sufficient given that we're planning to introduce labels for test selection, which would support a suite: "olm" label, for example?

@dmesser thoughts?

To expand on this, I feel like v1alpha2 types are probably more complex than is really necessary.

At the end of the day, what I think we want to output is:

A list of test results

The logs from the overall run, if failure or verbose or JSON (which I think is similar to how go test behaves).

I think for each test result we would output:

Name

State (pass/fail/error)

Description, if verbose or JSON

Labels, if verbose or JSON (I know we're not to the label work yet, but it might be good to go ahead and add this to the type)

Errors, if any

Suggestions, if any

We would also need to capture the overall outcome of the scorecard run. Is it overall failed vs. pass etc. Otherwise agree with @joelanford

I was going to hold off on doing any label work in this PR since its pretty large already, I've added the overall outcome to the v1alpha2 api based on this comment.

@jmccormick2001 Since we're making v1alpha2 the default as of this PR, I think we should try to get it's type definition right, so we don't have to go back and change it when we get to the rest of the labels features. I'd suggest doing three things in this PR:

Add a labels field to v1alpha2.ScorecardTestResult as a placeholder (we won't populate them yet, however)

Remove the suite layer from v1alpha2 entirely

Make sure the JSON output format spec contains everything we'll need to finish the epic. If we aren't confident we can do this piece yet, I would hesitate to make v1alpha2 the default.

@dmesser What do you think is sufficient to capture the overall output of the scorecard run? Do you think the log and a simple pass/fail/error state field? Anything else?

Basically, would we be alright with a top-level output struct that contains?

type ScorecardOutput struct { Results []ScorecardTestResult State string // pass, fail, or error Log string } type ScorecardTestResult struct { Name string State string Description string Labels map[string]string Errors []error Suggestions []string }

WDYT?

sounds good, I was going to add the labels field in the upcoming label story work but could do that in this PR if you want. I think the notion of the 'suite' was not clear in my mind how it would be implemented or if it totally removed as a concept, but rather it just becomes another label in the map that is populated.

doc/sdk-cli-reference.md

… into v1alpha2-api

…d by go vet

jmccormick2001 · 2019-10-03T20:59:50Z

/test e2e-aws-subcommand

jmccormick2001 · 2019-10-04T13:02:17Z

/test e2e-aws-subcommand

jmccormick2001 · 2019-10-04T14:57:58Z

/test e2e-aws-ansible
/test e2e-aws-helm
/test e2e-aws-subcommand

jmccormick2001 · 2019-10-04T17:06:03Z

/test e2e-aws-helm
/test e2e-aws-ansible
/test e2e-aws-subcommand

jmccormick2001 · 2019-10-07T13:22:04Z

/test e2e-aws-ansible
/test e2e-aws-helm
/test e2e-aws-subcommand

jmccormick2001 · 2019-10-07T14:36:58Z

/test e2e-aws-subcommand

internal/pkg/scorecard/helpers/versions.go

internal/pkg/scorecard/scorecard_output.go

pkg/apis/scorecard/v1alpha1/formatter.go

pkg/apis/scorecard/v1alpha2/boilerplate.txt

joelanford

A few more nits :)

joelanford · 2019-10-08T16:12:10Z

CHANGELOG.md

@@ -51,6 +52,7 @@
 - [`pkg/test.FrameworkClient`](https://github.com/operator-framework/operator-sdk/blob/master/pkg/test/client.go#L33) methods `List()` and `Delete()` have new signatures corresponding to the homonymous methods of `sigs.k8s.io/controller-runtime/pkg/client.Client`. ([#1876](https://github.com/operator-framework/operator-sdk/pull/1876))
 - CRD file names were previously of the form `<group>_<version>_<kind>_crd.yaml`. Now that CRD manifest `spec.version` is deprecated in favor of `spec.versions`, i.e. multiple versions can be specified in one CRD, CRD file names have the form `<full group>_<resource>_crd.yaml`. `<full group>` is the full group name of your CRD while `<group>` is the last subdomain of `<full group>`, ex. `foo.bar.com` vs `foo`. `<resource>` is the plural lower-case CRD Kind found at `spec.names.plural`. ([#1876](https://github.com/operator-framework/operator-sdk/pull/1876))
 - Upgrade Python version from `2.7` to `3.6`, Ansible version from `2.8.0` to `~=2.8` and ansible-runner from `1.2` to `1.3.4` in the Ansible based images. ([#1947](https://github.com/operator-framework/operator-sdk/pull/1947)) 
+- Made the default scorecard version v1alpha2 which is new for this release and could break users that were parsing the older scorecard output (v1alpha1).  Users can still specify version v1alpha1 on the scorecard configuration to use the older style for some period of time till v1alpha1 is removed.


Suggested change

- Made the default scorecard version v1alpha2 which is new for this release and could break users that were parsing the older scorecard output (v1alpha1). Users can still specify version v1alpha1 on the scorecard configuration to use the older style for some period of time till v1alpha1 is removed.

- Made the default scorecard version `v1alpha2` which is new for this release and could break users that were parsing the older scorecard output (`v1alpha1`). Users can still specify version `v1alpha1` on the scorecard configuration to use the older style for some period of time until `v1alpha1` is removed.

updated with your suggested change.

joelanford · 2019-10-08T16:14:41Z

hack/tests/subcommand-scorecard.sh

+failCount=`echo $commandoutput | grep -o ": fail" | wc -l`
+if [ $failCount -ne 7 ]
+then
+	echo "expected fail count not equal to output"


Nit: define $expectedFailCount and improve the failure text a bit?

Suggested change

echo "expected fail count not equal to output"

echo "expected fail count $expectedFailCount, got $failCount"

agreed, made the change.

joelanford · 2019-10-08T16:17:54Z

internal/pkg/scorecard/helpers/versions.go

-func IsLatestVersion() bool {
-	if viper.Sub("scorecard").GetString(VersionOpt) == LatestScorecardVersion {
+func IsV1alpha2(version string) bool {
+	if version == v1alpha2 {


Nit: This could be simplified to return version == v1alpha2

agreed, made the change.

joelanford · 2019-10-08T16:28:36Z

internal/pkg/scorecard/scorecard_output.go

+	}
+
+	// produce text output
+	if scViper.GetString(OutputFormatOpt) == TextOutputFormat {


Nit: This is out of scope for this PR, but I'm not a huge fan of the global variables used throughout scorecard (e.g. scViper) because they cause non-obvious side effects if you're looking at the call site of the functions that use the global variables.

In a follow-on, I think it would be worth it to refactor things so that viper is used only in the cmd/operator-sdk/scorecard package to parse the config from flags, env, config files, etc. and then populate a scorecard struct with the resulting config, so that we don't have to constantly interrogate a global viper instance.

agreed, I could write up a new Jira story for refactoring that.

https://jira.coreos.com/browse/OSDK-586

https://jira.coreos.com/browse/OSDK-587
another one that falls into the refactoring camp that would be an improvement.

jmccormick2001 · 2019-10-09T14:46:43Z

@joelanford @dmesser based on the comments, I'll be removing the suite layer from the v1alpha2 API and including that set of changes into this PR.

… label

…nto v1alpha2-api

jmccormick2001 · 2019-10-09T18:56:13Z

/retest

joelanford

Hopefully last round of changes. :)

In a follow-up, I think we'll want to re-work the scorecard config file format a bit to handle:

Always requiring a bundle directory and pulling required manifests from it.
Probably removing the duplication between the basic and olm configs since they're already basically identical.
Fail if external plugins are defined for v1alpha2 since the longer term plan is to support Job-based tests provided in the bundle.

Ideally this follow-up would happen before we release v0.12.0 so we don't have a release that has the partial v1alpha2 changes.

joelanford · 2019-10-15T17:56:46Z

pkg/apis/scorecard/v1alpha2/types.go

+	// RequiredTestsPassed occurs when all required tests pass
+	RequiredTestsPassedState State = "All required tests passed."
+	// RequiredTestsFailed occurs when one of the required tests fails
+	RequiredTestsFailedState State = "A required test has failed."


Now that we're not planning any special handling for required tests (right?), I think we can remove these and the code that uses them.

https://jira.coreos.com/browse/OSDK-607

added that story to capture reworking the config file format for v1alpha2

joelanford · 2019-10-15T18:00:44Z

pkg/apis/scorecard/v1alpha2/formatter.go

+	requiredTestStatus := fmt.Sprintf(passColor, RequiredTestsPassedState)
+
+	if s.FailedRequiredTests > 0 {
+		requiredTestStatus = fmt.Sprintf(failColor, RequiredTestsFailedState)
+	}


I think we remove this as well? Since we plan to have the exit code reflect the state of pass/fail for the entire run, I'm not sure we still need a message at the end to that effect.

joelanford · 2019-10-15T18:19:18Z

pkg/apis/scorecard/v1alpha2/formatter.go

+	var currentSuite string
+	for _, result := range s.Results {
+		if currentSuite != result.Labels["suite"] {
+			sb.WriteString(fmt.Sprintf("%s:\n", result.Labels["suite"]))
+			currentSuite = result.Labels["suite"]
+		}
+		sb.WriteString(fmt.Sprintf("\t%-35s: ", result.Name))
+
+		if result.State == PassState {
+			sb.WriteString(fmt.Sprintf(passColor, PassState))
+		} else {
+			sb.WriteString(fmt.Sprintf(failColor, FailState))
+		}
+	}
+
+	sb.WriteString(fmt.Sprintf(requiredTestStatus))
+
+	for _, result := range s.Results {
+		for _, suggestion := range result.Suggestions {
+			// 33 is yellow (specifically, the same shade of yellow that logrus uses for warnings)
+			sb.WriteString(fmt.Sprintf("\x1b[%dmSUGGESTION:\x1b[0m %s\n", 33, suggestion))
+		}
+	}
+
+	for _, result := range s.Results {
+		for _, err := range result.Errors {
+			// 31 is red (specifically, the same shade of red that logrus uses for errors)
+			sb.WriteString(fmt.Sprintf("\x1b[%dmERROR:\x1b[0m %s\n", 31, err))
+		}
+	}


WDYT about going ahead and implementing the non-verbose text output I suggested here.

do you mind if I do that in a separate story?

Yeah that should be fine. Ideally we'll have the output formats done before our next release so we don't have to change them. I think it's probably okay to change the text format since it's meant for humans.

JSON is the one we definitely want to get right before the next release since that's meant to be machine-readable and changes might break pipelines. I think JSON is good to go, but I'll give it one more run with these latest changes to confirm.

pkg/apis/scorecard/v1alpha2/types.go

jmccormick2001 · 2019-10-15T21:51:45Z

/test e2e-aws-ansible

jmccormick2001 · 2019-10-16T13:53:28Z

/test e2e-aws-ansible

jmccormick2001 · 2019-10-16T15:00:42Z

/test e2e-aws-ansible

camilamacedo86 · 2019-10-16T15:11:59Z

CHANGELOG.md

@@ -20,6 +20,7 @@
 - Ansible based operators now gather and serve metrics about each custom resource on port 8686 of the metrics service. ([#1723](https://github.com/operator-framework/operator-sdk/pull/1723))
 - Added the Go version, OS, and architecture to the output of `operator-sdk version` ([#1863](https://github.com/operator-framework/operator-sdk/pull/1863))
 - Added support for `ppc64le-linux` for the `operator-sdk` binary and the Helm operator base image. ([#1533](https://github.com/operator-framework/operator-sdk/pull/1533))
+- Added new `--version` flag to the `operator-sdk scorecard` command to support a new output format for the scorecard. ([#1916](https://github.com/operator-framework/operator-sdk/pull/1916)


I checked that k8s uses the nomenclature --experimental-x for this kind of impl. WDYT about we adopted the same? As for example --experimental-version

I think that's meant more for enabling or configuring experimental features. In this case, this will control core features of the scorecard (output formats, support for external plugins, exit code handling, label selection, etc.).

So I think we should leave --version as the flag name.

joelanford

LGTM

In the follow-on when we're working on labels, I'd suggest changing the suite labels from "Basic Tests" and "OLM Tests" to "basic" and "olm"

jmccormick2001 added 8 commits September 26, 2019 15:53

initial set of v1alpha2 api files and output formatting

17841d3

add simple v1alpha2 scorecard test that can be run on demand

50feda0

add v1alpha2 version docs updates

dd0bf33

update CHANGELOG to mention new version flag added

6c30748

adjust json output for v1alpha2 rendering to show failed required tests

6c4731f

refactor a bit of basic_tests

3e5ab12

tweak output for text v1alpha2

b391538

remove erroneous v1alpha2 reference in v1alpha1 test

477b53f

openshift-ci-robot requested review from estroz and theishshah September 30, 2019 19:11

openshift-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Sep 30, 2019

jmccormick2001 requested a review from joelanford September 30, 2019 19:11

joelanford reviewed Sep 30, 2019

View reviewed changes

jmccormick2001 added 12 commits October 1, 2019 16:29

initial set of v1alpha2 api files and output formatting

8e420e1

add simple v1alpha2 scorecard test that can be run on demand

f6cd0f7

add v1alpha2 version docs updates

3a8ae98

update CHANGELOG to mention new version flag added

146fa90

adjust json output for v1alpha2 rendering to show failed required tests

54b8f50

refactor a bit of basic_tests

68ac6d3

tweak output for text v1alpha2

8f20945

remove erroneous v1alpha2 reference in v1alpha1 test

fba804e

add code changes per PR review comments

178e17e

after rebase

5c0d199

fix merge

456d337

Merge branch 'master' into v1alpha2-api

394ab82

joelanford reviewed Oct 2, 2019

View reviewed changes

doc/sdk-cli-reference.md Show resolved Hide resolved

jmccormick2001 added 3 commits October 2, 2019 15:27

refactor api output formatting implementation to use an interface

3ba185a

Merge branch 'v1alpha2-api' of github.com:jmccormick2001/operator-sdk…

ee143ad

… into v1alpha2-api

change interface function name for json output to avoid conflict foun…

cf74e91

…d by go vet

jmccormick2001 added 2 commits October 3, 2019 10:13

make v1alpha2 the default scorecard version

6767e66

refactor

b61eca7

Merge branch 'master' into v1alpha2-api

5740698

joelanford reviewed Oct 7, 2019

View reviewed changes

jmccormick2001 added 2 commits October 7, 2019 15:04

updates

2830cc2

fix ci scorecard test

b961f88

joelanford reviewed Oct 8, 2019

View reviewed changes

minor tweaks per review comments

83b27d2

camilamacedo86 requested review from joelanford and dmesser October 8, 2019 17:56

add labels field to v1alpha2 ScorecardTestResult type

f09d64d

jmccormick2001 added 2 commits October 9, 2019 11:50

remove Suite from the v1alpha2 API, adjust text output to print suite…

7ce3a9d

… label

Merge branch 'master' of github.com:operator-framework/operator-sdk i…

611375f

…nto v1alpha2-api

joelanford reviewed Oct 15, 2019

View reviewed changes

jmccormick2001 added 2 commits October 15, 2019 14:24

Merge remote-tracking branch 'upstream/master' into v1alpha2-api

d31e808

removed required test status, add omitempty to v1alpha2 types

9903139

camilamacedo86 reviewed Oct 16, 2019

View reviewed changes

joelanford approved these changes Oct 16, 2019

View reviewed changes

jmccormick2001 merged commit b964dd6 into operator-framework:master Oct 16, 2019

	- Made the default scorecard version v1alpha2 which is new for this release and could break users that were parsing the older scorecard output (v1alpha1). Users can still specify version v1alpha1 on the scorecard configuration to use the older style for some period of time till v1alpha1 is removed.
	- Made the default scorecard version `v1alpha2` which is new for this release and could break users that were parsing the older scorecard output (`v1alpha1`). Users can still specify version `v1alpha1` on the scorecard configuration to use the older style for some period of time until `v1alpha1` is removed.

	echo "expected fail count not equal to output"
	echo "expected fail count $expectedFailCount, got $failCount"

V1alpha2 api - scorecard output changes for v1alpha2 version #1995

V1alpha2 api - scorecard output changes for v1alpha2 version #1995

Uh oh!

Conversation

jmccormick2001 commented Sep 30, 2019

Uh oh!

jmccormick2001 commented Sep 30, 2019

Uh oh!

joelanford left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmesser Oct 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jmccormick2001 commented Oct 3, 2019

Uh oh!

jmccormick2001 commented Oct 4, 2019

Uh oh!

jmccormick2001 commented Oct 4, 2019

Uh oh!

jmccormick2001 commented Oct 4, 2019

Uh oh!

jmccormick2001 commented Oct 7, 2019

Uh oh!

jmccormick2001 commented Oct 7, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

joelanford left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmesser Oct 7, 2019 •

edited

Loading

jmccormick2001 Oct 8, 2019 •

edited

Loading