internal/pkg/scorecard: implement new scorecard config system #1641

AlexNPavel · 2019-07-03T21:14:37Z

Description of the change: Implement new file based configuration system for the operator-sdk scorecard as detailed in the proposal made via PR #1558. It is a large PR, but a large majority of the new/changed lines are from 3 new scorecard subcommand tests and the related JSON assets and configs for the tests, as well as replacing all usages of viper in the internal plugins to a config struct built by unmarshalling the user provided configs. Documentation will be added in a separate PR that should be ready soon

Motivation for the change: Implement PR #1558

Based on ./doc/proposals/improved-scorecard-config.md

AlexNPavel · 2019-07-05T23:54:56Z

Documentation PR is now open: #1648

internal/pkg/scorecard/interfaces.go

Co-Authored-By: Ish Shah <ish@ishshah.me>

theishshah

LGTM, let's wait for another set of eyes before merging

estroz

Looks good on a first pass. A few questions and suggestions.

internal/pkg/scorecard/scorecard.go

estroz · 2019-07-16T20:55:38Z

internal/pkg/scorecard/scorecard.go

-		if err != nil {
-			return err
-		}
-		log.Warn("Could not load config file; using flags")


Now users always need a config file? Can they not run only their plugins with global CLI args? If a config is always required, mark --config as required.

I'm not quite sure. It might make sense to make a slight behavior change here. Currently, if you don't provide a config file, the scorecard will attempt to run all plugins. It will fail for the olm and basic plugins (as it did before due to missing cr-manifest; I intentionally kept that behavior for this PR), but for external plugins that don't need extra configuration it should work.

It might make sense to run all external plugins without configuration and ignore the internal plugins if a config file is not provided, but I feel like that might kinda hide the internal plugins from the users a bit, which is not ideal. This might need further discussion.

IMO we should run only external plugins if no configuration file is provided, but log an info statement before doing so. Something similar to:

No internal plugin configuration detected. Skipping Basic and OLM Plugins and running all external plugins.

@joelanford @hasbro17 @lilic @shawn-hurley @theishshah WDYT about handling this case?

Yeah, I think that skipping them and logging an info statement would be a better behavior

So it seems like the options are:

Skip internal plugins and log a message that they were skipped

Fail/do nothing without a config file. Don't even run external plugins.

If we don't find a config file, initialize one and attempt to use sane defaults, even for the internal plugins. Would this be possible using CR manifests from the deploy/crds directory? Perhaps we need full support for multiple CRs first?

I kind of prefer option 2 to force the user to be explicit about what to run. Or option 3 if we think the defaulting is possible/sane.

AFAIK option 3 could work only if we assume all CR's in deploy/crds that also have accompanying CRD's should be tested, which is a bad idea. I agree we need to be explicit about what is being run.

Users should still be able to run internal plugins without a config, as they can do now, which is why I'm against option 2 and in favor of 1.

AFAIK option 3 could work only if we assume all CR's in deploy/crds that also have accompanying CRD's should be tested, which is a bad idea. I agree we need to be explicit about what is being run.

That's a good point. I know we don't do this anywhere else, but I know kubebuilder does - we could use interactive prompts in that case to have the user answer yes/no questions about including a CR in the config file. I'm not personally sold on that, but just throwing it out there.

Users should still be able to run internal plugins without a config, as they can do now, which is why I'm against option 2 and in favor of 1.

With option 1, aren't we saying that a config file has to be present to run internal plugins? Otherwise, they'll be skipped, a message will be logged, and the results will not contain anything about them?

For interactive use cases, I think option 1 is fine. But for non-interactive use cases, it seems like that message will get buried and results that don't include the basic and OLM tests may be misleading. I guess it depends on how the thing interpreting the JSON results is implemented.

Yeah I'm backtracking on my opinion of option 1. To align our UX I'm now thinking we should go with option 2, and scaffold a scorecard config when running operator-sdk new.

internal/pkg/scorecard/interfaces.go

joelanford · 2019-07-31T18:59:11Z

internal/pkg/scorecard/interfaces.go

