commands/.../scorecard: add support for JSON output #1228

AlexNPavel · 2019-03-19T21:40:53Z

Description of the change: Add JSON output support based on proposal from PR #1049.

Motivation for the change: Closes issue #1189.

This PR will be considered WIP until PR #1049 is merged.

AlexNPavel · 2019-03-19T21:43:14Z

Note: Since this is a draft PR, Travis-CI will not run until I mark it ready for review

AlexNPavel · 2019-04-01T22:30:52Z

NOTE: I will be basing this PR on top of #1272, so this will be blocked until that PR is merged

We can use viper directly now, so we can ignore the return values for cobra flags

AlexNPavel · 2019-04-22T18:45:06Z

/cc @joelanford @shawn-hurley @hasbro17 @estroz

joelanford

What do you think about abstracting the output formatter as an interface or function definition such that we do the setup of the output formatter implementation near the top of the function and then just use the abstraction without needing the conditionals when we're doing the actual outputting?

If we did something like that, I think it would help separate the result output from the non-result output and make it more obvious what the output implementation actually requires (it looks like the suites and the log?).

joelanford · 2019-04-22T19:21:04Z

cmd/operator-sdk/scorecard/cmd.go

@@ -51,6 +51,7 @@ func NewCmd() *cobra.Command {
 	scorecardCmd.Flags().String(scorecard.ProxyImageOpt, fmt.Sprintf("quay.io/operator-framework/scorecard-proxy:%s", strings.TrimSuffix(version.Version, "+git")), "Image name for scorecard proxy")
 	scorecardCmd.Flags().String(scorecard.ProxyPullPolicyOpt, "Always", "Pull policy for scorecard proxy image")
 	scorecardCmd.Flags().String(scorecard.CRDsDirOpt, scaffold.CRDsDir, "Directory containing CRDs (all CRD manifest filenames must have the suffix 'crd.yaml')")
+	scorecardCmd.Flags().String(scorecard.OutputFormatOpt, "human-readable", "Output format for results. Valid values: human-readable, json")


My vote would be -o, --output, which would align with kubectl's usage.

joelanford · 2019-04-22T19:55:38Z

internal/pkg/scorecard/scorecard.go

+	origStdout := os.Stdout
+	readLog, writeLog, _ := os.Pipe()
+	deferLogPrint := true
+	// suppress all output except the resulting json


I'm having trouble following what gets written to os.Stdout and os.Stderr when they're being redirected to writeLog. Is it just the log.Errorf lines (227, 244, and 258)?

Rather than the complex logic to temporarily redirect stdout and stderr, I'm wondering if it's possible for us to explicitly control where non-results output goes with something like:

buf := &bytes.Buffer{} ... log.SetOutput(buf) ... fmt.Fprintf(buf, "something I want to print that isn't part of the results output")

And then when we're printing results, include the contents of buf in the output based on the output format.

I've changed the way the logging is handled for this. We instead configure a global logrus var called log and configure it as you suggested (using SetOutput) and all logging is handled through log. Only the final printing of the results goes through fmt.Printf, so we don't need all the workarounds I previously had.

joelanford · 2019-04-22T20:18:51Z

pkg/apis/scorecard/v1alpha1/register.go

+
+var (
+	// SchemeGroupVersion is group version used to register these objects
+	SchemeGroupVersion = schema.GroupVersion{Group: "scorecard", Version: "v1alpha1"}


Should the scorecard group include a domain to make it more unique? I don't know if we have a convention for SDK APIs (if not, we probably should). As a point of reference, OLM currently uses operators.coreos.com.

Yeah, it would be a good idea to include a domain. I'm not quite sure what to use though. Since we are creating this now, I feel like it would be better to go with an openshift URL (like the metering-operator, which uses metering.openshift.io) instead of a coreos URL.

Oh yeah, I agree with using openshift.io. I'd be good with any of these:

operator-sdk.openshift.io

operatorsdk.openshift.io

osdk.openshift.io

And the kinds are ScorecardOutput and ScorecardOutputList, so since "Scorecard" is in the kind name, I figure we don't need "scorecard" in the group?

@AlexNPavel Ping on this question about the domain.

I'll use osdk.openshift.io for the domain

internal/pkg/scorecard/scorecard.go

This commit configures a global logrus log variable that can be used by the entire package and is configured based on the output format. All logging should happen via the `log` var and fmt.Printf should only be used for the final output.

joelanford · 2019-04-23T15:54:33Z

internal/pkg/scorecard/scorecard.go

+		}
+	}
+	if viper.GetString(OutputFormatOpt) == "json" {
+		log, err := ioutil.ReadAll(logReadWriter)


With this, ioutil.ReadAll() will run before the deferred log statements, so I don't think we'll see any of them in the output.

The simplest way to fix this would be to move lines 108-291 into a separate function so that the deferred log statements run before we output the results.

Long term, this could potentially be improved further by using a scorecardRunner struct to decouple the actual scorecard run from the cobra CLI stuff to make it more reusable.

joelanford

Looks just about ready to me. Just the one remaining question about the API domain.

Not for this PR, but I am curious if you had thoughts on my question in this comment. It looks like the complication would be that for human-readable we intermingle the log and the results and for json we collect the log and include it in the results.

joelanford · 2019-04-26T17:59:37Z

pkg/apis/scorecard/v1alpha1/register.go

+
+var (
+	// SchemeGroupVersion is group version used to register these objects
+	SchemeGroupVersion = schema.GroupVersion{Group: "scorecard", Version: "v1alpha1"}


@AlexNPavel Ping on this question about the domain.

AlexNPavel · 2019-04-26T18:58:55Z

@joelanford We could collect the logs in a buffer during the human-readable output as well. For instance:

Logs:
---
...
---

Basic Test:
...

Now that the scorecard run function is separated from the output, all the logs appear before the final results anyway. I feel that keeping the streaming logs might be useful for the human-readble output though.

joelanford

Agreed that streaming output in human-readable mode is valuable. We can put that thought on the backburner for now.

LGTM

estroz

A few small things, otherwise LGTM

internal/pkg/scorecard/scorecard.go

pkg/apis/scorecard/v1alpha1/types.go

internal/pkg/scorecard/helpers.go

Co-Authored-By: AlexNPavel <alexpavel123@gmail.com>

shawn-hurley

I think we need to be more careful with potential nil pointer exceptions

hack/tests/scorecard-subcommand.sh

shawn-hurley · 2019-05-01T03:36:45Z

internal/pkg/scorecard/helpers.go

@@ -35,3 +40,85 @@ func ResultsCumulative(results []TestResult) (earned, max int) {
 	}
 	return earned, max
 }
+
+// CalculateResult returns a ScorecardSuiteResult with the state and Tests fields set based on a slice of ScorecardTestResults
+func CalculateResult(tests []*scapiv1alpha1.ScorecardTestResult) *scapiv1alpha1.ScorecardSuiteResult {


Why are we choosing to pass in pointers and then not nil checking?

I think that the correct thing to do is not take pointers in the slice.

I also would like to understand why a pointer back? this just means callers will have to check for nil.

shawn-hurley · 2019-05-01T03:40:50Z

internal/pkg/scorecard/helpers.go

+}
+
+// UpdateState updates the state of a TestResult.
+func (res *TestResult) UpdateState() {


I think this should be close to the TestResult struct IMO. having it here would be confusing and hard to find IMO.

shawn-hurley · 2019-05-01T03:48:58Z

internal/pkg/scorecard/scorecard.go

+		fmt.Printf("\nTotal Score: %.0f%%\n", totalScore)
+		// Print suggestions
+		for _, suite := range suites {
+			for _, result := range suite.TestResults {


So is the place I am worried about. Technically suite could have a slice in TestResults like:

[]{nil, nil, nil}

We should really make sure that this is not the case so we don't nil panic here. I would think instead if you don't have the structs of the slice as pointers (which I can't see a reason to have them be a pointer at the moment) then this would not be a concern.

AlexNPavel · 2019-05-01T18:25:15Z

Fixed

/cc @shawn-hurley

…rk#1228)

AlexNPavel added the scorecard Issue relates to the scorecard subcomponent label Mar 19, 2019

AlexNPavel requested review from joelanford, shawn-hurley, lilic, hasbro17, theishshah and estroz March 19, 2019 21:40

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 19, 2019

openshift-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 1, 2019

cmd/.../scorecard: move scorecard libraries out of cmd

4c1dd1c

AlexNPavel force-pushed the json-scorecard branch from e915fd0 to d48f70f Compare April 1, 2019 22:29

openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 1, 2019

AlexNPavel added 4 commits April 3, 2019 10:00

pkg/scorecard: move pkg/scorecard/lib to pkg/scorecard

689f021

cmd/.../scorecard: don't store cobra vars

c466da6

We can use viper directly now, so we can ignore the return values for cobra flags

(internal)/pkg/scorecard: update package and import names

025c020

(internal)/pkg/scorecard: add JSON output support

5503e0c

AlexNPavel force-pushed the json-scorecard branch from 3be48f4 to 5503e0c Compare April 3, 2019 18:01

AlexNPavel added 2 commits April 3, 2019 13:46

Merge branch 'master' into json-scorecard

6d13687

Merge branch 'master' into json-scorecard

d3697d1

openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Apr 8, 2019

pkg/apis: move some libraries around

3d098db

AlexNPavel force-pushed the json-scorecard branch from 0a1eda8 to 3d098db Compare April 8, 2019 23:54

pkg/apis/scorecard: add name and desc to suites

30c5db2

openshift-ci-robot requested a review from shawn-hurley April 22, 2019 18:45

joelanford reviewed Apr 22, 2019

View reviewed changes

AlexNPavel force-pushed the json-scorecard branch from 7f8bd9d to aaa4ea3 Compare April 22, 2019 22:27

AlexNPavel force-pushed the json-scorecard branch from aaa4ea3 to 83d48bc Compare April 22, 2019 22:28

cmd/.../scorecard: add -o shorthand for output flag

8b216c5

joelanford reviewed Apr 23, 2019

View reviewed changes

AlexNPavel added 2 commits April 25, 2019 11:40

internal/pkg/scorecard: capture deferred scorecard logs

2a7c881

Merge branch 'master' into json-scorecard

dad187e

joelanford reviewed Apr 26, 2019

View reviewed changes

pkg/apis/scorecard: change group name

e5a5531

joelanford approved these changes Apr 26, 2019

View reviewed changes

estroz approved these changes Apr 26, 2019

View reviewed changes

internal/pkg/scorecard/scorecard.go Outdated Show resolved Hide resolved

pkg/apis/scorecard/v1alpha1/types.go Outdated Show resolved Hide resolved

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

AlexNPavel and others added 3 commits April 26, 2019 15:05

internal/pkg/scorecard: fix APIVersion

671ca96

Update pkg/apis/scorecard/v1alpha1/types.go

f239f7d

Co-Authored-By: AlexNPavel <alexpavel123@gmail.com>

Update internal/pkg/scorecard/scorecard.go

017de9f

Co-Authored-By: AlexNPavel <alexpavel123@gmail.com>

shawn-hurley requested changes May 1, 2019

View reviewed changes

scorecard: reduce execessive use of pointers

d1a184a

AlexNPavel force-pushed the json-scorecard branch from 904a39a to d1a184a Compare May 1, 2019 18:13

openshift-ci-robot requested a review from shawn-hurley May 1, 2019 18:25

shawn-hurley approved these changes May 1, 2019

View reviewed changes

joelanford mentioned this pull request May 1, 2019

commands/.../scorecard: add multi-cr support #1236

Merged

AlexNPavel merged commit 052bd65 into operator-framework:master May 1, 2019

AlexNPavel deleted the json-scorecard branch May 1, 2019 20:05

estroz pushed a commit to estroz/operator-sdk that referenced this pull request May 2, 2019

commands/.../scorecard: add support for JSON output (operator-framewo…

8121298

…rk#1228)

AlexNPavel mentioned this pull request Jun 6, 2019

Support JSON output for scorecard #1189

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

commands/.../scorecard: add support for JSON output #1228

commands/.../scorecard: add support for JSON output #1228

AlexNPavel commented Mar 19, 2019

AlexNPavel commented Mar 19, 2019

AlexNPavel commented Apr 1, 2019

AlexNPavel commented Apr 22, 2019

joelanford left a comment

joelanford Apr 22, 2019

joelanford Apr 22, 2019

AlexNPavel Apr 22, 2019

joelanford Apr 22, 2019

AlexNPavel Apr 22, 2019

joelanford Apr 23, 2019

joelanford Apr 26, 2019

AlexNPavel Apr 26, 2019

joelanford Apr 23, 2019

joelanford left a comment

joelanford Apr 26, 2019

AlexNPavel commented Apr 26, 2019

joelanford left a comment

estroz left a comment

shawn-hurley left a comment

shawn-hurley May 1, 2019

shawn-hurley May 1, 2019

shawn-hurley May 1, 2019

AlexNPavel commented May 1, 2019

commands/.../scorecard: add support for JSON output #1228

commands/.../scorecard: add support for JSON output #1228

Conversation

AlexNPavel commented Mar 19, 2019

AlexNPavel commented Mar 19, 2019

AlexNPavel commented Apr 1, 2019

AlexNPavel commented Apr 22, 2019

joelanford left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joelanford left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexNPavel commented Apr 26, 2019

joelanford left a comment

Choose a reason for hiding this comment

estroz left a comment

Choose a reason for hiding this comment

shawn-hurley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexNPavel commented May 1, 2019