[#42] Refactor flag operations to be uniform

This also increases unit test coverage

Change-Id: I324954a1216f2204ff3977c05eb9bd087cd87795

cmd/config/get_authinfo.go

@@ -36,19 +36,19 @@ airshipctl config get-credential e2e`
 // An AuthInfo refers to a particular user for a cluster
 // NewCmdConfigGetAuthInfo returns a Command instance for 'config -AuthInfo' sub command
 func NewCmdConfigGetAuthInfo(rootSettings *environment.AirshipCTLSettings) *cobra.Command {
-	theAuthInfo := &config.AuthInfoOptions{}
+	o := &config.AuthInfoOptions{}
-	getauthinfocmd := &cobra.Command{
+	cmd := &cobra.Command{
 		Use:     "get-credentials NAME",
 		Short:   "Gets a user entry from the airshipctl config",
 		Long:    getAuthInfoLong,
 		Example: getAuthInfoExample,
 		RunE: func(cmd *cobra.Command, args []string) error {
 			if len(args) == 1 {
-				theAuthInfo.Name = args[0]
+				o.Name = args[0]
 			}
-			return config.RunGetAuthInfo(theAuthInfo, cmd.OutOrStdout(), rootSettings.Config())
+			return config.RunGetAuthInfo(o, cmd.OutOrStdout(), rootSettings.Config())
 		},
 	}
 
-	return getauthinfocmd
+	return cmd
 }

cmd/config/get_cluster.go

@@ -38,32 +38,35 @@ airshipctl config get-cluster e2e --%v=ephemeral`, config.FlagClusterType)
 
 // NewCmdConfigGetCluster returns a Command instance for 'config -Cluster' sub command
 func NewCmdConfigGetCluster(rootSettings *environment.AirshipCTLSettings) *cobra.Command {
-	theCluster := &config.ClusterOptions{}
+	o := &config.ClusterOptions{}
-	getclustercmd := &cobra.Command{
+	cmd := &cobra.Command{
 		Use:     "get-cluster NAME",
 		Short:   getClusterLong,
 		Example: getClusterExample,
 		RunE: func(cmd *cobra.Command, args []string) error {
 			if len(args) == 1 {
-				theCluster.Name = args[0]
+				o.Name = args[0]
 			}
 
-			err := validate(theCluster)
+			err := validate(o)
 			if err != nil {
 				return err
 			}
 
-			return config.RunGetCluster(theCluster, cmd.OutOrStdout(), rootSettings.Config())
+			return config.RunGetCluster(o, cmd.OutOrStdout(), rootSettings.Config())
 		},
 	}
 
-	gcInitFlags(theCluster, getclustercmd)
+	addGetClusterFlags(o, cmd)
-
+	return cmd
-	return getclustercmd
 }
 
-func gcInitFlags(o *config.ClusterOptions, getclustercmd *cobra.Command) {
+func addGetClusterFlags(o *config.ClusterOptions, cmd *cobra.Command) {
-	getclustercmd.Flags().StringVar(&o.ClusterType, config.FlagClusterType, "",
+	flags := cmd.Flags()
+	flags.StringVar(
+		&o.ClusterType,
+		config.FlagClusterType,
+		"",
 		config.FlagClusterType+" for the cluster entry in airshipctl config")
 }
 

cmd/config/get_context.go

@@ -45,25 +45,29 @@ airshipctl config get-context e2e`,
 
 // NewCmdConfigGetContext returns a Command instance for 'config -Context' sub command
 func NewCmdConfigGetContext(rootSettings *environment.AirshipCTLSettings) *cobra.Command {
-	theContext := &config.ContextOptions{}
+	o := &config.ContextOptions{}
-	getcontextcmd := &cobra.Command{
+	cmd := &cobra.Command{
 		Use:     "get-context NAME",
 		Short:   getContextLong,
 		Example: getContextExample,
 		RunE: func(cmd *cobra.Command, args []string) error {
 			if len(args) == 1 {
-				theContext.Name = args[0]
+				o.Name = args[0]
 			}
-			return config.RunGetContext(theContext, cmd.OutOrStdout(), rootSettings.Config())
+			return config.RunGetContext(o, cmd.OutOrStdout(), rootSettings.Config())
 		},
 	}
 
-	gctxInitFlags(theContext, getcontextcmd)
+	addGetContextFlags(o, cmd)
-
+	return cmd
-	return getcontextcmd
 }
 
-func gctxInitFlags(o *config.ContextOptions, getcontextcmd *cobra.Command) {
+func addGetContextFlags(o *config.ContextOptions, cmd *cobra.Command) {
-	getcontextcmd.Flags().BoolVar(&o.CurrentContext, config.FlagCurrentContext, false,
+	flags := cmd.Flags()
-		config.FlagCurrentContext+" to retrieve the current context entry in airshipctl config")
+
+	flags.BoolVar(
+		&o.CurrentContext,
+		config.FlagCurrentContext,
+		false,
+		"retrieve the current context entry in airshipctl config")
 }

cmd/config/set_authinfo.go

@@ -23,7 +23,6 @@ import (
 
 	"opendev.org/airship/airshipctl/pkg/config"
 	"opendev.org/airship/airshipctl/pkg/environment"
-	"opendev.org/airship/airshipctl/pkg/log"
 )
 
 var (
@@ -67,62 +66,69 @@ airshipctl config set-credentials cluster-admin --%v=~/.kube/admin.crt --%v=true
 // NewCmdConfigSetAuthInfo creates a command object for the "set-credentials" action, which
 // defines a new AuthInfo airship config.
 func NewCmdConfigSetAuthInfo(rootSettings *environment.AirshipCTLSettings) *cobra.Command {
-	theAuthInfo := &config.AuthInfoOptions{}
+	o := &config.AuthInfoOptions{}
 
-	setauthinfo := &cobra.Command{
+	cmd := &cobra.Command{
 		Use:     "set-credentials NAME",
 		Short:   "Sets a user entry in the airshipctl config",
 		Long:    setAuthInfoLong,
 		Example: setAuthInfoExample,
 		Args:    cobra.ExactArgs(1),
 		RunE: func(cmd *cobra.Command, args []string) error {
-			theAuthInfo.Name = args[0]
+			o.Name = args[0]
-			modified, err := config.RunSetAuthInfo(theAuthInfo, rootSettings.Config(), true)
+			modified, err := config.RunSetAuthInfo(o, rootSettings.Config(), true)
 			if err != nil {
 				return err
 			}
 			if modified {
-				fmt.Fprintf(cmd.OutOrStdout(), "User information %q modified.\n", theAuthInfo.Name)
+				fmt.Fprintf(cmd.OutOrStdout(), "User information %q modified.\n", o.Name)
 			} else {
-				fmt.Fprintf(cmd.OutOrStdout(), "User information %q created.\n", theAuthInfo.Name)
+				fmt.Fprintf(cmd.OutOrStdout(), "User information %q created.\n", o.Name)
 			}
 			return nil
 		},
 	}
 
-	err := suInitFlags(theAuthInfo, setauthinfo)
+	addSetAuthInfoFlags(o, cmd)
-	if err != nil {
+	return cmd
-		log.Fatal(err)
-	}
-	return setauthinfo
 }
 
-func suInitFlags(o *config.AuthInfoOptions, setauthinfo *cobra.Command) error {
+func addSetAuthInfoFlags(o *config.AuthInfoOptions, cmd *cobra.Command) {
-	setauthinfo.Flags().StringVar(&o.ClientCertificate, config.FlagCertFile, o.ClientCertificate,
+	flags := cmd.Flags()
+
+	flags.StringVar(
+		&o.ClientCertificate,
+		config.FlagCertFile,
+		"",
 		"Path to "+config.FlagCertFile+" file for the user entry in airshipctl")
-	err := setauthinfo.MarkFlagFilename(config.FlagCertFile)
-	if err != nil {
-		return err
-	}
 
-	setauthinfo.Flags().StringVar(&o.ClientKey, config.FlagKeyFile, o.ClientKey,
+	flags.StringVar(
+		&o.ClientKey,
+		config.FlagKeyFile,
+		"",
 		"Path to "+config.FlagKeyFile+" file for the user entry in airshipctl")
-	err = setauthinfo.MarkFlagFilename(config.FlagKeyFile)
-	if err != nil {
-		return err
-	}
 
-	setauthinfo.Flags().StringVar(&o.Token, config.FlagBearerToken, o.Token,
+	flags.StringVar(
+		&o.Token,
+		config.FlagBearerToken,
+		"",
 		config.FlagBearerToken+" for the user entry in airshipctl")
 
-	setauthinfo.Flags().StringVar(&o.Username, config.FlagUsername, o.Username,
+	flags.StringVar(
+		&o.Username,
+		config.FlagUsername,
+		"",
 		config.FlagUsername+" for the user entry in airshipctl")
 
-	setauthinfo.Flags().StringVar(&o.Password, config.FlagPassword, o.Password,
+	flags.StringVar(
+		&o.Password,
+		config.FlagPassword,
+		"",
 		config.FlagPassword+" for the user entry in airshipctl")
 
-	setauthinfo.Flags().BoolVar(&o.EmbedCertData, config.FlagEmbedCerts, false,
+	flags.BoolVar(
+		&o.EmbedCertData,
+		config.FlagEmbedCerts,
+		false,
 		"Embed client cert/key for the user entry in airshipctl")
-
-	return nil
 }

cmd/config/set_cluster.go

@@ -57,52 +57,69 @@ airshipctl config set-cluster e2e --%v=target --%v=true --%v=".airship/cert_file
 // NewCmdConfigSetCluster creates a command object for the "set-cluster" action, which
 // defines a new cluster airshipctl config.
 func NewCmdConfigSetCluster(rootSettings *environment.AirshipCTLSettings) *cobra.Command {
-	theCluster := &config.ClusterOptions{}
+	o := &config.ClusterOptions{}
-
+	cmd := &cobra.Command{
-	setclustercmd := &cobra.Command{
 		Use:     "set-cluster NAME",
 		Short:   "Sets a cluster entry in the airshipctl config",
 		Long:    setClusterLong,
 		Example: setClusterExample,
 		Args:    cobra.ExactArgs(1),
 		RunE: func(cmd *cobra.Command, args []string) error {
-			theCluster.Name = cmd.Flags().Args()[0]
+			o.Name = args[0]
-			modified, err := config.RunSetCluster(theCluster, rootSettings.Config(), true)
+			modified, err := config.RunSetCluster(o, rootSettings.Config(), true)
 			if err != nil {
 				return err
 			}
 			if modified {
 				fmt.Fprintf(cmd.OutOrStdout(), "Cluster %q of type %q modified.\n",
-					theCluster.Name, theCluster.ClusterType)
+					o.Name, o.ClusterType)
 			} else {
 				fmt.Fprintf(cmd.OutOrStdout(), "Cluster %q of type %q created.\n",
-					theCluster.Name, theCluster.ClusterType)
+					o.Name, o.ClusterType)
 			}
 			return nil
 		},
 	}
 
-	scInitFlags(theCluster, setclustercmd)
+	addSetClusterFlags(o, cmd)
-	return setclustercmd
+	return cmd
 }
 
-func scInitFlags(o *config.ClusterOptions, setclustercmd *cobra.Command) {
+func addSetClusterFlags(o *config.ClusterOptions, cmd *cobra.Command) {
-	setclustercmd.Flags().StringVar(&o.Server, config.FlagAPIServer, o.Server,
+	flags := cmd.Flags()
+
+	flags.StringVar(
+		&o.Server,
+		config.FlagAPIServer,
+		"",
 		config.FlagAPIServer+" for the cluster entry in airshipctl config")
 
-	setclustercmd.Flags().StringVar(&o.ClusterType, config.FlagClusterType, o.ClusterType,
+	flags.StringVar(
+		&o.ClusterType,
+		config.FlagClusterType,
+		"",
 		config.FlagClusterType+" for the cluster entry in airshipctl config")
 
-	setclustercmd.Flags().BoolVar(&o.InsecureSkipTLSVerify, config.FlagInsecure, true,
+	err := cmd.MarkFlagRequired(config.FlagClusterType)
-		config.FlagInsecure+" for the cluster entry in airshipctl config")
-
-	setclustercmd.Flags().StringVar(&o.CertificateAuthority, config.FlagCAFile, o.CertificateAuthority,
-		"Path to "+config.FlagCAFile+" file for the cluster entry in airshipctl config")
-	err := setclustercmd.MarkFlagFilename(config.FlagCAFile)
 	if err != nil {
 		log.Fatal(err)
 	}
 
-	setclustercmd.Flags().BoolVar(&o.EmbedCAData, config.FlagEmbedCerts, false,
+	flags.BoolVar(
+		&o.InsecureSkipTLSVerify,
+		config.FlagInsecure,
+		true,
+		config.FlagInsecure+" for the cluster entry in airshipctl config")
+
+	flags.StringVar(
+		&o.CertificateAuthority,
+		config.FlagCAFile,
+		"",
+		"Path to "+config.FlagCAFile+" file for the cluster entry in airshipctl config")
+
+	flags.BoolVar(
+		&o.EmbedCAData,
+		config.FlagEmbedCerts,
+		false,
 		config.FlagEmbedCerts+" for the cluster entry in airshipctl config")
 }

cmd/config/set_context.go

@@ -44,11 +44,11 @@ airshipctl config set-context e2e`,
 )
 
 // NewCmdConfigSetContext creates a command object for the "set-context" action, which
-// defines a new Context airshipctl config.
+// creates and modifies contexts in the airshipctl config
 func NewCmdConfigSetContext(rootSettings *environment.AirshipCTLSettings) *cobra.Command {
-	theContext := &config.ContextOptions{}
+	o := &config.ContextOptions{}
 
-	setcontextcmd := &cobra.Command{
+	cmd := &cobra.Command{
 		Use:     "set-context NAME",
 		Short:   "Switch to a new context or update context values in the airshipctl config",
 		Long:    setContextLong,
@@ -58,40 +58,56 @@ func NewCmdConfigSetContext(rootSettings *environment.AirshipCTLSettings) *cobra
 			nFlags := cmd.Flags().NFlag()
 			if nFlags == 0 {
 				// Change the current context to the provided context name if no flags are provided
-				theContext.CurrentContext = true
+				o.CurrentContext = true
 			}
-			theContext.Name = cmd.Flags().Args()[0]
+			o.Name = args[0]
-			modified, err := config.RunSetContext(theContext, rootSettings.Config(), true)
+			modified, err := config.RunSetContext(o, rootSettings.Config(), true)
-
 			if err != nil {
 				return err
 			}
 			if modified {
-				fmt.Fprintf(cmd.OutOrStdout(), "Context %q modified.\n", theContext.Name)
+				fmt.Fprintf(cmd.OutOrStdout(), "Context %q modified.\n", o.Name)
 			} else {
-				fmt.Fprintf(cmd.OutOrStdout(), "Context %q created.\n", theContext.Name)
+				fmt.Fprintf(cmd.OutOrStdout(), "Context %q created.\n", o.Name)
 			}
 			return nil
 		},
 	}
 
-	sctxInitFlags(theContext, setcontextcmd)
+	addSetContextFlags(o, cmd)
-	return setcontextcmd
+	return cmd
 }
 
-func sctxInitFlags(o *config.ContextOptions, setcontextcmd *cobra.Command) {
+func addSetContextFlags(o *config.ContextOptions, cmd *cobra.Command) {
-	setcontextcmd.Flags().StringVar(&o.Cluster, config.FlagClusterName, o.Cluster,
+	flags := cmd.Flags()
-		config.FlagClusterName+" for the context entry in airshipctl config")
 
-	setcontextcmd.Flags().StringVar(&o.AuthInfo, config.FlagAuthInfoName, o.AuthInfo,
+	flags.StringVar(
-		config.FlagAuthInfoName+" for the context entry in airshipctl config")
+		&o.Cluster,
+		config.FlagClusterName,
+		"",
+		"sets the "+config.FlagClusterName+" for the specified context in the airshipctl config")
 
-	setcontextcmd.Flags().StringVar(&o.Manifest, config.FlagManifest, o.Manifest,
+	flags.StringVar(
-		config.FlagManifest+" for the context entry in airshipctl config")
+		&o.AuthInfo,
+		config.FlagAuthInfoName,
+		"",
+		"sets the "+config.FlagAuthInfoName+" for the specified context in the airshipctl config")
 
-	setcontextcmd.Flags().StringVar(&o.Namespace, config.FlagNamespace, o.Namespace,
+	flags.StringVar(
-		config.FlagNamespace+" for the context entry in airshipctl config")
+		&o.Manifest,
+		config.FlagManifest,
+		"",
+		"sets the "+config.FlagManifest+" for the specified context in the airshipctl config")
 
-	setcontextcmd.Flags().StringVar(&o.ClusterType, config.FlagClusterType, "",
+	flags.StringVar(
-		config.FlagClusterType+" for the context entry in airshipctl config")
+		&o.Namespace,
+		config.FlagNamespace,
+		"",
+		"sets the "+config.FlagNamespace+" for the specified context in the airshipctl config")
+
+	flags.StringVar(
+		&o.ClusterType,
+		config.FlagClusterType,
+		"",
+		"sets the "+config.FlagClusterType+" for the specified context in the airshipctl config")
 }

cmd/config/testdata/TestConfigSetContextGoldenOutput/config-cmd-set-context-too-few-args.golden

@@ -11,10 +11,10 @@ airshipctl config set-context e2e --namespace=kube-system --manifest=manifest --
 airshipctl config set-context e2e
 
 Flags:
-      --cluster string        cluster for the context entry in airshipctl config
+      --cluster string        sets the cluster for the specified context in the airshipctl config
-      --cluster-type string   cluster-type for the context entry in airshipctl config
+      --cluster-type string   sets the cluster-type for the specified context in the airshipctl config
   -h, --help                  help for set-context
-      --manifest string       manifest for the context entry in airshipctl config
+      --manifest string       sets the manifest for the specified context in the airshipctl config
-      --namespace string      namespace for the context entry in airshipctl config
+      --namespace string      sets the namespace for the specified context in the airshipctl config
-      --user string           user for the context entry in airshipctl config
+      --user string           sets the user for the specified context in the airshipctl config
 

cmd/config/testdata/TestConfigSetContextGoldenOutput/config-cmd-set-context-too-many-args.golden

@@ -11,10 +11,10 @@ airshipctl config set-context e2e --namespace=kube-system --manifest=manifest --
 airshipctl config set-context e2e
 
 Flags:
-      --cluster string        cluster for the context entry in airshipctl config
+      --cluster string        sets the cluster for the specified context in the airshipctl config
-      --cluster-type string   cluster-type for the context entry in airshipctl config
+      --cluster-type string   sets the cluster-type for the specified context in the airshipctl config
   -h, --help                  help for set-context
-      --manifest string       manifest for the context entry in airshipctl config
+      --manifest string       sets the manifest for the specified context in the airshipctl config
-      --namespace string      namespace for the context entry in airshipctl config
+      --namespace string      sets the namespace for the specified context in the airshipctl config
-      --user string           user for the context entry in airshipctl config
+      --user string           sets the user for the specified context in the airshipctl config
 

cmd/config/testdata/TestConfigSetContextGoldenOutput/config-cmd-set-context-with-help.golden

@@ -14,9 +14,9 @@ airshipctl config set-context e2e --namespace=kube-system --manifest=manifest --
 airshipctl config set-context e2e
 
 Flags:
-      --cluster string        cluster for the context entry in airshipctl config
+      --cluster string        sets the cluster for the specified context in the airshipctl config
-      --cluster-type string   cluster-type for the context entry in airshipctl config
+      --cluster-type string   sets the cluster-type for the specified context in the airshipctl config
   -h, --help                  help for set-context
-      --manifest string       manifest for the context entry in airshipctl config
+      --manifest string       sets the manifest for the specified context in the airshipctl config
-      --namespace string      namespace for the context entry in airshipctl config
+      --namespace string      sets the namespace for the specified context in the airshipctl config
-      --user string           user for the context entry in airshipctl config
+      --user string           sets the user for the specified context in the airshipctl config

cmd/config/testdata/TestGetContextCmdGoldenOutput/missing.golden

@@ -13,6 +13,6 @@ airshipctl config get-context --current-context
 airshipctl config get-context e2e
 
 Flags:
-      --current-context   current-context to retrieve the current context entry in airshipctl config
+      --current-context   retrieve the current context entry in airshipctl config
   -h, --help              help for get-context
 

cmd/document/render.go

@@ -8,16 +8,6 @@ import (
 	"opendev.org/airship/airshipctl/pkg/errors"
 )
 
-// InitFlags add flags for document render sub-command
-func initRenderFlags(settings *render.Settings, cmd *cobra.Command) {
-	flags := cmd.Flags()
-	flags.StringArrayVarP(&settings.Label, "label", "l", nil, "Filter documents by Labels")
-	flags.StringArrayVarP(&settings.Annotation, "annotation", "a", nil, "Filter documents by Annotations")
-	flags.StringArrayVarP(&settings.GroupVersion, "apiversion", "g", nil, "Filter documents by API version")
-	flags.StringArrayVarP(&settings.Kind, "kind", "k", nil, "Filter documents by Kinds")
-	flags.StringVarP(&settings.RawFilter, "filter", "f", "", "Logical expression for document filtering")
-}
-
 // NewRenderCommand create a new command for document rendering
 func NewRenderCommand(rootSettings *environment.AirshipCTLSettings) *cobra.Command {
 	renderSettings := &render.Settings{AirshipCTLSettings: rootSettings}
@@ -29,6 +19,46 @@ func NewRenderCommand(rootSettings *environment.AirshipCTLSettings) *cobra.Comma
 		},
 	}
 
-	initRenderFlags(renderSettings, renderCmd)
+	addRenderFlags(renderSettings, renderCmd)
 	return renderCmd
 }
+
+// addRenderFlags adds flags for document render sub-command
+func addRenderFlags(settings *render.Settings, cmd *cobra.Command) {
+	flags := cmd.Flags()
+
+	flags.StringArrayVarP(
+		&settings.Label,
+		"label",
+		"l",
+		nil,
+		"Filter documents by Labels")
+
+	flags.StringArrayVarP(
+		&settings.Annotation,
+		"annotation",
+		"a",
+		nil,
+		"Filter documents by Annotations")
+
+	flags.StringArrayVarP(
+		&settings.GroupVersion,
+		"apiversion",
+		"g",
+		nil,
+		"Filter documents by API version")
+
+	flags.StringArrayVarP(
+		&settings.Kind,
+		"kind",
+		"k",
+		nil,
+		"Filter documents by Kinds")
+
+	flags.StringVarP(
+		&settings.RawFilter,
+		"filter",
+		"f",
+		"",
+		"Logical expression for document filtering")
+}

pkg/config/cmds.go

@@ -20,76 +20,9 @@ import (
 	"errors"
 	"fmt"
 	"io"
-	"io/ioutil"
 )
 
-// Validate that the arguments are correct
+// RunGetAuthInfo performs the execution of 'config get-credentials' sub command
-func (o *ClusterOptions) Validate() error {
-	if len(o.Name) == 0 {
-		return errors.New("you must specify a non-empty cluster name")
-	}
-	err := ValidClusterType(o.ClusterType)
-	if err != nil {
-		return err
-	}
-	if o.InsecureSkipTLSVerify && o.CertificateAuthority != "" {
-		return fmt.Errorf("you cannot specify a %s and %s mode at the same time", FlagCAFile, FlagInsecure)
-	}
-
-	if !o.EmbedCAData {
-		return nil
-	}
-	caPath := o.CertificateAuthority
-	if caPath == "" {
-		return fmt.Errorf("you must specify a --%s to embed", FlagCAFile)
-	}
-	if _, err := ioutil.ReadFile(caPath); err != nil {
-		return fmt.Errorf("could not read %s data from %s: %v", FlagCAFile, caPath, err)
-	}
-	return nil
-}
-
-func (o *ContextOptions) Validate() error {
-	if len(o.Name) == 0 {
-		return errors.New("you must specify a non-empty context name")
-	}
-	// Expect ClusterType only when this is not setting currentContext
-	if o.ClusterType != "" {
-		err := ValidClusterType(o.ClusterType)
-		if err != nil {
-			return err
-		}
-	}
-	// TODO Manifest, Cluster could be validated against the existing config maps
-	return nil
-}
-
-func (o *AuthInfoOptions) Validate() error {
-	if len(o.Token) > 0 && (len(o.Username) > 0 || len(o.Password) > 0) {
-		return fmt.Errorf("you cannot specify more than one authentication method at the same time: --%v  or --%v/--%v",
-			FlagBearerToken, FlagUsername, FlagPassword)
-	}
-	if !o.EmbedCertData {
-		return nil
-	}
-	certPath := o.ClientCertificate
-	if certPath == "" {
-		return fmt.Errorf("you must specify a --%s to embed", FlagCertFile)
-	}
-	if _, err := ioutil.ReadFile(certPath); err != nil {
-		return fmt.Errorf("error reading %s data from %s: %v", FlagCertFile, certPath, err)
-	}
-	keyPath := o.ClientKey
-	if keyPath == "" {
-		return fmt.Errorf("you must specify a --%s to embed", FlagKeyFile)
-	}
-	if _, err := ioutil.ReadFile(keyPath); err != nil {
-		return fmt.Errorf("error reading %s data from %s: %v", FlagKeyFile, keyPath, err)
-	}
-	return nil
-}
-
-// runGetAuthInfo performs the execution of 'config get-credentials' sub command
 func RunGetAuthInfo(o *AuthInfoOptions, out io.Writer, airconfig *Config) error {
 	if o.Name == "" {
 		getAuthInfos(out, airconfig)
@@ -117,7 +50,7 @@ func getAuthInfos(out io.Writer, airconfig *Config) {
 	}
 }
 
-// runGetCluster performs the execution of 'config get-cluster' sub command
+// RunGetCluster performs the execution of 'config get-cluster' sub command
 func RunGetCluster(o *ClusterOptions, out io.Writer, airconfig *Config) error {
 	if o.Name == "" {
 		getClusters(out, airconfig)
@@ -126,8 +59,7 @@ func RunGetCluster(o *ClusterOptions, out io.Writer, airconfig *Config) error {
 	return getCluster(o.Name, o.ClusterType, out, airconfig)
 }
 
-func getCluster(cName, cType string,
+func getCluster(cName, cType string, out io.Writer, airconfig *Config) error {
-	out io.Writer, airconfig *Config) error {
 	cluster, err := airconfig.GetCluster(cName, cType)
 	if err != nil {
 		return err
@@ -147,7 +79,7 @@ func getClusters(out io.Writer, airconfig *Config) {
 	}
 }
 
-// runGetContext performs the execution of 'config get-Context' sub command
+// RunGetContext performs the execution of 'config get-Context' sub command
 func RunGetContext(o *ContextOptions, out io.Writer, airconfig *Config) error {
 	if o.Name == "" && !o.CurrentContext {
 		getContexts(out, airconfig)
@@ -157,11 +89,10 @@ func RunGetContext(o *ContextOptions, out io.Writer, airconfig *Config) error {
 }
 
 func getContext(o *ContextOptions, out io.Writer, airconfig *Config) error {
-	cName := o.Name
 	if o.CurrentContext {
-		cName = airconfig.CurrentContext
+		o.Name = airconfig.CurrentContext
 	}
-	context, err := airconfig.GetContext(cName)
+	context, err := airconfig.GetContext(o.Name)
 	if err != nil {
 		return err
 	}
@@ -180,19 +111,18 @@ func getContexts(out io.Writer, airconfig *Config) {
 }
 
 func RunSetAuthInfo(o *AuthInfoOptions, airconfig *Config, writeToStorage bool) (bool, error) {
-	authinfoWasModified := false
+	modified := false
 	err := o.Validate()
 	if err != nil {
-		return authinfoWasModified, err
+		return modified, err
 	}
 
-	authinfoIWant := o.Name
+	authinfo, err := airconfig.GetAuthInfo(o.Name)
-	authinfo, err := airconfig.GetAuthInfo(authinfoIWant)
 	if err != nil {
 		var cerr ErrMissingConfig
 		if !errors.As(err, &cerr) {
 			// An error occurred, but it wasn't a "missing" config error.
-			return authinfoWasModified, err
+			return modified, err
 		}
 
 		// authinfo didn't exist, create it
@@ -201,24 +131,24 @@ func RunSetAuthInfo(o *AuthInfoOptions, airconfig *Config, writeToStorage bool)
 	} else {
 		// AuthInfo exists, lets update
 		airconfig.ModifyAuthInfo(authinfo, o)
-		authinfoWasModified = true
+		modified = true
 	}
 	// Update configuration file just in time persistence approach
 	if writeToStorage {
 		if err := airconfig.PersistConfig(); err != nil {
 			// Error that it didnt persist the changes
-			return authinfoWasModified, ErrConfigFailed{}
+			return modified, ErrConfigFailed{}
 		}
 	}
 
-	return authinfoWasModified, nil
+	return modified, nil
 }
 
 func RunSetCluster(o *ClusterOptions, airconfig *Config, writeToStorage bool) (bool, error) {
-	clusterWasModified := false
+	modified := false
 	err := o.Validate()
 	if err != nil {
-		return clusterWasModified, err
+		return modified, err
 	}
 
 	cluster, err := airconfig.GetCluster(o.Name, o.ClusterType)
@@ -226,22 +156,22 @@ func RunSetCluster(o *ClusterOptions, airconfig *Config, writeToStorage bool) (b
 		var cerr ErrMissingConfig
 		if !errors.As(err, &cerr) {
 			// An error occurred, but it wasn't a "missing" config error.
-			return clusterWasModified, err
+			return modified, err
 		}
 
 		// Cluster didn't exist, create it
 		_, err := airconfig.AddCluster(o)
 		if err != nil {
-			return clusterWasModified, err
+			return modified, err
 		}
-		clusterWasModified = false
+		modified = false
 	} else {
 		// Cluster exists, lets update
 		_, err := airconfig.ModifyCluster(cluster, o)
 		if err != nil {
-			return clusterWasModified, err
+			return modified, err
 		}
-		clusterWasModified = true
+		modified = true
 	}
 
 	// Update configuration file
@@ -251,31 +181,30 @@ func RunSetCluster(o *ClusterOptions, airconfig *Config, writeToStorage bool) (b
 			// Some warning here , that it didnt persist the changes because of this
 			// Or should we float this up
 			// What would it mean? No value.
-			return clusterWasModified, err
+			return modified, err
 		}
 	}
 
-	return clusterWasModified, nil
+	return modified, nil
 }
 
 func RunSetContext(o *ContextOptions, airconfig *Config, writeToStorage bool) (bool, error) {
-	contextWasModified := false
+	modified := false
 	err := o.Validate()
 	if err != nil {
-		return contextWasModified, err
+		return modified, err
 	}
 
-	contextIWant := o.Name
+	context, err := airconfig.GetContext(o.Name)
-	context, err := airconfig.GetContext(contextIWant)
 	if err != nil {
 		var cerr ErrMissingConfig
 		if !errors.As(err, &cerr) {
 			// An error occurred, but it wasn't a "missing" config error.
-			return contextWasModified, err
+			return modified, err
 		}
 
 		if o.CurrentContext {
-			return contextWasModified, ErrMissingConfig{}
+			return modified, ErrMissingConfig{}
 		}
 		// context didn't exist, create it
 		// ignoring the returned added context
@@ -289,15 +218,15 @@ func RunSetContext(o *ContextOptions, airconfig *Config, writeToStorage bool) (b
 			// Context exists, lets update
 			airconfig.ModifyContext(context, o)
 		}
-		contextWasModified = true
+		modified = true
 	}
 	// Update configuration file just in time persistence approach
 	if writeToStorage {
 		if err := airconfig.PersistConfig(); err != nil {
 			// Error that it didnt persist the changes
-			return contextWasModified, ErrConfigFailed{}
+			return modified, ErrConfigFailed{}
 		}
 	}
 
-	return contextWasModified, nil
+	return modified, nil
 }

pkg/config/cmds_test.go

@@ -23,65 +23,6 @@ import (
 	"github.com/stretchr/testify/assert"
 )
 
-func TestValidateCluster(t *testing.T) {
-	co := DummyClusterOptions()
-
-	// Assert that the initial dummy config is valid
-	err := co.Validate()
-	assert.NoError(t, err)
-
-	// Validate with Embedded Data
-	// Empty CA
-	co.EmbedCAData = true
-	co.CertificateAuthority = ""
-	err = co.Validate()
-	assert.Error(t, err)
-
-	// Lets add a CA
-	co.CertificateAuthority = "testdata/ca.crt"
-	err = co.Validate()
-	assert.NoError(t, err)
-
-	// Lets add a CA but garbage
-	co.CertificateAuthority = "garbage"
-	err = co.Validate()
-	assert.Error(t, err)
-
-	// Lets change the Insecure mode
-	co.InsecureSkipTLSVerify = true
-	err = co.Validate()
-	assert.Error(t, err)
-
-	// Invalid Cluster Type
-	co.ClusterType = "Invalid"
-	err = co.Validate()
-	assert.Error(t, err)
-
-	// Empty Cluster Name case
-	co.Name = ""
-	err = co.Validate()
-	assert.Error(t, err)
-}
-
-func TestValidateContext(t *testing.T) {
-	co := DummyContextOptions()
-	// Valid Data case
-	err := co.Validate()
-	assert.NoError(t, err)
-}
-
-func TestValidateAuthInfo(t *testing.T) {
-	co := DummyAuthInfoOptions()
-	// Token and cert error case
-	err := co.Validate()
-	assert.Error(t, err)
-
-	// Valid Data case
-	co.Token = ""
-	err = co.Validate()
-	assert.NoError(t, err)
-}
-
 func TestRunGetAuthInfo(t *testing.T) {
 	t.Run("testNonExistentAuthInfo", func(t *testing.T) {
 		conf := DummyConfig()

pkg/config/cmds_types.go

@@ -1,30 +0,0 @@
-package config
-
-type ClusterOptions struct {
-	Name                  string
-	ClusterType           string
-	Server                string
-	InsecureSkipTLSVerify bool
-	CertificateAuthority  string
-	EmbedCAData           bool
-}
-
-type ContextOptions struct {
-	Name           string
-	ClusterType    string
-	CurrentContext bool
-	Cluster        string
-	AuthInfo       string
-	Manifest       string
-	Namespace      string
-}
-
-type AuthInfoOptions struct {
-	Name              string
-	ClientCertificate string
-	ClientKey         string
-	Token             string
-	Username          string
-	Password          string
-	EmbedCertData     bool
-}

pkg/config/config.go

@@ -971,11 +971,13 @@ func (c *ClusterComplexName) SetDefaultType() {
 func (c *ClusterComplexName) String() string {
 	return fmt.Sprintf("clusterName:%s, clusterType:%s", c.clusterName, c.clusterType)
 }
-func ValidClusterType(ctype string) error {
+func ValidClusterType(clusterType string) error {
-	if ctype == Ephemeral || ctype == Target {
+	for _, validType := range AllClusterTypes {
-		return nil
+		if clusterType == validType {
+			return nil
+		}
 	}
-	return errors.New("Cluster Type must be specified. Valid values are :" + Ephemeral + " or " + Target + ".")
+	return fmt.Errorf("Cluster Type must be one of %v", AllClusterTypes)
 }
 
 /* ______________________________

pkg/config/options.go

@@ -0,0 +1,131 @@
+/*
+Copyright 2020 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package config
+
+// TODO(howell): Switch to strongly-typed errors
+
+import (
+	"errors"
+	"fmt"
+	"os"
+)
+
+type AuthInfoOptions struct {
+	Name              string
+	ClientCertificate string
+	ClientKey         string
+	Token             string
+	Username          string
+	Password          string
+	EmbedCertData     bool
+}
+
+type ContextOptions struct {
+	Name           string
+	ClusterType    string
+	CurrentContext bool
+	Cluster        string
+	AuthInfo       string
+	Manifest       string
+	Namespace      string
+}
+
+type ClusterOptions struct {
+	Name                  string
+	ClusterType           string
+	Server                string
+	InsecureSkipTLSVerify bool
+	CertificateAuthority  string
+	EmbedCAData           bool
+}
+
+func (o *AuthInfoOptions) Validate() error {
+	if o.Token != "" && (o.Username != "" || o.Password != "") {
+		return fmt.Errorf("you cannot specify more than one authentication method at the same time: --%v or --%v/--%v",
+			FlagBearerToken, FlagUsername, FlagPassword)
+	}
+
+	if !o.EmbedCertData {
+		return nil
+	}
+
+	if err := checkExists(FlagCertFile, o.ClientCertificate); err != nil {
+		return err
+	}
+
+	if err := checkExists(FlagKeyFile, o.ClientKey); err != nil {
+		return err
+	}
+
+	return nil
+}
+
+func (o *ContextOptions) Validate() error {
+	if o.Name == "" {
+		return errors.New("you must specify a non-empty context name")
+	}
+
+	// If the user simply wants to change the current context, no further validation is needed
+	if o.CurrentContext {
+		return nil
+	}
+
+	// If the cluster-type was specified, verify that it's valid
+	if o.ClusterType != "" {
+		if err := ValidClusterType(o.ClusterType); err != nil {
+			return err
+		}
+	}
+
+	// TODO Manifest, Cluster could be validated against the existing config maps
+	return nil
+}
+
+func (o *ClusterOptions) Validate() error {
+	if o.Name == "" {
+		return errors.New("you must specify a non-empty cluster name")
+	}
+
+	err := ValidClusterType(o.ClusterType)
+	if err != nil {
+		return err
+	}
+
+	if o.InsecureSkipTLSVerify && o.CertificateAuthority != "" {
+		return fmt.Errorf("you cannot specify a %s and %s mode at the same time", FlagCAFile, FlagInsecure)
+	}
+
+	if !o.EmbedCAData {
+		return nil
+	}
+
+	if err := checkExists(FlagCAFile, o.CertificateAuthority); err != nil {
+		return err
+	}
+
+	return nil
+}
+
+func checkExists(flagName, path string) error {
+	if path == "" {
+		return fmt.Errorf("you must specify a --%s to embed", flagName)
+	}
+	if _, err := os.Stat(path); err != nil {
+		return fmt.Errorf("could not read %s data from '%s': %v", flagName, path, err)
+	}
+	return nil
+}

pkg/config/options_test.go

@@ -0,0 +1,244 @@
+/*
+Copyright 2020 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package config_test
+
+import (
+	"io/ioutil"
+	"os"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	"opendev.org/airship/airshipctl/pkg/config"
+)
+
+func TestAuthInfoOptionsValidate(t *testing.T) {
+	aRealFile, err := ioutil.TempFile("", "a-real-file")
+	require.NoError(t, err)
+
+	aRealFilename := aRealFile.Name()
+	defer os.Remove(aRealFilename)
+
+	tests := []struct {
+		name        string
+		testOptions config.AuthInfoOptions
+		expectError bool
+	}{
+		{
+			name: "TokenAndUserPass",
+			testOptions: config.AuthInfoOptions{
+				Token:    "testToken",
+				Username: "testUser",
+				Password: "testPassword",
+			},
+			expectError: true,
+		},
+		{
+			name: "DontEmbed",
+			testOptions: config.AuthInfoOptions{
+				EmbedCertData: false,
+			},
+			expectError: false,
+		},
+		{
+			name: "EmbedWithoutCert",
+			testOptions: config.AuthInfoOptions{
+				EmbedCertData: true,
+			},
+			expectError: true,
+		},
+		{
+			name: "EmbedWithoutClientKey",
+			testOptions: config.AuthInfoOptions{
+				EmbedCertData:     true,
+				ClientCertificate: aRealFilename,
+			},
+			expectError: true,
+		},
+		{
+			name: "EmbedWithCertAndClientKey",
+			testOptions: config.AuthInfoOptions{
+				EmbedCertData:     true,
+				ClientCertificate: aRealFilename,
+				ClientKey:         aRealFilename,
+			},
+			expectError: false,
+		},
+	}
+
+	for _, tt := range tests {
+		tt := tt
+		t.Run(tt.name, func(subTest *testing.T) {
+			err := tt.testOptions.Validate()
+			if tt.expectError {
+				assert.Error(t, err)
+			} else {
+				assert.NoError(t, err)
+			}
+		})
+	}
+}
+
+func TestContextOptionsValidate(t *testing.T) {
+	tests := []struct {
+		name        string
+		testOptions config.ContextOptions
+		expectError bool
+	}{
+		{
+			name: "MissingName",
+			testOptions: config.ContextOptions{
+				Name: "",
+			},
+			expectError: true,
+		},
+		{
+			name: "InvalidClusterType",
+			testOptions: config.ContextOptions{
+				Name:        "testContext",
+				ClusterType: "badType",
+			},
+			expectError: true,
+		},
+		{
+			name: "SettingCurrentContext",
+			testOptions: config.ContextOptions{
+				Name:           "testContext",
+				CurrentContext: true,
+			},
+			expectError: false,
+		},
+		{
+			name: "NoClusterType",
+			testOptions: config.ContextOptions{
+				Name: "testContext",
+			},
+			expectError: false,
+		},
+		{
+			name: "ValidClusterType",
+			testOptions: config.ContextOptions{
+				Name:        "testContext",
+				ClusterType: "target",
+			},
+			expectError: false,
+		},
+	}
+
+	for _, tt := range tests {
+		tt := tt
+		t.Run(tt.name, func(subTest *testing.T) {
+			err := tt.testOptions.Validate()
+			if tt.expectError {
+				assert.Error(t, err)
+			} else {
+				assert.NoError(t, err)
+			}
+		})
+	}
+}
+
+func TestClusterOptionsValidate(t *testing.T) {
+	aRealfile, err := ioutil.TempFile("", "a-real-file")
+	require.NoError(t, err)
+
+	aRealFilename := aRealfile.Name()
+	defer os.Remove(aRealFilename)
+
+	tests := []struct {
+		name        string
+		testOptions config.ClusterOptions
+		expectError bool
+	}{
+		{
+			name: "MissingName",
+			testOptions: config.ClusterOptions{
+				Name: "",
+			},
+			expectError: true,
+		},
+		{
+			name: "InvalidClusterType",
+			testOptions: config.ClusterOptions{
+				Name:        "testCluster",
+				ClusterType: "badType",
+			},
+			expectError: true,
+		},
+		{
+			name: "InsecureSkipTLSVerifyAndCertificateAuthority",
+			testOptions: config.ClusterOptions{
+				Name:                  "testCluster",
+				ClusterType:           "target",
+				InsecureSkipTLSVerify: true,
+				CertificateAuthority:  "cert_file",
+			},
+			expectError: true,
+		},
+		{
+			name: "DontEmbed",
+			testOptions: config.ClusterOptions{
+				Name:        "testCluster",
+				ClusterType: "target",
+				EmbedCAData: false,
+			},
+			expectError: false,
+		},
+		{
+			name: "EmbedWithoutCA",
+			testOptions: config.ClusterOptions{
+				Name:        "testCluster",
+				ClusterType: "target",
+				EmbedCAData: true,
+			},
+			expectError: true,
+		},
+		{
+			name: "EmbedWithFaultyCA",
+			testOptions: config.ClusterOptions{
+				Name:                 "testCluster",
+				ClusterType:          "target",
+				EmbedCAData:          true,
+				CertificateAuthority: "not-a-real-file",
+			},
+			expectError: true,
+		},
+		{
+			name: "EmbedWithGoodCA",
+			testOptions: config.ClusterOptions{
+				Name:                 "testCluster",
+				ClusterType:          "target",
+				EmbedCAData:          true,
+				CertificateAuthority: aRealFilename,
+			},
+			expectError: false,
+		},
+	}
+
+	for _, tt := range tests {
+		tt := tt
+		t.Run(tt.name, func(subTest *testing.T) {
+			err := tt.testOptions.Validate()
+			if tt.expectError {
+				assert.Error(t, err)
+			} else {
+				assert.NoError(t, err)
+			}
+		})
+	}
+}