diff --git a/CHANGELOG.md b/CHANGELOG.md index 5983d562c8..66bf5617e9 100644 --- a/CHANGELOG.md +++ b/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] diff --git a/cmd/influxd/upgrade/upgrade.go b/cmd/influxd/upgrade/upgrade.go index 85ca61d6b3..59313a008a 100644 --- a/cmd/influxd/upgrade/upgrade.go +++ b/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, - Flag: "username", - Default: "", - Desc: "primary username", - Short: 'u', - Required: true, + DestP: &options.target.userName, + Flag: "username", + Default: "", + Desc: "primary username", + Short: 'u', }, { - DestP: &options.target.password, - Flag: "password", - Default: "", - Desc: "password for username", - Short: 'p', - Required: true, + DestP: &options.target.password, + Flag: "password", + Default: "", + Desc: "password for username", + Short: 'p', }, { - DestP: &options.target.orgName, - Flag: "org", - Default: "", - Desc: "primary organization name", - Short: 'o', - Required: true, + DestP: &options.target.orgName, + Flag: "org", + Default: "", + Desc: "primary organization name", + Short: 'o', }, { - DestP: &options.target.bucket, - Flag: "bucket", - Default: "", - Desc: "primary bucket name", - Short: 'b', - Required: true, + DestP: &options.target.bucket, + Flag: "bucket", + Default: "", + Desc: "primary bucket name", + Short: 'b', }, { DestP: &options.target.retention, diff --git a/kit/cli/viper.go b/kit/cli/viper.go index 5656896128..d776aed646 100644 --- a/kit/cli/viper.go +++ b/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() } - - if o.Required { - cmd.MarkFlagRequired(o.Flag) - } - - envVar := o.Flag - if o.EnvVar != "" { - envVar = o.EnvVar - } - + envVal := lookupEnv(v, &o) 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 != "" { - _ = (*destP).DecodeFromString(v.GetString(envVar)) + if envVal != nil { + 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 != "" { - _ = (*destP).Set(v.GetString(envVar)) + if envVal != nil { + 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 - // hidden. This is in contrast to the MarkRequired, which can be set before... + // N.B. these "Mark" calls must run after the block above, + // 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) +} diff --git a/kit/cli/viper_test.go b/kit/cli/viper_test.go index c32f77265f..e0db27fdd7 100644 --- a/kit/cli/viper_test.go +++ b/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()) +}