Allow the --plugin flag to specify a directory of plugins #911

johnSchnake · 2019-09-27T15:05:53Z

What this PR does / why we need it:
If someone specifies sonobuoy run|gen --plugin X then X ought to
be allowed to be either a single plugin file or a directory of plugins.

To avoid dropping errors intentionally or processing lots of unnecessary
files, when processing a directory, we only consider *.yaml files. We
also do not recurse deeper into the directory.

Which issue(s) this PR fixes
Fixes #906

Special notes for your reviewer:
This entire diff is almost just test updates. I can split it out so that it is more clear if you want.

I originally wasn't going to have tests for this since there are no tests for flags->GenConfig generation. However I decided that it should be covered and the test was almost doing that anyways.

However, I found a bug when making the test so I had to resolve that too (a one word change). Made comments throughout to help clarify.

Release note:

The --plugin flag (for specifying custom plugins on the command line) can now accept a directory name. If a directory is specified, all *.yaml files will be processed as plugins and loaded as if they had been specified individually.

johnSchnake · 2019-09-27T15:07:37Z

cmd/sonobuoy/app/gen.go

@@ -104,7 +104,7 @@ func (g *genFlags) Config() (*client.GenConfig, error) {

 	// the Enabled and Disabled modes of rbacmode don't need the client, so kubeclient can be nil.
 	// if kubeclient is needed, ErrRBACNoClient will be returned and that error can be reported back up.
-	rbacEnabled, err := genflags.rbacMode.Enabled(kubeclient)
+	rbacEnabled, err := g.rbacMode.Enabled(kubeclient)


Tiny, mean bug that I found while coding these changes. It is referencing a global variable instead of the value it it was invoked on. This is why it happens to work for actual runs of Sonobuoy (since the global value gets used/set) but not in the test.

Very frustrating to find and another reason why we shouldn't be relying on them. One day we'll get around to fixing all the globals for these cmds...one day...

cmd/sonobuoy/app/gen_test.go

johnSchnake · 2019-09-27T15:08:24Z

cmd/sonobuoy/app/gen_test.go

@@ -85,19 +91,31 @@ func TestResolveConformanceImage(t *testing.T) {
 func TestResolveConfig(t *testing.T) {
 	defaultPluginSearchPath := config.New().PluginSearchPath
 	defaultAggr := plugin.AggregationConfig{TimeoutSeconds: 10800}
+	dynamicConfigFileName := "*determinedAtRuntime*"


I use this to hot-swap out the name since I write the config to a temp file but want the cmdline input to be specified explicitly.

johnSchnake · 2019-09-27T15:09:24Z

cmd/sonobuoy/app/gen_test.go

 	}{
 		{
-			name: "NonDisruptiveConformance mode when supplied config is nil (nothing interesting happens)",
-			input: &genFlags{


The test used to be a bit confusing since it could be given a genFlags object or it could be given cmdline input.

Now the test is a bit more consistent by always parsing cmdline input and relying on actual config files.

johnSchnake · 2019-09-27T15:10:27Z

cmd/sonobuoy/app/gen_test.go

+			},
+			expectedGenConfigPlugins: &client.GenConfig{
+				StaticPlugins: []*manifest.Manifest{
+					{SonobuoyConfig: manifest.SonobuoyConfig{Driver: "Job", PluginName: "plugin1", SkipCleanup: false, ResultFormat: "raw"}, Spec: manifest.Container{Container: v1.Container{Name: "plugin", Image: "foo/bar:v1", Command: []string{"./run.sh"}, VolumeMounts: []v1.VolumeMount{{ReadOnly: false, Name: "results", MountPath: plugin.ResultsDir}}}}},


These static plugin lines are long but shouldn't really change. I just had to grab one and then a bit of copy/paste. Details arent too important here, the point is that the right set of plugins got loaded (really just need to look at name/image)

johnSchnake · 2019-09-27T15:11:48Z

cmd/sonobuoy/app/gen_test.go

 			}

-			conf := tc.input.resolveConfig()


The old test was using resolveConfig which generates the config.Config object. However, this isn't sufficient to see if the GenConfig would be appropriate so we were missing the ability to test large sections of input processing.

johnSchnake · 2019-09-27T15:12:25Z

cmd/sonobuoy/app/gen_test.go

 			}

-			if conf.WorkerImage != tc.expected.WorkerImage {
-				t.Errorf("Expected worker image %v but got %v", tc.expected.WorkerImage, conf.WorkerImage)
+			if genConfig.Config.Aggregation.TimeoutSeconds != tc.expected.Aggregation.TimeoutSeconds {


Now that we are generating the 'higher level' genConfig object, these checks just needed tweaked to look at the same value.

johnSchnake · 2019-09-27T15:13:59Z

cmd/sonobuoy/app/gen_test.go

 			}

-			if !reflect.DeepEqual(conf.PluginSelections, tc.expected.PluginSelections) {
-				t.Errorf("expected PluginSelections %v but got %v", tc.expected.PluginSelections, conf.PluginSelections)
+			if diff := pretty.Compare(genConfig.StaticPlugins, tc.expectedGenConfigPlugins.StaticPlugins); diff != "" {


Added 2 checks here just for the static/dynamic plugins. Consistent with the existing test which is just checking for key bits of data.

If we wanted to change these tests to check the entire genconfig we should probably just serialize the values and use golden files since the objects are so big.

johnSchnake · 2019-09-27T15:14:47Z

cmd/sonobuoy/app/pluginList.go

@@ -71,19 +74,53 @@ func (p *pluginList) Set(str string) error {
 	case pluginSystemdLogs:
 		p.DynamicPlugins = append(p.DynamicPlugins, str)
 	default:
-		b, err := ioutil.ReadFile(str)
+		finfo, err := os.Stat(str)


Since I have to stat to check if it is a directory or not, I had to have a new possible error message and so some tests needed updated for this reason as well. Tiny change but impacts a handful of tests.

johnSchnake · 2019-09-27T15:15:33Z

cmd/sonobuoy/app/pluginList.go

+}
+
+// loadSinglePlugin loads a single plugin located at the given path.
+func (p *pluginList) loadSinglePlugin(filepath string) error {


This entire section is just moving code around. The existing load plugin logic was moved here and then the load dir of plugins just gets a list of files and leverages this same method.

johnSchnake · 2019-09-27T15:20:01Z

cmd/sonobuoy/app/gen_test.go

-			cliInput: "--sonobuoy-image=flagImage --config testdata/sonobuoy.conf",
+			name:               "Flags shouldn't override the config settings unless set",
+			input:              "--sonobuoy-image=flagImage --config " + dynamicConfigFileName,
+			configFileContents: `{"Namespace":"configNS","WorkerImage":"configImage","ImagePullPolicy":"Never","Server":{"timeoutseconds":500}}`,


This sonobuoy.conf was used just for a few of these tests and since I already added the logic to make/use a temp one I just deleted the file and copied the contents to a few tests.

Much more flexible and now it is clear what the conf file is for each test instead of having to go look it up.

johnSchnake · 2019-09-27T15:58:51Z

Test failing in CI but passing locally due to me having my KUBECONFIG set and pointed to a real cluster. Will update the tests.

codecov-io · 2019-09-27T16:16:01Z

Codecov Report

❗ No coverage uploaded for pull request base (master@d9ca4ba). Click here to learn what that means.
The diff coverage is 75%.

@@            Coverage Diff            @@
##             master     #911   +/-   ##
=========================================
  Coverage          ?   47.42%           
=========================================
  Files             ?       76           
  Lines             ?     5155           
  Branches          ?        0           
=========================================
  Hits              ?     2445           
  Misses            ?     2560           
  Partials          ?      150

Impacted Files	Coverage Δ
cmd/sonobuoy/app/gen.go	`73.72% <100%> (ø)`
cmd/sonobuoy/app/pluginList.go	`80.85% <73.91%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d9ca4ba...82f1d41. Read the comment docs.

zubron

Thanks @johnSchnake! All the extra comments were really helpful 👍

zubron · 2019-09-27T18:37:34Z

cmd/sonobuoy/app/gen_test.go

-			name:     "Flags shouldn't override the config settings unless set",
-			input:    &genFlags{},
-			cliInput: "--config testdata/sonobuoy.conf",
+			name:               "Flags shouldn't override the config settings unless set",


It was already like this, but this is a duplicate test name.

Its not clear to me what one of those is supposed to test; the name indicates resolving conflicts with the flag/config but doesnt actually give any flags other than the config. I'll have it check against some other field so it does something reasonable before merging (and update the name)

If someone specifies `sonobuoy run|gen --plugin X` then X ought to be allowed to be either a single plugin file or a directory of plugins. To avoid dropping errors intentionally or processing lots of unnecessary files, when processing a directory, we only consider *.yaml files. We also do not recurse deeper into the directory. Fixes #906 Signed-off-by: John Schnake <[email protected]>

johnSchnake requested a review from zubron September 27, 2019 15:05

johnSchnake commented Sep 27, 2019

View reviewed changes

cmd/sonobuoy/app/gen_test.go Outdated Show resolved Hide resolved

johnSchnake commented Sep 27, 2019

View reviewed changes

zubron approved these changes Sep 27, 2019

View reviewed changes

johnSchnake merged commit 06f638e into vmware-tanzu:master Sep 27, 2019

johnSchnake deleted the pluginsDir branch September 27, 2019 19:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow the --plugin flag to specify a directory of plugins #911

Allow the --plugin flag to specify a directory of plugins #911

johnSchnake commented Sep 27, 2019

johnSchnake Sep 27, 2019

johnSchnake Sep 27, 2019

johnSchnake Sep 27, 2019

johnSchnake Sep 27, 2019

johnSchnake Sep 27, 2019

johnSchnake Sep 27, 2019

johnSchnake Sep 27, 2019

johnSchnake Sep 27, 2019

johnSchnake Sep 27, 2019

johnSchnake Sep 27, 2019

johnSchnake commented Sep 27, 2019

codecov-io commented Sep 27, 2019 •

edited

Loading

zubron left a comment

zubron Sep 27, 2019

johnSchnake Sep 27, 2019

Allow the --plugin flag to specify a directory of plugins #911

Allow the --plugin flag to specify a directory of plugins #911

Conversation

johnSchnake commented Sep 27, 2019

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

johnSchnake commented Sep 27, 2019

codecov-io commented Sep 27, 2019 • edited Loading

Codecov Report

zubron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Sep 27, 2019 •

edited

Loading