fix(kit/cli): don't ignore failures to mark CLI options as required (#20490)

CHANGELOG.md

@@ -36,6 +36,7 @@ Replacement `tsi1` indexes will be automatically generated on startup for shards
 1. [20440](https://github.com/influxdata/influxdb/pull/20440): Add confirmation step w/ file sizes before copying data files in `influxd upgrade`.
 1. [20409](https://github.com/influxdata/influxdb/pull/20409): Improve messages in DBRP API validation errors.
 1. [20489](https://github.com/influxdata/influxdb/pull/20489): Improve error message when opening BoltDB with unsupported file system options.
+1. [20490](https://github.com/influxdata/influxdb/pull/20490): Fix silent failure to register CLI args as required.
 
 ## v2.0.3 [2020-12-14]
 

cmd/influxd/upgrade/upgrade.go

@@ -193,36 +193,32 @@ func NewCommand(v *viper.Viper) *cobra.Command {
 			Desc:    "path for exported 1.x continuous queries",
 		},
 		{
-			DestP:    &options.target.userName,
+			DestP:   &options.target.userName,
-			Flag:     "username",
+			Flag:    "username",
-			Default:  "",
+			Default: "",
-			Desc:     "primary username",
+			Desc:    "primary username",
-			Short:    'u',
+			Short:   'u',
-			Required: true,
 		},
 		{
-			DestP:    &options.target.password,
+			DestP:   &options.target.password,
-			Flag:     "password",
+			Flag:    "password",
-			Default:  "",
+			Default: "",
-			Desc:     "password for username",
+			Desc:    "password for username",
-			Short:    'p',
+			Short:   'p',
-			Required: true,
 		},
 		{
-			DestP:    &options.target.orgName,
+			DestP:   &options.target.orgName,
-			Flag:     "org",
+			Flag:    "org",
-			Default:  "",
+			Default: "",
-			Desc:     "primary organization name",
+			Desc:    "primary organization name",
-			Short:    'o',
+			Short:   'o',
-			Required: true,
 		},
 		{
-			DestP:    &options.target.bucket,
+			DestP:   &options.target.bucket,
-			Flag:     "bucket",
+			Flag:    "bucket",
-			Default:  "",
+			Default: "",
-			Desc:     "primary bucket name",
+			Desc:    "primary bucket name",
-			Short:    'b',
+			Short:   'b',
-			Required: true,
 		},
 		{
 			DestP:   &options.target.retention,

kit/cli/viper.go

@@ -8,6 +8,7 @@ import (
 	"time"
 
 	"github.com/influxdata/influxdb/v2"
+	"github.com/spf13/cast"
 	"github.com/spf13/cobra"
 	"github.com/spf13/pflag"
 	"github.com/spf13/viper"
@@ -29,16 +30,6 @@ type Opt struct {
 	Desc    string
 }
 
-// NewOpt creates a new command line option.
-func NewOpt(destP interface{}, flag string, dflt interface{}, desc string) Opt {
-	return Opt{
-		DestP:   destP,
-		Flag:    flag,
-		Default: dflt,
-		Desc:    desc,
-	}
-}
-
 // Program parses CLI options
 type Program struct {
 	// Run is invoked by cobra on execute.
@@ -114,16 +105,7 @@ func BindOptions(v *viper.Viper, cmd *cobra.Command, opts []Opt) {
 		if o.Persistent {
 			flagset = cmd.PersistentFlags()
 		}
-
+		envVal := lookupEnv(v, &o)
-		if o.Required {
-			cmd.MarkFlagRequired(o.Flag)
-		}
-
-		envVar := o.Flag
-		if o.EnvVar != "" {
-			envVar = o.EnvVar
-		}
-
 		hasShort := o.Short != 0
 
 		switch destP := o.DestP.(type) {
@@ -138,7 +120,12 @@ func BindOptions(v *viper.Viper, cmd *cobra.Command, opts []Opt) {
 				flagset.StringVar(destP, o.Flag, d, o.Desc)
 			}
 			mustBindPFlag(v, o.Flag, flagset)
-			*destP = v.GetString(envVar)
+			if envVal != nil {
+				if s, err := cast.ToStringE(envVal); err == nil {
+					*destP = s
+				}
+			}
+
 		case *int:
 			var d int
 			if o.Default != nil {
@@ -150,7 +137,12 @@ func BindOptions(v *viper.Viper, cmd *cobra.Command, opts []Opt) {
 				flagset.IntVar(destP, o.Flag, d, o.Desc)
 			}
 			mustBindPFlag(v, o.Flag, flagset)
-			*destP = v.GetInt(envVar)
+			if envVal != nil {
+				if i, err := cast.ToIntE(envVal); err == nil {
+					*destP = i
+				}
+			}
+
 		case *int32:
 			var d int32
 			if o.Default != nil {
@@ -177,7 +169,12 @@ func BindOptions(v *viper.Viper, cmd *cobra.Command, opts []Opt) {
 				flagset.Int32Var(destP, o.Flag, d, o.Desc)
 			}
 			mustBindPFlag(v, o.Flag, flagset)
-			*destP = v.GetInt32(envVar)
+			if envVal != nil {
+				if i, err := cast.ToInt32E(envVal); err == nil {
+					*destP = i
+				}
+			}
+
 		case *int64:
 			var d int64
 			if o.Default != nil {
@@ -202,7 +199,12 @@ func BindOptions(v *viper.Viper, cmd *cobra.Command, opts []Opt) {
 				flagset.Int64Var(destP, o.Flag, d, o.Desc)
 			}
 			mustBindPFlag(v, o.Flag, flagset)
-			*destP = v.GetInt64(envVar)
+			if envVal != nil {
+				if i, err := cast.ToInt64E(envVal); err == nil {
+					*destP = i
+				}
+			}
+
 		case *bool:
 			var d bool
 			if o.Default != nil {
@@ -214,7 +216,12 @@ func BindOptions(v *viper.Viper, cmd *cobra.Command, opts []Opt) {
 				flagset.BoolVar(destP, o.Flag, d, o.Desc)
 			}
 			mustBindPFlag(v, o.Flag, flagset)
-			*destP = v.GetBool(envVar)
+			if envVal != nil {
+				if b, err := cast.ToBoolE(envVal); err == nil {
+					*destP = b
+				}
+			}
+
 		case *time.Duration:
 			var d time.Duration
 			if o.Default != nil {
@@ -226,7 +233,12 @@ func BindOptions(v *viper.Viper, cmd *cobra.Command, opts []Opt) {
 				flagset.DurationVar(destP, o.Flag, d, o.Desc)
 			}
 			mustBindPFlag(v, o.Flag, flagset)
-			*destP = v.GetDuration(envVar)
+			if envVal != nil {
+				if d, err := cast.ToDurationE(envVal); err == nil {
+					*destP = d
+				}
+			}
+
 		case *[]string:
 			var d []string
 			if o.Default != nil {
@@ -238,7 +250,12 @@ func BindOptions(v *viper.Viper, cmd *cobra.Command, opts []Opt) {
 				flagset.StringSliceVar(destP, o.Flag, d, o.Desc)
 			}
 			mustBindPFlag(v, o.Flag, flagset)
-			*destP = v.GetStringSlice(envVar)
+			if envVal != nil {
+				if ss, err := cast.ToStringSliceE(envVal); err == nil {
+					*destP = ss
+				}
+			}
+
 		case *map[string]string:
 			var d map[string]string
 			if o.Default != nil {
@@ -250,7 +267,12 @@ func BindOptions(v *viper.Viper, cmd *cobra.Command, opts []Opt) {
 				flagset.StringToStringVar(destP, o.Flag, d, o.Desc)
 			}
 			mustBindPFlag(v, o.Flag, flagset)
-			*destP = v.GetStringMapString(envVar)
+			if envVal != nil {
+				if sms, err := cast.ToStringMapStringE(envVal); err == nil {
+					*destP = sms
+				}
+			}
+
 		case pflag.Value:
 			if hasShort {
 				flagset.VarP(destP, o.Flag, string(o.Short), o.Desc)
@@ -258,10 +280,15 @@ func BindOptions(v *viper.Viper, cmd *cobra.Command, opts []Opt) {
 				flagset.Var(destP, o.Flag, o.Desc)
 			}
 			if o.Default != nil {
-				destP.Set(o.Default.(string))
+				_ = destP.Set(o.Default.(string))
 			}
 			mustBindPFlag(v, o.Flag, flagset)
-			destP.Set(v.GetString(envVar))
+			if envVal != nil {
+				if s, err := cast.ToStringE(envVal); err == nil {
+					_ = destP.Set(s)
+				}
+			}
+
 		case *influxdb.ID:
 			var d influxdb.ID
 			if o.Default != nil {
@@ -272,9 +299,12 @@ func BindOptions(v *viper.Viper, cmd *cobra.Command, opts []Opt) {
 			} else {
 				IDVar(flagset, destP, o.Flag, d, o.Desc)
 			}
-			if s := v.GetString(envVar); s != "" {
+			if envVal != nil {
-				_ = (*destP).DecodeFromString(v.GetString(envVar))
+				if s, err := cast.ToStringE(envVal); err == nil {
+					_ = (*destP).DecodeFromString(s)
+				}
 			}
+
 		case *zapcore.Level:
 			var l zapcore.Level
 			if o.Default != nil {
@@ -285,19 +315,33 @@ func BindOptions(v *viper.Viper, cmd *cobra.Command, opts []Opt) {
 			} else {
 				LevelVar(flagset, destP, o.Flag, l, o.Desc)
 			}
-			if s := v.GetString(envVar); s != "" {
+			if envVal != nil {
-				_ = (*destP).Set(v.GetString(envVar))
+				if s, err := cast.ToStringE(envVal); err == nil {
+					_ = (*destP).Set(s)
+				}
 			}
+
 		default:
 			// if you get a panic here, sorry about that!
 			// anyway, go ahead and make a PR and add another type.
 			panic(fmt.Errorf("unknown destination type %t", o.DestP))
 		}
 
-		// so weirdness with the flagset her, the flag must be set before marking it
+		// N.B. these "Mark" calls must run after the block above,
-		// hidden. This is in contrast to the MarkRequired, which can be set before...
+		// otherwise cobra will return a "no such flag" error.
+
+		// Cobra will complain if a flag marked as required isn't present on the CLI.
+		// To support setting required args via config and env variables, we only enforce
+		// the required check if we didn't find a value in the viper instance.
+		if o.Required && envVal == nil {
+			if err := cmd.MarkFlagRequired(o.Flag); err != nil {
+				panic(fmt.Errorf("failed to mark flag %q as required: %w", o.Flag, err))
+			}
+		}
 		if o.Hidden {
-			flagset.MarkHidden(o.Flag)
+			if err := flagset.MarkHidden(o.Flag); err != nil {
+				panic(fmt.Errorf("failed to mark flag %q as hidden: %w", o.Flag, err))
+			}
 		}
 	}
 }
@@ -307,3 +351,12 @@ func mustBindPFlag(v *viper.Viper, key string, flagset *pflag.FlagSet) {
 		panic(err)
 	}
 }
+
+// lookupEnv returns the value for a CLI option found in the environment, if any.
+func lookupEnv(v *viper.Viper, o *Opt) interface{} {
+	envVar := o.Flag
+	if o.EnvVar != "" {
+		envVar = o.EnvVar
+	}
+	return v.Get(envVar)
+}

kit/cli/viper_test.go

@@ -121,6 +121,7 @@ func ExampleNewCommand() {
 		},
 	})
 
+	cmd.SetArgs([]string{})
 	if err := cmd.Execute(); err != nil {
 		fmt.Fprintln(os.Stderr, err)
 	}
@@ -254,3 +255,23 @@ func newConfigFile(t *testing.T, config interface{}) (string, func()) {
 		os.RemoveAll(testDir)
 	}
 }
+
+func Test_RequiredFlag(t *testing.T) {
+	var testVar string
+	program := &Program{
+		Name: "test",
+		Opts: []Opt{
+			{
+				DestP:    &testVar,
+				Flag:     "foo",
+				Required: true,
+			},
+		},
+	}
+
+	cmd := NewCommand(viper.New(), program)
+	cmd.SetArgs([]string{})
+	err := cmd.Execute()
+	require.Error(t, err)
+	require.Equal(t, `required flag(s) "foo" not set`, err.Error())
+}