-		description := fmt.Sprintf("Plugin with file name `%s` failed", filepath.Base(p.binPath))
+	if err := os.Chdir(filepath.Join(rootDir, scViper.GetString(PluginDirOpt))); err != nil {
+		name := fmt.Sprintf("Failed Plugin: %s", p.name)
+		description := fmt.Sprintf("Plugin with file name `%s` failed", filepath.Base(p.config.Command))


Probably out of scope for this PR since we're not changing it here, but it seems odd to me that we're changing the name and description of the test based on the test outcome.

It seems like those should always stay the same, since there are other indicators of the outcome (e.g. "error": 1 and "log": "failed to chdir into scorecard plugin directory...")

This is a new problem since we define a name for the plugins with this config design. However, the plugin also returns a name field, and in a successful run, we return the name that the plugin gave us, not the name from the config. This is a bit confusing, because this means we only ever use the config provided name during a failure, where we specify the name. I'm not quite sure how we should handle the name specified in the config file, or if we should even have one at all now...

Hmm that's an interesting question. These end up being the suite names right? So going through various options:

Plugin provides name in output, config name field is removed

Plugin fails to execute or does not return name in output => use basename of plugin executable

Plugin returns name in output => use name provided by plugin

Plugin provides name, but can be overridden by config

Plugin fails to execute or does not return name in output => use config name if present, fallback to basename of plugin executable

Plugin returns name in output => use config name if present, fallback to name provided by plugin

Plugin name is always basename of plugin executable

Plugin name is always provided by config

None of these are perfect IMO.

Option 1 gives control to the plugin author. Plugin authors could do something weird, but generally I'd imagine this would be static or based on flags/input. This property is nice because shared plugins will generally result in the same name when run against different projects, which makes it easier to compare apples-to-apples.

Option 2 is probably my least favorite option, since its the \_(ツ)_/ of the group.

Option 3 and 4 are nice because duplicates can be found without executing the plugins, and they're very straightforward. Not sure if/how we handle duplicate names? Con: the same plugin might be renamed or configured with a different name, making comparison across projects a no-go.

I think I lean towards option 1. Thoughts? Any other options I'm not thinking of?

A plugin could return multiple suites. This would mean that there would be multiple names (each suite has a name), so changing the name(s) of the suite using the scorecard config can get messy.

Option 1 is good, but there is the issue that people may want to run the same plugin with different arguments or environment. If we only print the command's basename, a user may have difficulty identifying which test failed. By also providing the name that a user provided in their config, it makes it a lot easier to identify which plugin/configuration failed.

We could go with option 1 and append an md5 digest of all arguments/environment data to the plugin name, so you can run the same plugin with different configurations.

A plugin could return multiple suites. This would mean that there would be multiple names (each suite has a name), so changing the name(s) of the suite using the scorecard config can get messy.

Maybe having a plugin return multiple suites is worth revisiting in scorecard 2.0. In the current output, is it possible to know which suites came from which plugin? If plugins ran only one suite, the plugin name would be the suite name, which seems like a decent way to handle plugins.

Either way, that isn't the case now. So in the meantime, it sounds like we're in agreement on not allowing user-overrideable test names?

Option 1 is good, but there is the issue that people may want to run the same plugin with different arguments or environment. If we only print the command's basename, a user may have difficulty identifying which test failed. By also providing the name that a user provided in their config, it makes it a lot easier to identify which plugin/configuration failed.

True, but it sounds like the only time the basename would be used is when the plugin fails. In that case, can we populate the log output with the full path, args, env of the failed plugin, along with whatever the stdout/err was.

internal/pkg/scorecard/interfaces.go

internal/pkg/scorecard/plugins/resource_handler.go

joelanford · 2019-07-31T20:42:28Z

internal/pkg/scorecard/interfaces.go

 }

