Refactor/Cleanup for output parameter (#1645)

* Cleanup for output parameter

Use type for output
Add separate Options for plan status
Add tests to validate invalid output parameter
Cleanup tests to use separate t.Run executions

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

pkg/kudoctl/cmd/init.go

@@ -68,7 +68,7 @@ type initCmd struct {
 	image               string
 	imagePullPolicy     string
 	dryRun              bool
-	output              string
+	output              output.Type
 	version             string
 	ns                  string
 	serviceAccount      string
@@ -110,7 +110,7 @@ func newInitCmd(fs afero.Fs, out io.Writer, errOut io.Writer, client *kube.Clien
 	f.StringVarP(&i.image, "kudo-image", "i", "", "Override KUDO controller image and/or version")
 	f.StringVarP(&i.imagePullPolicy, "kudo-image-pull-policy", "", "Always", "Override KUDO controller image pull policy")
 	f.StringVarP(&i.version, "version", "", "", "Override KUDO controller version of the KUDO image")
-	f.StringVarP(&i.output, "output", "o", "", "Output format")
+	f.StringVarP(i.output.AsStringPtr(), "output", "o", "", "Output format")
 	f.BoolVar(&i.dryRun, "dry-run", false, "Do not install local or remote")
 	f.BoolVar(&i.upgrade, "upgrade", false, "Upgrade an existing KUDO installation")
 	f.BoolVar(&i.verify, "verify", false, "Verify an existing KUDO installation")
@@ -148,6 +148,9 @@ func (initCmd *initCmd) validate(flags *flag.FlagSet) error {
 	if initCmd.crdOnly && initCmd.upgrade {
 		return errors.New("'--upgrade' and '--crd-only' can not be used at the same time: you can not upgrade *only* crds")
 	}
+	if err := initCmd.output.Validate(); err != nil {
+		return err
+	}
 
 	return nil
 }

pkg/kudoctl/cmd/init_test.go

@@ -3,6 +3,7 @@ package cmd
 import (
 	"bytes"
 	"flag"
+	"fmt"
 	"io"
 	"io/ioutil"
 	"path/filepath"
@@ -26,6 +27,7 @@ import (
 	fake2 "sigs.k8s.io/controller-runtime/pkg/client/fake"
 
 	"github.com/kudobuilder/kudo/pkg/kudoctl/clog"
+	"github.com/kudobuilder/kudo/pkg/kudoctl/cmd/output"
 	"github.com/kudobuilder/kudo/pkg/kudoctl/kube"
 	"github.com/kudobuilder/kudo/pkg/kudoctl/kudohome"
 	"github.com/kudobuilder/kudo/pkg/kudoctl/util/repo"
@@ -92,43 +94,46 @@ func TestInitCmd_output(t *testing.T) {
 	MockCRD(c.client, "certificates.cert-manager.io", "v1alpha2")
 	MockCRD(c.client, "issuers.cert-manager.io", "v1alpha2")
 
-	tests := []string{"yaml"}
-	for _, s := range tests {
-		var buf bytes.Buffer
-		var errOut bytes.Buffer
-		cmd := &initCmd{
-			out:     &buf,
-			errOut:  &errOut,
-			client:  c.client,
-			output:  s,
-			dryRun:  true,
-			version: "dev",
-		}
-		// ensure that we can marshal
-		if err := cmd.run(); err != nil {
-			t.Fatal(err)
-		}
-		// ensure no modifying calls against the server
-		forbiddenVerbs := []string{"create", "update", "patch", "delete"}
-		for _, a := range c.fc.Actions() {
-			if funk.Contains(forbiddenVerbs, a.GetVerb()) {
-				t.Errorf("got modifying server call: %v", a)
+	tests := output.ValidTypes
+	for _, tt := range tests {
+		tt := tt
+		t.Run(fmt.Sprintf("output %s", tt), func(t *testing.T) {
+			var buf bytes.Buffer
+			var errOut bytes.Buffer
+			cmd := &initCmd{
+				out:     &buf,
+				errOut:  &errOut,
+				client:  c.client,
+				output:  tt,
+				dryRun:  true,
+				version: "dev",
+			}
+			// ensure that we can marshal
+			if err := cmd.run(); err != nil {
+				t.Fatal(err)
+			}
+			// ensure no modifying calls against the server
+			forbiddenVerbs := []string{"create", "update", "patch", "delete"}
+			for _, a := range c.fc.Actions() {
+				if funk.Contains(forbiddenVerbs, a.GetVerb()) {
+					t.Errorf("got modifying server call: %v", a)
+				}
 			}
-		}
 
-		assert.True(t, len(buf.Bytes()) > 0, "Buffer needs to have an output")
-		// ensure we can decode what was created
-		var obj interface{}
-		decoder := yamlutil.NewYAMLOrJSONDecoder(&buf, 4096)
-		for {
-			err := decoder.Decode(&obj)
-			if err != nil {
-				if err == io.EOF {
-					break
+			assert.True(t, len(buf.Bytes()) > 0, "Buffer needs to have an output")
+			// ensure we can decode what was created
+			var obj interface{}
+			decoder := yamlutil.NewYAMLOrJSONDecoder(&buf, 4096)
+			for {
+				err := decoder.Decode(&obj)
+				if err != nil {
+					if err == io.EOF {
+						break
+					}
+					t.Errorf("error decoding init %s output %s %s", tt, err, buf.String())
 				}
-				t.Errorf("error decoding init %s output %s %s", s, err, buf.String())
 			}
-		}
+		})
 	}
 }
 
@@ -167,50 +172,62 @@ func TestInitCmd_yamlOutput(t *testing.T) {
 	MockCRD(c.client, "issuers.cert-manager.io", "v1alpha2")
 
 	tests := []struct {
-		name       string
-		goldenFile string
-		flags      map[string]string
+		name          string
+		goldenFile    string
+		flags         map[string]string
+		expectedError string
 	}{
-		{"custom namespace", "deploy-kudo-ns.yaml", map[string]string{"dry-run": "true", "output": "yaml", "namespace": "foo", "version": "dev"}},
-		{"yaml output", "deploy-kudo.yaml", map[string]string{"dry-run": "true", "output": "yaml", "version": "dev"}},
-		{"service account", "deploy-kudo-sa.yaml", map[string]string{"dry-run": "true", "output": "yaml", "service-account": "safoo", "namespace": "foo", "version": "dev"}},
-		{"json output", "deploy-kudo.json", map[string]string{"dry-run": "true", "output": "json", "version": "dev"}},
+		{name: "custom namespace", goldenFile: "deploy-kudo-ns.yaml", flags: map[string]string{"dry-run": "true", "output": "yaml", "namespace": "foo", "version": "dev"}},
+		{name: "yaml output", goldenFile: "deploy-kudo.yaml", flags: map[string]string{"dry-run": "true", "output": "yaml", "version": "dev"}},
+		{name: "service account", goldenFile: "deploy-kudo-sa.yaml", flags: map[string]string{"dry-run": "true", "output": "yaml", "service-account": "safoo", "namespace": "foo", "version": "dev"}},
+		{name: "json output", goldenFile: "deploy-kudo.json", flags: map[string]string{"dry-run": "true", "output": "json", "version": "dev"}},
+		{name: "invalid output", expectedError: output.InvalidOutputError, flags: map[string]string{"dry-run": "true", "output": "invalid", "version": "dev"}},
 	}
 
 	for _, tt := range tests {
-		fs := afero.NewMemMapFs()
-		out := &bytes.Buffer{}
-		errOut := &bytes.Buffer{}
-		initCmd := newInitCmd(fs, out, errOut, c.client)
+		tt := tt
+
+		t.Run(tt.name, func(t *testing.T) {
+			fs := afero.NewMemMapFs()
+			out := &bytes.Buffer{}
+			errOut := &bytes.Buffer{}
+			initCmd := newInitCmd(fs, out, errOut, c.client)
 
-		Settings.AddFlags(initCmd.Flags())
+			Settings.AddFlags(initCmd.Flags())
 
-		for f, value := range tt.flags {
-			if err := initCmd.Flags().Set(f, value); err != nil {
-				t.Fatal(err)
+			for f, value := range tt.flags {
+				if err := initCmd.Flags().Set(f, value); err != nil {
+					t.Fatal(err)
+				}
 			}
-		}
 
-		if err := initCmd.RunE(initCmd, []string{}); err != nil {
-			t.Fatal(err, errOut.String())
-		}
+			if err := initCmd.RunE(initCmd, []string{}); err != nil {
+				if tt.expectedError != "" {
+					assert.Equal(t, tt.expectedError, err.Error())
+				} else {
+					t.Fatal(err, errOut.String())
+				}
+			}
 
-		gp := filepath.Join("testdata", tt.goldenFile+".golden")
+			if tt.goldenFile != "" {
+				gp := filepath.Join("testdata", tt.goldenFile+".golden")
 
-		if *updateGolden {
-			t.Logf("updating golden file %s", tt.goldenFile)
+				if *updateGolden {
+					t.Logf("updating golden file %s", tt.goldenFile)
 
-			//nolint:gosec
-			if err := ioutil.WriteFile(gp, out.Bytes(), 0644); err != nil {
-				t.Fatalf("failed to update golden file: %s", err)
-			}
-		}
-		g, err := ioutil.ReadFile(gp)
-		if err != nil {
-			t.Fatalf("failed reading .golden: %s", err)
-		}
+					//nolint:gosec
+					if err := ioutil.WriteFile(gp, out.Bytes(), 0644); err != nil {
+						t.Fatalf("failed to update golden file: %s", err)
+					}
+				}
+				g, err := ioutil.ReadFile(gp)
+				if err != nil {
+					t.Fatalf("failed reading .golden: %s", err)
+				}
 
-		assert.Equal(t, string(g), out.String(), "for golden file: %s, for test %s", gp, tt.name)
+				assert.Equal(t, string(g), out.String(), "for golden file: %s, for test %s", gp, tt.name)
+			}
+		})
 	}
 
 }

pkg/kudoctl/cmd/output/output.go

@@ -4,18 +4,45 @@ import (
 	"encoding/json"
 	"fmt"
 	"io"
-	"strings"
 
+	"github.com/thoas/go-funk"
 	"sigs.k8s.io/yaml"
 )
 
+// OutputType specifies the type of output a command produces
+type Type string
+
 const (
-	TypeYAML = "yaml"
-	TypeJSON = "json"
+	// StringValueType is used for parameter values that are provided as a string.
+	TypeYAML Type = "yaml"
+
+	// ArrayValueType is used for parameter values that described an array of values.
+	TypeJSON Type = "json"
+
+	InvalidOutputError = "invalid output format, only support 'yaml' or 'json' or empty"
 )
 
-func WriteObjects(objs []interface{}, outputType string, out io.Writer) error {
-	if strings.ToLower(outputType) == TypeYAML {
+var (
+	ValidTypes = []Type{TypeYAML, TypeJSON}
+)
+
+func (t *Type) AsStringPtr() *string {
+	return (*string)(t)
+}
+
+func (t Type) Validate() error {
+	if t == "" {
+		return nil
+	}
+	if funk.Contains(ValidTypes, t) {
+		return nil
+	}
+
+	return fmt.Errorf(InvalidOutputError)
+}
+
+func WriteObjects(objs []interface{}, outputType Type, out io.Writer) error {
+	if outputType == TypeYAML {
 		// Write YAML objects with separators
 		for _, obj := range objs {
 			if _, err := fmt.Fprintln(out, "---"); err != nil {
@@ -30,21 +57,21 @@ func WriteObjects(objs []interface{}, outputType string, out io.Writer) error {
 		// YAML ending document boundary marker
 		_, err := fmt.Fprintln(out, "...")
 		return err
-	} else if strings.ToLower(outputType) == TypeJSON {
+	} else if outputType == TypeJSON {
 		return writeObjectJSON(objs, out)
 	}
 
-	return fmt.Errorf("invalid output format, only support yaml or json")
+	return fmt.Errorf(InvalidOutputError)
 }
 
-func WriteObject(obj interface{}, outputType string, out io.Writer) error {
-	if strings.ToLower(outputType) == TypeYAML {
+func WriteObject(obj interface{}, outputType Type, out io.Writer) error {
+	if outputType == TypeYAML {
 		return writeObjectYAML(obj, out)
-	} else if strings.ToLower(outputType) == TypeJSON {
+	} else if outputType == TypeJSON {
 		return writeObjectJSON(obj, out)
 	}
 
-	return fmt.Errorf("invalid output format, only support yaml or json")
+	return fmt.Errorf(InvalidOutputError)
 }
 
 func writeObjectJSON(obj interface{}, out io.Writer) error {

pkg/kudoctl/cmd/plan.go

@@ -56,7 +56,7 @@ func NewPlanHistoryCmd() *cobra.Command {
 
 //NewPlanStatusCmd creates a new command that shows the status of an instance by looking at its current plan
 func NewPlanStatusCmd(out io.Writer) *cobra.Command {
-	options := &plan.Options{Out: out}
+	options := &plan.StatusOptions{Out: out}
 	cmd := &cobra.Command{
 		Use:     "status",
 		Short:   "Shows the status of all plans to an particular instance.",
@@ -68,7 +68,7 @@ func NewPlanStatusCmd(out io.Writer) *cobra.Command {
 
 	cmd.Flags().StringVar(&options.Instance, "instance", "", "The instance name available from 'kubectl get instances'")
 	cmd.Flags().BoolVar(&options.Wait, "wait", false, "Specify if the CLI should wait for the plan to complete before returning (default \"false\")")
-	cmd.Flags().StringVarP(&options.Output, "output", "o", "", "Output format")
+	cmd.Flags().StringVarP(options.Output.AsStringPtr(), "output", "o", "", "Output format")
 
 	if err := cmd.MarkFlagRequired("instance"); err != nil {
 		clog.Printf("Please choose the instance with '--instance=<instanceName>': %v", err)

pkg/kudoctl/cmd/plan/plan_history.go

@@ -3,7 +3,6 @@ package plan
 import (
 	"errors"
 	"fmt"
-	"io"
 	"sort"
 
 	"github.com/spf13/cobra"
@@ -16,10 +15,7 @@ import (
 
 // Options are the configurable options for plans
 type Options struct {
-	Out      io.Writer
 	Instance string
-	Wait     bool
-	Output   string
 }
 
 var (

pkg/kudoctl/cmd/plan/plan_status.go

@@ -16,8 +16,16 @@ import (
 	"github.com/kudobuilder/kudo/pkg/kudoctl/util/kudo"
 )
 
+// Options are the configurable options for plans
+type StatusOptions struct {
+	Out      io.Writer
+	Instance string
+	Wait     bool
+	Output   output.Type
+}
+
 // Status runs the plan status command
-func Status(options *Options, settings *env.Settings) error {
+func Status(options *StatusOptions, settings *env.Settings) error {
 	kc, err := env.GetClient(settings)
 	if err != nil {
 		return err
@@ -26,7 +34,7 @@ func Status(options *Options, settings *env.Settings) error {
 	return status(kc, options, settings.Namespace)
 }
 
-func statusFormatted(kc *kudo.Client, options *Options, ns string) error {
+func statusFormatted(kc *kudo.Client, options *StatusOptions, ns string) error {
 	instance, err := kc.GetInstance(options.Instance, ns)
 	if err != nil {
 		return err
@@ -37,7 +45,7 @@ func statusFormatted(kc *kudo.Client, options *Options, ns string) error {
 	return output.WriteObject(instance.Status, options.Output, options.Out)
 }
 
-func status(kc *kudo.Client, options *Options, ns string) error {
+func status(kc *kudo.Client, options *StatusOptions, ns string) error {
 
 	if options.Output != "" {
 		return statusFormatted(kc, options, ns)