scorecard - Add list flag #2137

jmccormick2001 · 2019-10-30T21:07:08Z

Description of the change:

this change adds a new 'list' flag to the scorecard CLI set of flags, this flag will
cause only the test names to be printed out and not actually run the scorecard tests.

The tests that 'would' be run are determined by the selector filter being applied and gives
the end user a chance to see which tests would be run given a particular selector.

Motivation for the change:

This feature is part of a larger effort to build the v1alpha2 version of the scorecard.

joelanford · 2019-10-30T21:20:32Z

internal/scorecard/scorecard.go

+	if scViper.GetBool(ListOpt) {
+		fmt.Printf("\nTests that would be executed include:\n")
+	}
+


I tried this out in ./test/test-framework and got:

$ cd ./test/test-framework $ operator-sdk scorecard -L Tests that would be executed include: Error: error validating plugin config: revert to v1alpha1 to use external plugins: external plugins are not currently supported with v1alpha2

We should print this after we've validated the config, or maybe we don't need it at all?

I removed that in this next update.

joelanford · 2019-10-30T21:35:00Z

internal/scorecard/plugins/plugin_runner.go

-			if err != nil {
-				basicTests.Log = fmt.Sprintf("failed to read log buffer: %v", err)
+			if config.ListOpt {
+				basicTests.List()


Rather than using a new List() function on TestSuite, I was kind of thinking we would build the pluginOutputs without actually running the tests, and then just let printPluginOutputs() do its thing. Then the output formating would be the same regardless of run vs. list, and we'd be able to list in JSON or text.

var pluginOutputs []scapiv1alpha1.ScorecardOutput for _, plugin := range plugins { if scViper.GetBool(ListOpt) { pluginOutputs = append(pluginOutputs, plugin.List()) } else { pluginOutputs = append(pluginOutputs, plugin.Run()) } }

I think we also need to print the labels in the output somewhere so users know how to build their label selectors.

Maybe something like the verbose text format I mocked up here?

Another alternative I like is a column-based format that looks like kubectl output, something like this, using a tab writer:

TEST NAME LABELS STATUS Spec Block Exists "necessity"="required", "suite"="basic" pass Status Block Exists "necessity"="required", "suite"="basic" pass Writing into CRs has an effect "necessity"="required", "suite"="basic" fail

We'd leave off STATUS when using --list. But when actually running the tests, if we did choose something like this, we'd have to figure out how to output the suggestions, errors, and logs, and make it obvious which test and which CR they're for. Maybe initially, we'd just append them at the end like we do now?

in this version the text output with the --list flag looks like this:

basic:
Spec Block Exists
Labels:
necessity:required
suite:basic

Status Block Exists Labels: necessity:required suite:basic Writing into CRs has an effect Labels: necessity:required suite:basic

olm:
Provided APIs have validation
Labels:
necessity:required
suite:olm

Owned CRDs have resources listed Labels: necessity:required suite:olm CRs have at least 1 example Labels: necessity:required suite:olm Spec fields with descriptors Labels: necessity:required suite:olm Status fields with descriptors Labels: necessity:required suite:olm

this is the output when --list is not specified:

basic:
Spec Block Exists : pass
Labels:
necessity:required
suite:basic

Status Block Exists : pass Labels: necessity:required suite:basic Writing into CRs has an effect : pass Labels: suite:basic necessity:required

olm:
Spec fields with descriptors : pass
Labels:
necessity:required
suite:olm

Status fields with descriptors : fail Labels: suite:olm necessity:required Suggestions: Add a status descriptor for status Provided APIs have validation : pass Labels: necessity:required suite:olm Owned CRDs have resources listed : pass Labels: necessity:required suite:olm Suggestions: If it would be helpful to an end-user to understand or troubleshoot your CR, consider adding resources [configmaps/v1 memcacheds/v1alpha1 replicasets/v1 deployments/v1 services/v1] to the resources section for owned CRD Memcached CRs have at least 1 example : pass Labels: necessity:required suite:olm

internal/scorecard/helpers/test_definitions.go

internal/scorecard/plugins/plugin_runner.go

pkg/apis/scorecard/v1alpha2/types.go

joelanford · 2019-11-01T03:00:27Z

pkg/apis/scorecard/v1alpha2/formatter.go

-		for _, suggestion := range result.Suggestions {
+		sb.WriteString("\tLabels: \n")
+		for labelKey, labelValue := range result.Labels {
+			sb.WriteString(fmt.Sprintf("\t\t%s:%s\n", labelKey, labelValue))


Should we put labels and values in quotes in case they have spaces in them? I think for our own labels this should never be a problem, but if/when we add support back in for external plugins with v1alpha2, we'll have less control.

Suggested change

sb.WriteString(fmt.Sprintf("\t\t%s:%s\n", labelKey, labelValue))

sb.WriteString(fmt.Sprintf("\t\t%q: %q\n", labelKey, labelValue))

I'll add this, it renders pretty good I think.

pkg/apis/scorecard/v1alpha2/formatter.go

jmccormick2001 · 2019-11-01T20:02:42Z

/test e2e-aws-subcommand

jmccormick2001 · 2019-11-01T21:10:30Z

/test marker

joelanford

LGTM

camilamacedo86

/lgtm
/approved

.

camilamacedo86 · 2019-11-01T22:11:39Z

hack/tests/subcommand-scorecard.sh

@@ -20,7 +20,7 @@ operator-sdk scorecard --version v1alpha2 --config "$CONFIG_PATH" |& grep '^.*er

 # test to see if v1alpha2 is used from the command line
 commandoutput="$(operator-sdk scorecard --version v1alpha2 --config "$CONFIG_PATH_V1ALPHA2" 2>&1)"
-failCount=`echo $commandoutput | grep -o ": fail" | wc -l`
+failCount=`echo $commandoutput | grep -o "fail" | wc -l`
 expectedFailCount=3


Hi @jmccormick2001,

I could not find any test here with --selector -l (passing the selector) and checking that just the tests with the label X was executed as I could not find the test with the --list / -l as well. Should we not add it here?

Besides this, all shows OK for me /LGTM

camilamacedo86

I am approving but please, check my comment above.
I think it is missing the tests and we should add it before merging.

camilamacedo86 · 2019-11-01T23:17:39Z

hack/tests/subcommand-scorecard.sh

@@ -28,6 +28,26 @@ then
 	exit 1
 fi

+# test to see if list flag work


joelanford

/lgtm

jmccormick2001 added 2 commits October 30, 2019 15:25

add initial list flag logic to scorecard

b3812d9

fix the list flag - v1alpha1 check

21142ea

jmccormick2001 added the scorecard Issue relates to the scorecard subcomponent label Oct 30, 2019

jmccormick2001 self-assigned this Oct 30, 2019

openshift-ci-robot requested review from estroz and joelanford October 30, 2019 21:07

openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 30, 2019

joelanford reviewed Oct 30, 2019

View reviewed changes

camilamacedo86 reviewed Oct 31, 2019

View reviewed changes

internal/scorecard/helpers/test_definitions.go Outdated Show resolved Hide resolved

refactor text output and list flag logic

f044446

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 31, 2019

camilamacedo86 reviewed Oct 31, 2019

View reviewed changes

internal/scorecard/plugins/plugin_runner.go Outdated Show resolved Hide resolved

camilamacedo86 reviewed Oct 31, 2019

View reviewed changes

internal/scorecard/plugins/plugin_runner.go Show resolved Hide resolved

joelanford reviewed Nov 1, 2019

View reviewed changes

jmccormick2001 added 2 commits November 1, 2019 10:42

clean up code, add quotes around labels

afa717d

slight refactor of applying selectors when listing

7313800

joelanford previously approved these changes Nov 1, 2019

View reviewed changes

camilamacedo86 previously approved these changes Nov 1, 2019

View reviewed changes

camilamacedo86 reviewed Nov 1, 2019

View reviewed changes

camilamacedo86 previously approved these changes Nov 1, 2019

View reviewed changes

add test for label and selector flags for scorecard

2c5604f

jmccormick2001 dismissed stale reviews from camilamacedo86 and joelanford via 2c5604f November 1, 2019 22:41

camilamacedo86 reviewed Nov 1, 2019

View reviewed changes

hack/tests/subcommand-scorecard.sh

@@ -28,6 +28,26 @@ then

exit 1

fi

# test to see if list flag work

Copy link

Contributor

camilamacedo86 Nov 1, 2019

Choose a reason for hiding this comment

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

🥇 Great

change test selector to use suite=basic

d1e510f

joelanford approved these changes Nov 4, 2019

View reviewed changes

openshift-ci-robot assigned joelanford Nov 4, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 4, 2019

jmccormick2001 merged commit 289ee29 into operator-framework:master Nov 4, 2019

	sb.WriteString(fmt.Sprintf("\t\t%s:%s\n", labelKey, labelValue))
	sb.WriteString(fmt.Sprintf("\t\t%q: %q\n", labelKey, labelValue))

scorecard - Add list flag #2137

scorecard - Add list flag #2137

Uh oh!

Conversation

jmccormick2001 commented Oct 30, 2019

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jmccormick2001 commented Nov 1, 2019

Uh oh!

jmccormick2001 commented Nov 1, 2019

Uh oh!

joelanford left a comment

Choose a reason for hiding this comment

Uh oh!

camilamacedo86 left a comment

Choose a reason for hiding this comment

Uh oh!

camilamacedo86 Nov 1, 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!

camilamacedo86 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joelanford left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

camilamacedo86 Nov 1, 2019 •

edited

Loading