urfave / cli

A simple, fast, and fun package for building command line apps in Go
https://cli.urfave.org
MIT License
21.89k stars 1.69k forks source link

Is the "cant duplicate this flag" error (v3) makes sense? #1926

Closed tarampampam closed 1 day ago

tarampampam commented 1 week ago

Hi there, team of the best CLI package 😄

I have a question that might be a bit silly, but I was confused when I encountered the error can't duplicate this flag while using only one flag with the OnlyOnce: true property for a cli.Command.

https://github.com/urfave/cli/blob/c4cd0a51cc113a90eca318cc8f7d328f05a5f2b7/flag_impl.go#L166-L168

Is this a mistake or not? If not, what is the purpose of this guard? Could someone explain it to me?

tarampampam commented 1 week ago

Ouch, I could provide a bit more context on how exactly I encountered this error.

Usually, if the app I develop has more than one sub-command that requires the same flags, I prefer to define every flag that I could reuse in a separate package named shared:

package shared

import (
    // ...
    "github.com/urfave/cli/v3"
)

var ListenPortFlag = &cli.UintFlag{
    Name:     "port",
    Aliases:  []string{"p"},
    Usage:    "TCP port number",
    Value:    8080, // default port number
    Sources:  cli.EnvVars("LISTEN_PORT"),
    OnlyOnce: true,
    Required: true,
    Validator: func(port uint64) error {
        if port == 0 || port > 65535 {
            return fmt.Errorf("invalid TCP port number [%d]", port)
        }
        return nil
    },
}

// ...

And use it in my commands in this manner:

import (
    // ...
    "github.com/urfave/cli/v3"
)

func NewCommand() *cli.Command {
    return &cli.Command{
        Name:    "some_command",
        Aliases: []string{"some", "command"},
        Action: func(ctx context.Context, c *cli.Command) error {
            runtime.Gosched() // <-- important, can be replaced with something else

            return nil // some code goes here
        },
        Flags: []cli.Flag{
            shared.ListenPortFlag, // <-- important
        },
    }
}

Everything works fine until I write a unit test to test the command execution:

func TestCommand_RunSuccess(t *testing.T) {
    t.Parallel()

    var cmd = healthcheck.NewCommand()

    require.NoError(t, cmd.Run(context.Background(), []string{"", "--port", "1234"}))
}

func TestCommand_RunFail(t *testing.T) {
    t.Parallel()

    cmd := healthcheck.NewCommand()

    require.NoError(t, cmd.Run(context.Background(), []string{"", "--port", "4321"}))
}

If you run each test one by one (not together - that's important), each of them will pass. But, if you run them in parallel (go test ./...), the test will fail with the error:

=== RUN   TestCommand_RunFail
=== PAUSE TestCommand_RunFail
=== CONT  TestCommand_RunFail
    command_test.go:47: 
            Error Trace:    /path/to/command_test.go:666
            Error:          Received unexpected error:
                            invalid value "4321" for flag -port: cant duplicate this flag
            Test:           TestCommand_RunFail
--- FAIL: TestCommand_RunFail (0.00s)

To fix this, all I need to do is replace

// file: shared/flags.go

var ListenPortFlag = &cli.UintFlag{
//                   ^

with

// file: shared/flags.go

var ListenPortFlag = cli.UintFlag{

// file: command.go

func NewCommand() *cli.Command {
    var portFlag = shared.ListenPortFlag // make a copy
//      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

    return &cli.Command{
        Name:    "some_command",
        Aliases: []string{"some", "command"},
        Action: func(ctx context.Context, c *cli.Command) error {
            return nil // some code goes here
        },
        Flags: []cli.Flag{
            &portFlag, // use the copy
//                      ^^^^^^^^^
        },
    }
}

This was so strange that I decided to create this issue to highlight this workaround.

[!NOTE] TL;DR - flags have their own state, so don't reuse them using pointers.

dearchap commented 2 days ago

@tarampampam The OnlyOnce field is used to tell the cli library to expect this flag on the cmd line only once. So if it is specified twice , for example

--port 1234 --port 4566

then you will get that error. This is to protect users from themselves. Without this option in above example port would be set to 4566