-type internalPlugin struct {
+type basicOrOLMPlugin struct {


Also probably out of scope for this PR since the API is what matters most, but did you consider defining separate basicPlugin and olmPlugin structs such that each could have its own Run() implementation?

It might be cleaner to extract the shared code from basicOrOLMPlugin.Run() and plugins.RunInternalPlugin into separate functions or packages so that we wouldn't have to support both plugin types with one function.

Yeah, we discussed separating the Run functions in another PR. They are very similar and would only differ by about 5-10 lines out of a very large function. They also have all the same configuration fields (and they both use all of them except for csv-path in the basic plugins). Splitting up the Run function into multiple smaller functions would be good to make it clearer as to what's actually going on in the function and is something we definitely should do, but we still would essentially be running the same functions for both plugins.

This isn't to say that no future internal plugins would be separate. For the proposed YAML-defined scorecard tests, we would have a completely separate Run: #1241

Oh right. Sorry I'm harping on it 🙂

They also have all the same configuration fields (and they both use all of them except for csv-path in the basic plugins)

For this we could use struct embedding where the common fields go in a basicPlugin struct and an olmPlugin struct embeds basicPlugin and adds csv-path.

Splitting up the Run function into multiple smaller functions would be good to make it clearer as to what's actually going on in the function and is something we definitely should do

Yeah that's pretty much what I'm suggesting.

but we still would essentially be running the same functions for both plugins.

Which is totally fine. I'm basically saying that code that runs based on a pluginType conditional could be extracted into the plugin's run function, and all other code could be shared in smaller split-out functions.

And again, probably not in scope for this PR, but something like this would be good to do in a follow-up depending on direction from CVP requirements.

internal/pkg/scorecard/scorecard.go

AlexNPavel · 2019-08-13T17:35:23Z

/test e2e-aws-subcommand

AlexNPavel · 2019-08-13T17:36:17Z

/test e2e-aws-go
/test e2e-aws-helm

joelanford

Looks pretty close. Hopefully just the one last question.

internal/pkg/scorecard/scorecard.go

estroz

Needs a changelog entry, otherwise LGTM.

Also there should be a follow-up PR that scaffolds a scorecard config on operator-sdk new as per #1641 (comment).

joelanford

Agree with @estroz. LGTM after CHANGELOG update.

AlexNPavel · 2019-08-21T18:08:33Z

Updated CHANGELOG. Will work on updating the docs PR now (#1648)

AlexNPavel · 2019-08-21T21:31:43Z

/test e2e-aws-ansible

AlexNPavel · 2019-08-22T14:32:32Z

/test e2e-aws-ansible

AlexNPavel · 2019-08-23T17:28:51Z

/retest

AlexNPavel · 2019-08-26T16:43:46Z

/test e2e-aws-ansible

AlexNPavel · 2019-08-27T17:09:13Z

/retest

internal/pkg/scorecard: implement new scorecard config system

200356d

Based on ./doc/proposals/improved-scorecard-config.md

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

AlexNPavel requested review from estroz, hasbro17, joelanford, shawn-hurley and theishshah July 3, 2019 21:14

openshift-ci-robot requested a review from lilic July 3, 2019 21:14

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

AlexNPavel added 5 commits July 3, 2019 15:54

internal/pkg/scorecard: update text output format opt

1048ad4

Merge branch 'master' into scorecard-config

0ccec8c

hack/tests/scorecard: correct tests

aa6b36a

internal/pkg/scorecard: fix kubeconfig handling for internal plugins

fbc835c

internal/pkg/scorecard,hack/tests: fail on invalid config

db2737f

AlexNPavel mentioned this pull request Jul 5, 2019

doc/test-framework/scorecard: document config file #1648

Merged

AlexNPavel added 2 commits July 10, 2019 14:34

Merge branch 'master' into scorecard-config

1950fdc

go.mod: run make tidy

f0b04e5

theishshah reviewed Jul 15, 2019

View reviewed changes

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

Update internal/pkg/scorecard/interfaces.go

7285de5

Co-Authored-By: Ish Shah <ish@ishshah.me>

theishshah approved these changes Jul 15, 2019

View reviewed changes

internal/pkg/scorecard: update a comment

a78b618

estroz suggested changes Jul 16, 2019

View reviewed changes

AlexNPavel added 2 commits July 16, 2019 14:29

scorecard: suggestions from code review

6f2ca99

internal/pkg/scorecard: change behavior

c089232

AlexNPavel force-pushed the scorecard-config branch from 695c7d9 to c089232 Compare July 29, 2019 21:07

joelanford reviewed Jul 31, 2019

View reviewed changes

AlexNPavel added 2 commits July 31, 2019 15:25

change waitUntilCRStatusExists to use time.Duration

8130531

simplify an if statement

40b5a63

openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Aug 12, 2019

AlexNPavel added 2 commits August 12, 2019 14:39

ci/tests: fix tests

c073829

internal/pkg/scorecard: new output format for failed plugins

39f08b7

AlexNPavel force-pushed the scorecard-config branch from d1c494d to 39f08b7 Compare August 12, 2019 21:53

AlexNPavel added 3 commits August 12, 2019 14:55

internal/pkg/scorecard: fix some Errorf statements

09bd12e

Merge branch 'master' into scorecard-config

4f8b685

ci/tests: move scorecard script back

c801ed1

AlexNPavel closed this Aug 13, 2019

AlexNPavel reopened this Aug 13, 2019

AlexNPavel added 2 commits August 13, 2019 11:55

ci/tests: fix sed substitution

26dc115

test/test-framework: remove unrelated debug file

00d7c7e

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 Aug 13, 2019

joelanford reviewed Aug 19, 2019

View reviewed changes

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

internal/pkg/scorecard: remove "Name" field from config

ba53aca

estroz approved these changes Aug 21, 2019

View reviewed changes

joelanford approved these changes Aug 21, 2019

View reviewed changes

CHANGELOG: update changelog

42a8fa3

Merge branch 'master' into scorecard-config

b0644e2

Merge branch 'master' into scorecard-config

b79c612

AlexNPavel merged commit 2c8f211 into operator-framework:master Aug 28, 2019

AlexNPavel deleted the scorecard-config branch August 28, 2019 00:15

internal/pkg/scorecard: implement new scorecard config system #1641

internal/pkg/scorecard: implement new scorecard config system #1641

Uh oh!

Conversation

AlexNPavel commented Jul 3, 2019

Uh oh!

AlexNPavel commented Jul 5, 2019

Uh oh!

Uh oh!

theishshah left a comment

Choose a reason for hiding this comment

Uh oh!

estroz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

estroz Jul 19, 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!

Uh oh!

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!

Uh oh!

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!

AlexNPavel commented Aug 13, 2019

Uh oh!

AlexNPavel commented Aug 13, 2019

Uh oh!

joelanford left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

estroz left a comment

Choose a reason for hiding this comment

Uh oh!

joelanford left a comment

Choose a reason for hiding this comment

Uh oh!

AlexNPavel commented Aug 21, 2019

Uh oh!

AlexNPavel commented Aug 21, 2019

Uh oh!

AlexNPavel commented Aug 22, 2019

estroz Jul 19, 2019 •

edited

Loading