internal/pkg/scorecard: implement plugin system #1379

AlexNPavel · 2019-05-02T22:24:51Z

Description of the change: Implement scorecard plugins

Motivation for the change: This PR implements support for scorecard plugins as described in this proposal: https://github.com/operator-framework/operator-sdk/blob/master/doc/proposals/scorecard-plugin-system.md

The Basic and OLM test suites will be split out into separate plugins after this PR is merged.

shawn-hurley

LGTM, I would suggest completing the helper function method in the TODO

internal/pkg/scorecard/scorecard.go

internal/pkg/scorecard/helpers.go

internal/pkg/scorecard/scorecard.go

joelanford · 2019-05-08T20:58:53Z

internal/pkg/scorecard/scorecard.go

+		}
+		for _, file := range files {
+			cmd := exec.Command("bash", "-c", "./bin/"+file.Name())
+			output, err := cmd.CombinedOutput()


In this case, it might make sense to capture stdout and stderr separately. We'd parse the stdout as JSON and maybe include the stderr in a log?

Where exactly would we put the log? We provide a per-suite log field for plugins to use so they shouldn't be outputting anything to stdout or stderr except the JSON or potentially fatal errors. I assume we could add it into the main scorecard log (in the final ScorecardOutput), but that could get messy pretty quickly

Yeah that's a good question. Wdyt of checking if result.Log is empty. If so, set result.Log to the stderr buffer. If not, log a warning that would be seen in the human-readable output?

That sounds reasonable. I'll do that.

Actually, there's a problem with that. A single plugin can have more than 1 suite. If that's the case, we won't be able to simply add the stderr to a log field.

The way I'll handle this for now is using log.Warn for the stderr output, which will print it nicely in the human-readable output or add it to the main log for the json output.

Co-Authored-By: AlexNPavel <[email protected]>

camilamacedo86 · 2019-05-08T22:28:51Z

internal/pkg/scorecard/scorecard.go

-	for _, suite := range suites {
-		for idx, res := range suite.TestResults {
-			suite.TestResults[idx] = UpdateState(res)
+	for _, suite := range pluginOutputs {


A for inside another shows not be the best solution. Note the Big O here. Could it not cause performance issues? Could it not be improved to achieve a better performance?

camilamacedo86 · 2019-05-08T22:29:37Z

internal/pkg/scorecard/scorecard.go

+		numSuites := 0
+		for _, plugin := range pluginOutputs {
+			for _, suite := range plugin.Results {
+				fmt.Printf("%s:\n", suite.Name)


The same point of attention in the impl. The complexity of this impl.

camilamacedo86 · 2019-05-08T22:34:09Z

internal/pkg/scorecard/scorecard.go

+			for _, suite := range plugin.Results {
+				for _, result := range suite.Tests {
+					for _, suggestion := range result.Suggestions {
+						// 33 is yellow (specifically, the same shade of yellow that logrus uses for warnings)


A loop the inside of another loop 4 times. Is not possible to solve this need in a more performative way?

camilamacedo86 · 2019-05-08T22:34:28Z

internal/pkg/scorecard/scorecard.go

-				for _, err := range result.Errors {
-					// 31 is red (specifically, the same shade of red that logrus uses for errors)
-					fmt.Printf("\x1b[%dmERROR:\x1b[0m %s\n", 31, err)
+		for _, plugin := range pluginOutputs {


A loop the inside of another loop 4 times. Is not possible to solve this need in a more performative way?

AlexNPavel · 2019-05-08T23:08:07Z

@camilamacedo86 BigO for these loops is irrelevant to the runtime of the scorecard. Much of the time spent in the scorecard will be on the actual plugins themselves (which includes the Basic and OLM tests), which will likely be waiting on networking. The plugins should be optimized if they are compute heavy. However, the time spent in the loops you mentioned will be on the order of milliseconds (excluding the print syscall time, which we would still have) even for runs with a lot of output and plugins. Those extra couple of milliseconds spent in the loops are irrelevant to the overall runtime of the scorecard and any optimizations there would sacrifice clarity.

camilamacedo86 · 2019-05-08T23:27:50Z

@AlexNPavel

IMHO shows that it could be changed/improved. Now it shows irrelevant, however, in it could grow and the issues started to be faced. The CI tests take too long already. Also, could not the code be more understandable and in this way easier to keep maintained and be improved without these loops inside of loops?

Note that the following exactly the same loop inside of a loop is made many times.

for _, plugin := range pluginOutputs {
			for _, suite := range plugin.Results {

You make a great work here, it is just a suggestion. Please, feel free to move forward as you decide

shawn-hurley · 2019-05-09T16:48:49Z

You make a great work here, it is just a suggestion. Please, feel free to move forward as you decide

I think that if we want to make this better with fewer loops we would need to redesign the structs.

I think that something we could do in this PR or as a follow up to make this better is:

Have the Plugin struct responsible for its output.
Allow this function to take a channel of type string. Allow the function to do the loops and create its output and then return it to the channel.
For each plugin calls each of these and then takes the return strings and create the output.

If that is too much, at the very least lets combine the two different for loops, by creating the output in two string buffers and then combining.

for _, suite := range plugin.Results {
				for _, result := range suite.Tests {
					for _, err := range result.Errors {
						// 31 is red (specifically, the same shade of red that logrus uses for errors)
						errorBuffer.Add(fmt.Sprintf("\x1b[%dmERROR:\x1b[0m %s\n", 31, err))
					}
					for _, suggestion := range result.Suggestions {
						// 33 is yellow (specifically, the same shade of yellow that logrus uses for warnings)
						suggestionBuffer.Addfmt.Sprintf("\x1b[%dmSUGGESTION:\x1b[0m %s\n", 33, suggestion))
					}

				}
}

/cc @camilamacedo86 @AlexNPavel

BTW I personally feel this is acceptable to do in a follow-up.

openshift-ci-robot · 2019-05-09T16:48:53Z

@shawn-hurley: GitHub didn't allow me to request PR reviews from the following users: camilamacedo86, AlexNPavel.

Note that only operator-framework members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

You make a great work here, it is just a suggestion. Please, feel free to move forward as you decide

I think that if we want to make this better with fewer loops we would need to redesign the structs.

I think that something we could do in this PR or as a follow up to make this better is:

Have the Plugin struct responsible for its output.

Allow this function to take a channel of type string. Allow the function to do the loops and create its output and then return it to the channel.

For each plugin calls each of these and then takes the return strings and create the output.

If that is too much, at the very least lets combine the two different for loops, by creating the output in two string buffers and then combining.
for _, suite := range plugin.Results {
  			for _, result := range suite.Tests {
  				for _, err := range result.Errors {
  					// 31 is red (specifically, the same shade of red that logrus uses for errors)
  					errorBuffer.Add(fmt.Sprintf("\x1b[%dmERROR:\x1b[0m %s\n", 31, err))
  				}
  				for _, suggestion := range result.Suggestions {
  					// 33 is yellow (specifically, the same shade of yellow that logrus uses for warnings)
  					suggestionBuffer.Addfmt.Sprintf("\x1b[%dmSUGGESTION:\x1b[0m %s\n", 33, suggestion))
  				}

  			}
}
/cc @camilamacedo86 @AlexNPavel

BTW I personally feel this is acceptable to do in a follow-up.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

joelanford

👍 on @shawn-hurley's comment about further improvements. I also think we could address them in a follow-on PR.

A few more nits and comments.

internal/pkg/scorecard/scorecard.go

joelanford · 2019-05-09T18:06:28Z

internal/pkg/scorecard/helpers.go

@@ -147,7 +135,7 @@ func TestResultToScorecardTestResult(tr TestResult) scapiv1alpha1.ScorecardTestR
 }

 // UpdateState updates the state of a TestResult.
-func UpdateState(res TestResult) TestResult {
+func UpdateState(res scapiv1alpha1.ScorecardTestResult) scapiv1alpha1.ScorecardTestResult {


We have a handful of exported helper functions for the scapiv1alpha1 types in this package. I'd suggest moving them into the scapiv1alpha1 package, possibly making them methods on the scapiv1alpha1 structs (where that makes sense), and also possibly unexporting them if we think they'll only be used on the SDK side of the plugin interface.

This could be in a follow-on PR. Since we're going to reorganize our built-in scorecard tests as plugins, I'd be interested in your thoughts on a design for a plugin library that would make it easy to write a plugin in go.

Yeah, I'll make a PR to expose some of these. I was initially intending to expose some of this with the JSON PR, but it wasn't quite ready. Now that we have the plugin support in place, we can probably expose the helpers.

joelanford

LGTM

hasbro17 · 2019-05-13T21:51:15Z

internal/pkg/scorecard/helpers.go

+			suite.PartialPass++
+		case scapiv1alpha1.FailState:
+			suite.Fail++
+		}


We should probably have a default case to catch and log any unknown state.
Even though it doesn't seem like we'll end up in an unknown state, it's still good practice in case we change it in the future.

I'll do this in a future PR. The issue is that there is no way to really handle this cleanly right now. One way to fix this would be to add another field to the SuiteResults of invalid. That way we can indicate to users that a plugin returned an invalid state. Another option (which is what I want to do in a future PR) is to change the state from whatever incorrect state it was in to error and add a string to the errors array indicating that the scorecard marked this test as failed due to an invalid state.

Technically though, due to the way the helpers are set up (we calculate the state using the earned vs max points unless the state is error), the only situation where this could occur (currently) would be if the earned points are higher than the max. I'd be fine marking that as an error.

internal/pkg/scorecard/scorecard.go

hasbro17 · 2019-05-13T22:53:41Z

Looks good overall. Just some small nits.

Co-Authored-By: Haseeb Tariq <[email protected]>

hasbro17

One last nit: We should update the CLI reference guide for the flags. But you can leave that out unless you have a separate PR for doc changes.

AlexNPavel · 2019-05-15T21:13:31Z

@hasbro17 I am going to make a PR with all the doc changes today

AlexNPavel added kind/feature Categorizes issue or PR as related to a new feature. scorecard Issue relates to the scorecard subcomponent labels May 2, 2019

AlexNPavel requested review from joelanford, shawn-hurley, hasbro17 and estroz May 2, 2019 22:24

openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 2, 2019

internal/pkg/scorecard: implement plugin system

014b2d6

AlexNPavel force-pushed the sc-plugin-support branch from bea1688 to 014b2d6 Compare May 2, 2019 22:46

shawn-hurley approved these changes May 7, 2019

View reviewed changes

estroz reviewed May 8, 2019

View reviewed changes

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

joelanford requested changes May 8, 2019

View reviewed changes

AlexNPavel and others added 6 commits May 8, 2019 14:23

Merge branch 'master' into sc-plugin-support

31c1e8d

Update internal/pkg/scorecard/helpers.go

f5e7618

Co-Authored-By: AlexNPavel <[email protected]>

internal/pkg/scorecard: create failedPlugin function

74ac861

internal/pkg/scorecard: check cleanup errors

666712e

internal/pkg/scorecard: don't use bash in exec

fad2178

internal/pkg/scorecard: improve failed plugin message

c6bdefb

camilamacedo86 reviewed May 8, 2019

View reviewed changes

joelanford reviewed May 9, 2019

View reviewed changes

AlexNPavel added 3 commits May 9, 2019 13:35

internal/pkg/scorecard: improve readability

26ae520

internal/pkg/scorecard: improve failedPlugin func

e847a10

internal/pkg/scorecard: collect stdout and stderr of plugins

b64ec17

internal/pkg/scorecard: add if statement

b4ca702

joelanford approved these changes May 13, 2019

View reviewed changes

hasbro17 reviewed May 13, 2019

View reviewed changes

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

hasbro17 reviewed May 13, 2019

View reviewed changes

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

hasbro17 reviewed May 13, 2019

View reviewed changes

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

hasbro17 reviewed May 13, 2019

View reviewed changes

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

hasbro17 reviewed May 13, 2019

View reviewed changes

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

hasbro17 reviewed May 13, 2019

View reviewed changes

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

hasbro17 reviewed May 13, 2019

View reviewed changes

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

AlexNPavel and others added 4 commits May 13, 2019 16:26

test/test-framework/scorecard: fix group name

03e6166

scorecard: suggestions from hasbro17

dbe25ea

Apply suggestions from code review

d1c66b8

Co-Authored-By: Haseeb Tariq <[email protected]>

scorecard: more suggestions from hasbro17

aff7728

hasbro17 approved these changes May 15, 2019

View reviewed changes

AlexNPavel merged commit aa1918a into operator-framework:master May 15, 2019

AlexNPavel deleted the sc-plugin-support branch May 15, 2019 21:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

internal/pkg/scorecard: implement plugin system #1379

internal/pkg/scorecard: implement plugin system #1379

AlexNPavel commented May 2, 2019

shawn-hurley left a comment

joelanford May 8, 2019

AlexNPavel May 8, 2019

joelanford May 9, 2019

AlexNPavel May 9, 2019

AlexNPavel May 9, 2019

AlexNPavel May 9, 2019

camilamacedo86 May 8, 2019

camilamacedo86 May 8, 2019

camilamacedo86 May 8, 2019

camilamacedo86 May 8, 2019

AlexNPavel commented May 8, 2019

camilamacedo86 commented May 8, 2019 •

edited

Loading

shawn-hurley commented May 9, 2019

openshift-ci-robot commented May 9, 2019

joelanford left a comment

joelanford May 9, 2019

AlexNPavel May 9, 2019

joelanford left a comment

hasbro17 May 13, 2019

AlexNPavel May 15, 2019

hasbro17 commented May 13, 2019

hasbro17 left a comment

AlexNPavel commented May 15, 2019

internal/pkg/scorecard: implement plugin system #1379

internal/pkg/scorecard: implement plugin system #1379

Conversation

AlexNPavel commented May 2, 2019

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

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

AlexNPavel commented May 8, 2019

camilamacedo86 commented May 8, 2019 • edited Loading

shawn-hurley commented May 9, 2019

openshift-ci-robot commented May 9, 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

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

hasbro17 commented May 13, 2019

hasbro17 left a comment

Choose a reason for hiding this comment

AlexNPavel commented May 15, 2019

camilamacedo86 commented May 8, 2019 •

edited

Loading