Remove plugin.ResultType due to confusion/lack of need (#857)

There was redundancy between three different fields of the plugin: name, resultType, and resultFormat. The first two existed prior to the third and were the same in almost every case, leading to confusion about why there were two different values at all. The problem with this is that the aggregator server would be primarily concerned with resultType and use it through its code when referring to plugins. As a result, the plugin name, despite having the more intuitive title, was actually almost never used. This all led to a few confusing situations: - If a plugin name != resultType it was unclear on which query/screen they would see which value - Uniqueness was only enforced on resultType and not plugin name, leading to the confusing requirement that they provide a name but not using it often and it not being required to be unique - The plugin name/type were almost always 100% the same even though their names don't imply that, whereas plugin type/format were never meant to be related despite sounding like similar ideas. By removing the plugin result type (and relying on plugin name for the unique identifier) we are left with two fields (name and format) which seem much more clear and unique. Internally, the aggregator server still uses the "ResultType" naming convention in its own types since those types did not have the 'Name' field to conflict with. In addition, that is not really a user facing field whereas plugin definitions (where type/name both exist) are. Fixes #830 Signed-off-by: John Schnake <[email protected]>
vmware-tanzu · Aug 30, 2019 · 3b14025 · 3b14025
1 parent 2d47854
commit 3b14025
Show file tree

Hide file tree

Showing 34 changed files with 27 additions and 88 deletions.
diff --git a/cmd/sonobuoy/app/gen_plugin_def.go b/cmd/sonobuoy/app/gen_plugin_def.go
@@ -116,7 +116,7 @@ func defaultManifest() manifest.Manifest {
 	m.Spec.Name = defaultPluginName
 	m.SonobuoyConfig.Driver = defaultPluginDriver
 	m.Spec.VolumeMounts = []v1.VolumeMount{
-		v1.VolumeMount{
+		{
 			MountPath: plugin.ResultsDir,
 			Name:      defaultMountName,
 			ReadOnly:  defaultMountReadOnly,
@@ -142,9 +142,6 @@ func genPluginDefWrapper(cfg *GenPluginDefConfig) func(cmd *cobra.Command, args
 // genPluginDef returns the YAML for the plugin which Sonobuoy would expect as
 // a configMap in order to run/gen a typical run.
 func genPluginDef(cfg *GenPluginDefConfig) ([]byte, error) {
-	// Result type just duplicates the name in most cases.
-	cfg.def.SonobuoyConfig.ResultType = cfg.def.SonobuoyConfig.PluginName
-
 	// Copy the validated value to the actual field.
 	cfg.def.SonobuoyConfig.Driver = cfg.driver.String()
 

diff --git a/cmd/sonobuoy/app/testdata/pluginDef-container.golden b/cmd/sonobuoy/app/testdata/pluginDef-container.golden
@@ -1,7 +1,6 @@
 sonobuoy-config:
   driver: ""
   plugin-name: ""
-  result-type: ""
 spec:
   command:
   - /bin/sh

diff --git a/cmd/sonobuoy/app/testdata/pluginDef-default-podspec.golden b/cmd/sonobuoy/app/testdata/pluginDef-default-podspec.golden
@@ -11,7 +11,6 @@ podSpec:
 sonobuoy-config:
   driver: ""
   plugin-name: "n"
-  result-type: "n"
 spec:
   name: ""
   resources: {}
diff --git a/cmd/sonobuoy/app/testdata/pluginDef-env.golden b/cmd/sonobuoy/app/testdata/pluginDef-env.golden
@@ -1,7 +1,6 @@
 sonobuoy-config:
   driver: ""
   plugin-name: ""
-  result-type: ""
 spec:
   env:
   - name: FOO

diff --git a/cmd/sonobuoy/app/testdata/pluginDef-quotes.golden b/cmd/sonobuoy/app/testdata/pluginDef-quotes.golden
@@ -1,7 +1,6 @@
 sonobuoy-config:
   driver: ""
   plugin-name: "n"
-  result-type: "n"
 spec:
   command:
   - /bin/sh

diff --git a/cmd/sonobuoy/app/testdata/pluginDef-sonoconfig.golden b/cmd/sonobuoy/app/testdata/pluginDef-sonoconfig.golden
@@ -1,7 +1,6 @@
 sonobuoy-config:
   driver: Job
   plugin-name: "n"
-  result-type: "n"
 spec:
   name: ""
   resources: {}
diff --git a/pkg/client/gen.go b/pkg/client/gen.go
@@ -251,7 +251,6 @@ func systemdLogsManifest(cfg *GenConfig) *manifest.Manifest {
 		SonobuoyConfig: manifest.SonobuoyConfig{
 			PluginName:   "systemd-logs",
 			Driver:       "DaemonSet",
-			ResultType:   "systemd-logs",
 			ResultFormat: "raw",
 		},
 		Spec: manifest.Container{
@@ -293,7 +292,6 @@ func e2eManifest(cfg *GenConfig) *manifest.Manifest {
 		SonobuoyConfig: manifest.SonobuoyConfig{
 			PluginName:   "e2e",
 			Driver:       "Job",
-			ResultType:   "e2e",
 			ResultFormat: "junit",
 		},
 		Spec: manifest.Container{

diff --git a/pkg/client/results/reader_test.go b/pkg/client/results/reader_test.go
@@ -76,9 +76,9 @@ func MustGetReader(path string, t *testing.T) *results.Reader {
 }
 
 var versions = []*version{
-	&version{0, 8},
-	&version{0, 9},
-	&version{0, 10},
+	{0, 8},
+	{0, 9},
+	{0, 10},
 }
 
 func TestServerGroup(t *testing.T) {

diff --git a/pkg/client/testdata/default-plugins-via-nil-selection.golden b/pkg/client/testdata/default-plugins-via-nil-selection.golden
@@ -31,7 +31,6 @@ data:
       driver: Job
       plugin-name: e2e
       result-format: junit
-      result-type: e2e
     spec:
       command:
       - /run_e2e.sh
@@ -49,7 +48,6 @@ data:
       driver: DaemonSet
       plugin-name: systemd-logs
       result-format: raw
-      result-type: systemd-logs
     spec:
       command:
       - /bin/sh

diff --git a/pkg/client/testdata/default-pod-spec.golden b/pkg/client/testdata/default-pod-spec.golden
@@ -41,7 +41,6 @@ data:
       driver: Job
       plugin-name: e2e
       result-format: junit
-      result-type: e2e
     spec:
       command:
       - /run_e2e.sh
@@ -72,7 +71,6 @@ data:
       driver: DaemonSet
       plugin-name: systemd-logs
       result-format: raw
-      result-type: systemd-logs
     spec:
       command:
       - /bin/sh

diff --git a/pkg/client/testdata/default.golden b/pkg/client/testdata/default.golden
@@ -31,7 +31,6 @@ data:
       driver: Job
       plugin-name: e2e
       result-format: junit
-      result-type: e2e
     spec:
       command:
       - /run_e2e.sh
@@ -49,7 +48,6 @@ data:
       driver: DaemonSet
       plugin-name: systemd-logs
       result-format: raw
-      result-type: systemd-logs
     spec:
       command:
       - /bin/sh

diff --git a/pkg/client/testdata/e2e-default.golden b/pkg/client/testdata/e2e-default.golden
@@ -31,7 +31,6 @@ data:
       driver: Job
       plugin-name: e2e
       result-format: junit
-      result-type: e2e
     spec:
       command:
       - /run_e2e.sh

diff --git a/pkg/client/testdata/envoverrides.golden b/pkg/client/testdata/envoverrides.golden
@@ -31,7 +31,6 @@ data:
       driver: Job
       plugin-name: e2e
       result-format: junit
-      result-type: e2e
     spec:
       command:
       - /run_e2e.sh

diff --git a/pkg/client/testdata/imagePullSecrets.golden b/pkg/client/testdata/imagePullSecrets.golden
@@ -31,7 +31,6 @@ data:
       driver: Job
       plugin-name: e2e
       result-format: junit
-      result-type: e2e
     spec:
       command:
       - /run_e2e.sh

diff --git a/pkg/client/testdata/manual-custom-plugin-plus-e2e.golden b/pkg/client/testdata/manual-custom-plugin-plus-e2e.golden
@@ -31,7 +31,6 @@ data:
       driver: Job
       plugin-name: e2e
       result-format: junit
-      result-type: e2e
     spec:
       command:
       - /run_e2e.sh
@@ -48,7 +47,6 @@ data:
     sonobuoy-config:
       driver: ""
       plugin-name: foo
-      result-type: ""
     spec:
       name: ""
       resources: {}

diff --git a/pkg/client/testdata/manual-custom-plugin-plus-systemd.golden b/pkg/client/testdata/manual-custom-plugin-plus-systemd.golden
@@ -30,7 +30,6 @@ data:
     sonobuoy-config:
       driver: ""
       plugin-name: foo
-      result-type: ""
     spec:
       name: ""
       resources: {}
@@ -39,7 +38,6 @@ data:
       driver: DaemonSet
       plugin-name: systemd-logs
       result-format: raw
-      result-type: systemd-logs
     spec:
       command:
       - /bin/sh

diff --git a/pkg/client/testdata/manual-custom-plugin.golden b/pkg/client/testdata/manual-custom-plugin.golden
@@ -30,7 +30,6 @@ data:
     sonobuoy-config:
       driver: ""
       plugin-name: foo
-      result-type: ""
     spec:
       name: ""
       resources: {}

diff --git a/pkg/client/testdata/manual-e2e.golden b/pkg/client/testdata/manual-e2e.golden
@@ -31,7 +31,6 @@ data:
       driver: Job
       plugin-name: e2e
       result-format: junit
-      result-type: e2e
     spec:
       command:
       - /run_e2e.sh

diff --git a/pkg/client/testdata/plugins-and-pluginSelection.golden b/pkg/client/testdata/plugins-and-pluginSelection.golden
@@ -30,15 +30,13 @@ data:
     sonobuoy-config:
       driver: ""
       plugin-name: a
-      result-type: ""
     spec:
       name: ""
       resources: {}
   plugin-1.yaml: |
     sonobuoy-config:
       driver: ""
       plugin-name: b
-      result-type: ""
     spec:
       name: ""
       resources: {}

diff --git a/pkg/client/testdata/ssh.golden b/pkg/client/testdata/ssh.golden
@@ -45,7 +45,6 @@ data:
       driver: Job
       plugin-name: e2e
       result-format: junit
-      result-type: e2e
     spec:
       command:
       - /run_e2e.sh

diff --git a/pkg/client/testdata/systemd-logs-default.golden b/pkg/client/testdata/systemd-logs-default.golden
@@ -31,7 +31,6 @@ data:
       driver: DaemonSet
       plugin-name: systemd-logs
       result-format: raw
-      result-type: systemd-logs
     spec:
       command:
       - /bin/sh

diff --git a/pkg/client/testdata/use-existing-pod-spec.golden b/pkg/client/testdata/use-existing-pod-spec.golden
@@ -32,7 +32,6 @@ data:
     sonobuoy-config:
       driver: ""
       plugin-name: a
-      result-type: ""
     spec:
       name: ""
       resources: {}

diff --git a/pkg/discovery/discovery.go b/pkg/discovery/discovery.go
@@ -201,7 +201,7 @@ func Run(restConf *rest.Config, cfg *config.Config) (errCount int) {
 			// Update the plugin status with this post-processed information.
 			statusInfo := map[string]int{}
 			statusCounts(&item, statusInfo)
-			updatePluginStatus(kubeClient, cfg.Namespace, p.GetResultType(), item.Status, statusInfo)
+			updatePluginStatus(kubeClient, cfg.Namespace, p.GetName(), item.Status, statusInfo)
 		}
 	}
 

diff --git a/pkg/plugin/aggregation/run.go b/pkg/plugin/aggregation/run.go
@@ -222,7 +222,7 @@ func (a *Aggregator) RunAndMonitorPlugin(ctx context.Context, p plugin.Interface
 	if err := p.Run(client, address, cert, aggregatorPod); err != nil {
 		err := errors.Wrapf(err, "error running plugin %v", p.GetName())
 		logrus.Error(err)
-		monitorCh <- utils.MakeErrorResult(p.GetResultType(), map[string]interface{}{"error": err.Error()}, "")
+		monitorCh <- utils.MakeErrorResult(p.GetName(), map[string]interface{}{"error": err.Error()}, "")
 	}
 
 	go p.Monitor(pCtx, client, nodes, monitorCh)
@@ -252,7 +252,7 @@ func (a *Aggregator) pluginHasResults(p plugin.Interface) bool {
 	a.resultsMutex.Lock()
 	defer a.resultsMutex.Unlock()
 
-	targetType := p.GetResultType()
+	targetType := p.GetName()
 	for expResultID, expResult := range a.ExpectedResults {
 		if expResult.ResultType != targetType {
 			continue

diff --git a/pkg/plugin/aggregation/run_test.go b/pkg/plugin/aggregation/run_test.go
@@ -36,7 +36,6 @@ func TestRunAndMonitorPlugin(t *testing.T) {
 			Definition: manifest.Manifest{
 				SonobuoyConfig: manifest.SonobuoyConfig{
 					PluginName: "myPlugin",
-					ResultType: "myPlugin",
 				},
 			},
 			Namespace: "testNS",
@@ -227,10 +226,6 @@ func (cp *MockCleanupPlugin) FillTemplate(_ string, _ *tls.Certificate) ([]byte,
 	return []byte{}, nil
 }
 
-func (cp *MockCleanupPlugin) GetResultType() string {
-	return "result-type"
-}
-
 func (cp *MockCleanupPlugin) GetName() string {
 	return "mock-cleanup-plugin"
 }

diff --git a/pkg/plugin/driver/base.go b/pkg/plugin/driver/base.go
@@ -58,11 +58,6 @@ func (b *Base) GetSecretName() string {
 	return fmt.Sprintf("sonobuoy-plugin-%s-%s", b.GetName(), b.GetSessionID())
 }
 
-// GetResultType returns the ResultType for this plugin (to adhere to plugin.Interface).
-func (b *Base) GetResultType() string {
-	return b.Definition.SonobuoyConfig.ResultType
-}
-
 // GetDriver returns the Driver for this plugin.
 func (b *Base) GetDriver() string {
 	return b.Definition.SonobuoyConfig.Driver
@@ -162,7 +157,7 @@ func (b *Base) workerEnvironment(hostname string, cert *tls.Certificate) []v1.En
 		},
 		{
 			Name:  "RESULT_TYPE",
-			Value: b.GetResultType(),
+			Value: b.GetName(),
 		},
 		{
 			Name:  "MASTER_URL",

diff --git a/pkg/plugin/driver/base_test.go b/pkg/plugin/driver/base_test.go
@@ -125,7 +125,6 @@ func TestCreateWorkerContainerDefinition(t *testing.T) {
 		Definition: manifest.Manifest{
 			SonobuoyConfig: manifest.SonobuoyConfig{
 				PluginName: "test-plugin",
-				ResultType: "test-plugin",
 			},
 		},
 		SonobuoyImage:   "sonobuoy:v1",
@@ -170,7 +169,7 @@ func TestCreateWorkerContainerDefinition(t *testing.T) {
 			},
 			{
 				Name:  "RESULT_TYPE",
-				Value: b.GetResultType(),
+				Value: b.GetName(),
 			},
 			{
 				Name:  "MASTER_URL",

diff --git a/pkg/plugin/driver/daemonset/daemonset.go b/pkg/plugin/driver/daemonset/daemonset.go
@@ -74,7 +74,7 @@ func (p *Plugin) ExpectedResults(nodes []v1.Node) []plugin.ExpectedResult {
 	for _, node := range nodes {
 		ret = append(ret, plugin.ExpectedResult{
 			NodeName:   node.Name,
-			ResultType: p.GetResultType(),
+			ResultType: p.GetName(),
 		})
 	}
 
@@ -88,9 +88,8 @@ func getAggregatorAddress(hostname string) string {
 func (p *Plugin) createDaemonSetDefinition(hostname string, cert *tls.Certificate, ownerPod *v1.Pod) appsv1.DaemonSet {
 	ds := appsv1.DaemonSet{}
 	annotations := map[string]string{
-		"sonobuoy-driver":      p.GetDriver(),
-		"sonobuoy-plugin":      p.GetName(),
-		"sonobuoy-result-type": p.GetResultType(),
+		"sonobuoy-driver": p.GetDriver(),
+		"sonobuoy-plugin": p.GetName(),
 	}
 	for k, v := range p.CustomAnnotations {
 		annotations[k] = v
@@ -291,7 +290,7 @@ func (p *Plugin) monitorOnce(kubeclient kubernetes.Interface, availableNodes []v
 		if isFailing, reason := utils.IsPodFailing(&pod); isFailing {
 			podsReported[nodeName] = true
 
-			retErrs = append(retErrs, utils.MakeErrorResult(p.GetResultType(), map[string]interface{}{
+			retErrs = append(retErrs, utils.MakeErrorResult(p.GetName(), map[string]interface{}{
 				"error": reason,
 				"pod":   pod,
 			}, nodeName))
@@ -306,7 +305,7 @@ func (p *Plugin) monitorOnce(kubeclient kubernetes.Interface, availableNodes []v
 	for _, node := range availableNodes {
 		if !podsFound[node.Name] && !podsReported[node.Name] {
 			podsReported[node.Name] = true
-			retErrs = append(retErrs, utils.MakeErrorResult(p.GetResultType(), map[string]interface{}{
+			retErrs = append(retErrs, utils.MakeErrorResult(p.GetName(), map[string]interface{}{
 				"error": fmt.Sprintf(
 					"No pod was scheduled on node %v within %v. Check tolerations for plugin %v",
 					node.Name,