diff --git a/pkg/kn/core/root.go b/pkg/kn/core/root.go index d19dea4926..5e7ab0dd7c 100644 --- a/pkg/kn/core/root.go +++ b/pkg/kn/core/root.go @@ -29,6 +29,7 @@ import ( "github.com/knative/client/pkg/kn/commands/revision" "github.com/knative/client/pkg/kn/commands/route" "github.com/knative/client/pkg/kn/commands/service" + "github.com/knative/client/pkg/kn/flags" homedir "github.com/mitchellh/go-homedir" "github.com/spf13/cobra" "github.com/spf13/viper" @@ -115,6 +116,10 @@ func NewKnCommand(params ...commands.KnParams) *cobra.Command { // Prevents Cobra from dealing with errors as we deal with them in main.go SilenceErrors: true, + + PersistentPreRunE: func(cmd *cobra.Command, args []string) error { + return flags.ReconcileBoolFlags(cmd.Flags()) + }, } if p.Output != nil { rootCmd.SetOutput(p.Output) @@ -123,7 +128,7 @@ func NewKnCommand(params ...commands.KnParams) *cobra.Command { // Persistent flags rootCmd.PersistentFlags().StringVar(&commands.CfgFile, "config", "", "kn config file (default is $HOME/.kn/config.yaml)") rootCmd.PersistentFlags().StringVar(&p.KubeCfgPath, "kubeconfig", "", "kubectl config file (default is $HOME/.kube/config)") - rootCmd.PersistentFlags().BoolVar(&p.LogHttp, "log-http", false, "log http traffic") + flags.AddBothBoolFlags(rootCmd.PersistentFlags(), &p.LogHttp, "log-http", "", false, "log http traffic") plugin.AddPluginFlags(rootCmd) plugin.BindPluginsFlagToViper(rootCmd) diff --git a/pkg/kn/flags/bool.go b/pkg/kn/flags/bool.go new file mode 100644 index 0000000000..d04c24609d --- /dev/null +++ b/pkg/kn/flags/bool.go @@ -0,0 +1,104 @@ +// Copyright © 2019 The Knative 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 flags + +import ( + "fmt" + "strconv" + "strings" + + "github.com/spf13/pflag" +) + +var negPrefix = "no-" + +// AddBothBoolFlags adds the given flag in both `--foo` and `--no-foo` variants. +// If you do this, make sure you call ReconcileBoolFlags later to catch errors and +// set the relationship between the flag values. +func AddBothBoolFlags(f *pflag.FlagSet, p *bool, name, short string, value bool, usage string) { + + negativeName := negPrefix + name + + f.BoolVarP(p, name, short, value, usage) + f.Bool(negativeName, !value, "do not "+usage) + + if value { + err := f.MarkHidden(name) + if err != nil { + panic(err) + } + } else { + err := f.MarkHidden(negativeName) + if err != nil { + panic(err) + } + } +} + +// ReconcileBoolFlags sets the value of the all the "--foo" flags based on +// "--no-foo" if provided, and returns an error if both were provided or an +// explicit value of false was provided to either (as that's confusing). +func ReconcileBoolFlags(f *pflag.FlagSet) error { + var err error + f.VisitAll(func(flag *pflag.Flag) { + // Return early from our comprehension + if err != nil { + return + } + // Walk the "no-" versions of the flags. Make sure we didn't set + // both, and set the positive value to the opposite of the "no-" + // value if it exists. + if strings.HasPrefix(flag.Name, "no-") { + positiveName := flag.Name[len(negPrefix):] + positive := f.Lookup(positiveName) + if flag.Changed { + if positive.Changed { + err = fmt.Errorf("only one of --%s and --%s may be specified", + flag.Name, positiveName) + return + } + var noValue bool + noValue, err = strconv.ParseBool(flag.Value.String()) + if err != nil { + return + } + if !noValue { + err = fmt.Errorf("use --%s instead of providing false to --%s", + positiveName, flag.Name) + if err != nil { + return + } + } + err = positive.Value.Set(strconv.FormatBool(!noValue)) + } else if positive.Changed { + // For the positive version, just check it wasn't set to the + // confusing "false" value. + var yesValue bool + yesValue, err = strconv.ParseBool(positive.Value.String()) + if err != nil { + return + } + if !yesValue { + err = fmt.Errorf("use --%s instead of providing false to --%s", + flag.Name, positiveName) + if err != nil { + return + } + } + } + } + }) + return err +} diff --git a/pkg/kn/flags/bool_test.go b/pkg/kn/flags/bool_test.go new file mode 100644 index 0000000000..affcfa157b --- /dev/null +++ b/pkg/kn/flags/bool_test.go @@ -0,0 +1,71 @@ +// Copyright © 2019 The Knative 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 flags + +import ( + "testing" + + "github.com/spf13/pflag" + "gotest.tools/assert" +) + +type boolPairTestCase struct { + name string + defaultVal bool + flags []string + expectedResult bool + expectedErrText string +} + +func TestBooleanPair(t *testing.T) { + cases := []*boolPairTestCase{ + {"foo", true, []string{}, true, ""}, + {"foo", true, []string{"--foo"}, true, ""}, + {"foo", true, []string{"--no-foo"}, false, ""}, + {"foo", false, []string{"--foo"}, true, ""}, + {"foo", false, []string{}, false, ""}, + {"foo", false, []string{"--no-foo"}, false, ""}, + {"foo", true, []string{"--foo", "--no-foo"}, false, "only one of"}, + {"foo", true, []string{"--no-foo", "--foo"}, false, "only one of"}, + // Disallow confusing "false" value. + {"foo", true, []string{"--foo=false"}, false, "use --no-foo instead of providing false to --foo"}, + {"foo", true, []string{"--no-foo=false"}, false, "use --foo instead of providing false to --no-foo"}, + + // Ensure tests still pass if positive sorts after no- alphabetically. + {"zoo", true, []string{}, true, ""}, + {"zoo", true, []string{"--zoo"}, true, ""}, + {"zoo", true, []string{"--no-zoo"}, false, ""}, + {"zoo", false, []string{"--zoo"}, true, ""}, + {"zoo", false, []string{}, false, ""}, + {"zoo", false, []string{"--no-zoo"}, false, ""}, + {"zoo", true, []string{"--zoo", "--no-zoo"}, false, "only one of"}, + {"zoo", true, []string{"--no-zoo", "--zoo"}, false, "only one of"}, + // Disallow confusing "false" value. + {"zoo", true, []string{"--zoo=false"}, false, "use --no-zoo instead of providing false to --zoo"}, + {"zoo", true, []string{"--no-zoo=false"}, false, "use --zoo instead of providing false to --no-zoo"}, + } + for _, c := range cases { + f := &pflag.FlagSet{} + var result bool + AddBothBoolFlags(f, &result, c.name, "", c.defaultVal, "set "+c.name) + f.Parse(c.flags) + err := ReconcileBoolFlags(f) + if c.expectedErrText != "" { + assert.ErrorContains(t, err, c.expectedErrText) + } else { + assert.Equal(t, result, c.expectedResult) + } + } +}