feature request: footgun-proofing --some-boolean-flag false

[zackelan]

minimal repro code:

package main

import (
    "fmt"

    "github.com/spf13/cobra"
)

var (
    name    string
    count   int
    enabled bool
)

var cmd = &cobra.Command{
    Use: "test",
    RunE: func(cmd *cobra.Command, args []string) error {
        fmt.Printf("name: %v\n", name)
        fmt.Printf("count: %v\n", count)
        fmt.Printf("enabled: %v\n", enabled)
        fmt.Printf("args: %v\n", args)

        return nil
    },
}

func init() {
    flags := cmd.Flags()
    flags.StringVar(&name, "name", "", "name of the thing")
    flags.IntVar(&count, "count", 0, "count of the thing")
    flags.BoolVar(&enabled, "enabled", true, "whether to enable the thing")

    cmd.MarkFlagRequired("name")
    cmd.MarkFlagRequired("count")
    cmd.MarkFlagRequired("enabled")
}

func main() {
    cmd.Execute()
}

running it:

$ ./test --name=foo --count=10 --enabled=false
name: foo
count: 10
enabled: false
args: []
$ ./test --name foo --count 10 --enabled false
name: foo
count: 10
enabled: true
args: [false]

I understand the reason it behaves this way...but I also think it's counter-intuitive that (AFAIK) every other flag type doesn't care about --flag=value or --flag value, and booleans are the one exception to this.

and I'm definitely not the only person who's been tripped up by this:

#288

spf13/cobra#613

spf13/cobra#1230

spf13/cobra#1606

spf13/cobra#1657

for the purposes of this issue, I'm asking if there's a way to make this less of a footgun. in other words, if --enabled false is the incorrect thing for the user to enter, how can we present them with an error telling them it's wrong? rather than silently plowing ahead and possibly doing the opposite of what they asked for? or, can we make --enabled false work as the user is expecting it to?

the motivation for this is that my company had a Cobra command for "add new feature flag to backend system" with --name and --default-value flags. I'm sure there are lots of use cases where having a flag value be true when the user meant for it to be false isn't a huge deal (--verbose etc). but in our case, having a feature flag that silently used true instead of false as its default value could be Real Bad.

a few possible workarounds i can think of:

A) add cobra.ExactArgs(0) to the command

@@ -14,6 +14,7 @@

 var cmd = &cobra.Command{
        Use: "test",
+       Args: cobra.ExactArgs(0),
        RunE: func(cmd *cobra.Command, args []string) error {
                fmt.Printf("name: %v\n", name)
                fmt.Printf("count: %v\n", count)

this results in an error if --enabled false is passed, but with a message that doesn't make it obvious what the user should do to fix the problem:

$ ./test --name foo --count 10 --enabled false
Error: accepts 0 arg(s), received 1
Usage:
  test [flags]

Flags:
      --count int     count of the thing
      --enabled       whether to enable the thing (default true)
  -h, --help          help for test
      --name string   name of the thing

B) explicitly check the length of the args

@@ -15,6 +15,10 @@
 var cmd = &cobra.Command{
        Use: "test",
        RunE: func(cmd *cobra.Command, args []string) error {
+               if len(args) > 0 {
+                       return fmt.Errorf("not expecting any args - make sure you're using --enabled=value and not --enabled value")
+               }
+
                fmt.Printf("name: %v\n", name)
                fmt.Printf("count: %v\n", count)
                fmt.Printf("enabled: %v\n", enabled)

this is similar to A, but at least allows printing a custom error message that can hint at the problem.

however, both A and B don't really work if the command is expecting a non-zero number of arguments under normal circumstances.

C) make the argument a bool slice

@@ -9,7 +9,7 @@
 var (
        name    string
        count   int
-       enabled bool
+       enabled []bool
 )

 var cmd = &cobra.Command{
@@ -28,7 +28,7 @@
        flags := cmd.Flags()
        flags.StringVar(&name, "name", "", "name of the thing")
        flags.IntVar(&count, "count", 0, "count of the thing")
-       flags.BoolVar(&enabled, "enabled", true, "whether to enable the thing")
+       flags.BoolSliceVar(&enabled, "enabled", []bool{true}, "whether to enable the thing")

        cmd.MarkFlagRequired("name")
        cmd.MarkFlagRequired("count")

this works, mostly:

$ ./test --name foo --count 10 --enabled false
name: foo
count: 10
enabled: [false]
args: []

but it causes the usage text to inaccurately suggest that multiple values are supported: (--enabled bools whether to enable the thing (default [false]))

I would also have to add in my own error-checking that the slice contains exactly one element.

D) make the argument a string, and then convert it myself

@@ -2,6 +2,7 @@

 import (
        "fmt"
+       "strconv"

        "github.com/spf13/cobra"
 )
@@ -9,15 +10,20 @@
 var (
        name    string
        count   int
-       enabled bool
+       enabled string
 )

 var cmd = &cobra.Command{
        Use: "test",
        RunE: func(cmd *cobra.Command, args []string) error {
+               enabledBool, err := strconv.ParseBool(enabled)
+               if err != nil {
+                       return err
+               }
+
                fmt.Printf("name: %v\n", name)
                fmt.Printf("count: %v\n", count)
-               fmt.Printf("enabled: %v\n", enabled)
+               fmt.Printf("enabled: %v\n", enabledBool)
                fmt.Printf("args: %v\n", args)

                return nil
@@ -28,7 +34,7 @@
        flags := cmd.Flags()
        flags.StringVar(&name, "name", "", "name of the thing")
        flags.IntVar(&count, "count", 0, "count of the thing")
-       flags.BoolVar(&enabled, "enabled", true, "whether to enable the thing")
+       flags.StringVar(&enabled, "enabled", "true", "whether to enable the thing")

        cmd.MarkFlagRequired("name")
        cmd.MarkFlagRequired("count")

this also works:

$ ./test --name foo --count 10 --enabled false
name: foo
count: 10
enabled: false
args: []

with the downside that you lose the BoolVar functionality of being able to specify --enabled on its own without a value (but at least with a helpful error message):

$ ./test --name foo --count 10 --enabled
Error: flag needs an argument: --enabled
Usage:
  test [flags]

Flags:
      --count int        count of the thing
      --enabled string   whether to enable the thing (default "true")
  -h, --help             help for test
      --name string      name of the thing

of these, D seems like the least-bad option...but it means that the workaround in these cases is just "don't use BoolVar, and instead reimplement its functionality in your command"

rather than any of these workarounds, I think a possible improvement to this library might be adding something like an ExplicitBoolVar function? that would behave like BoolVar, except it would consume a value the way that StringVar etc do.

having BoolVar and ExplicitBoolVar show up in the list of available functions might also help discoverability of this problem, because people will go looking to see how they're different, which could lead to a section in the documentation explaining this possible footgun.

[alimpfard]

imo the current system is fine, provided that it gets displayed in the help output as --enabled[=bool], most people are familiar with this scheme AFAIK; whereas --enabled false is a bit more obscure (again, imo).

The change is fairly trivial (basically just changing

pflag/flag.go

Lines 710 to 713 in d5e0c06

	case "bool":
	if flag.NoOptDefVal != "true" {
	line += fmt.Sprintf("[=%s]", flag.NoOptDefVal)
	}

a bit), and a user even glancing at the help output would know what forms of passing values are allowed.

Explicitly disallowing --enabled false seems like an unnecessary annoyance, someone actually meaning that would have to do --enabled -- false just to get around the parser being "nice".