From 4e5f0af9cd302038f99c0abe3971ca5b063c3942 Mon Sep 17 00:00:00 2001 From: Ian Campbell Date: Fri, 1 Mar 2019 14:50:57 +0000 Subject: [PATCH 1/5] e2e: start a new file for cli plugins arguments tests `TestRunGoodArgument` fits here. Signed-off-by: Ian Campbell --- e2e/cli-plugins/flags_test.go | 19 +++++++++++++++++++ e2e/cli-plugins/run_test.go | 12 ------------ 2 files changed, 19 insertions(+), 12 deletions(-) create mode 100644 e2e/cli-plugins/flags_test.go diff --git a/e2e/cli-plugins/flags_test.go b/e2e/cli-plugins/flags_test.go new file mode 100644 index 0000000000..d472257b9e --- /dev/null +++ b/e2e/cli-plugins/flags_test.go @@ -0,0 +1,19 @@ +package cliplugins + +import ( + "testing" + + "gotest.tools/icmd" +) + +// TestRunGoodArgument ensures correct behaviour when running a valid plugin with an `--argument`. +func TestRunGoodArgument(t *testing.T) { + run, _, cleanup := prepare(t) + defer cleanup() + + res := icmd.RunCmd(run("helloworld", "--who", "Cleveland")) + res.Assert(t, icmd.Expected{ + ExitCode: 0, + Out: "Hello Cleveland!", + }) +} diff --git a/e2e/cli-plugins/run_test.go b/e2e/cli-plugins/run_test.go index 8e6b49ca15..171ed81c83 100644 --- a/e2e/cli-plugins/run_test.go +++ b/e2e/cli-plugins/run_test.go @@ -144,18 +144,6 @@ func TestRunGoodSubcommand(t *testing.T) { }) } -// TestRunGoodArgument ensures correct behaviour when running a valid plugin with an `--argument`. -func TestRunGoodArgument(t *testing.T) { - run, _, cleanup := prepare(t) - defer cleanup() - - res := icmd.RunCmd(run("helloworld", "--who", "Cleveland")) - res.Assert(t, icmd.Expected{ - ExitCode: 0, - Out: "Hello Cleveland!", - }) -} - // TestHelpGoodSubcommand ensures correct behaviour when invoking help on a // valid plugin subcommand. A global argument is included to ensure it does not // interfere. From 8289ae03f802f8d26b7291a80cc9b9544643a157 Mon Sep 17 00:00:00 2001 From: Ian Campbell Date: Fri, 1 Mar 2019 16:03:14 +0000 Subject: [PATCH 2/5] Add e2e tests for plugin vs global argument issues These won't pass right now due to https://github.com/docker/cli/issues/1699 ("Plugins can't re-use the same flags as cli global flags") and the change in 935d47bbe967 ("Ignore unknown arguments on the top-level command."), but the intention is to fix them now. Signed-off-by: Ian Campbell --- cli-plugins/examples/helloworld/main.go | 21 ++++- e2e/cli-plugins/flags_test.go | 81 +++++++++++++++++++ .../testdata/docker-help-helloworld.golden | 4 +- 3 files changed, 104 insertions(+), 2 deletions(-) diff --git a/cli-plugins/examples/helloworld/main.go b/cli-plugins/examples/helloworld/main.go index cfbae368b0..6251e85916 100644 --- a/cli-plugins/examples/helloworld/main.go +++ b/cli-plugins/examples/helloworld/main.go @@ -44,7 +44,10 @@ func main() { }, } - var who string + var ( + who, context string + debug bool + ) cmd := &cobra.Command{ Use: "helloworld", Short: "A basic Hello World plugin for tests", @@ -53,6 +56,18 @@ func main() { // hook. PersistentPreRunE: plugin.PersistentPreRunE, RunE: func(cmd *cobra.Command, args []string) error { + if debug { + fmt.Fprintf(dockerCli.Err(), "Plugin debug mode enabled") + } + + switch context { + case "Christmas": + fmt.Fprintf(dockerCli.Out(), "Merry Christmas!\n") + return nil + case "": + // nothing + } + if who == "" { who, _ = dockerCli.ConfigFile().PluginConfig("helloworld", "who") } @@ -68,6 +83,10 @@ func main() { flags := cmd.Flags() flags.StringVar(&who, "who", "", "Who are we addressing?") + // These are intended to deliberately clash with the CLIs own top + // level arguments. + flags.BoolVarP(&debug, "debug", "D", false, "Enable debug") + flags.StringVarP(&context, "context", "c", "", "Is it Christmas?") cmd.AddCommand(goodbye, apiversion, exitStatus2) return cmd diff --git a/e2e/cli-plugins/flags_test.go b/e2e/cli-plugins/flags_test.go index d472257b9e..4bf01b9c47 100644 --- a/e2e/cli-plugins/flags_test.go +++ b/e2e/cli-plugins/flags_test.go @@ -3,6 +3,8 @@ package cliplugins import ( "testing" + "gotest.tools/assert" + is "gotest.tools/assert/cmp" "gotest.tools/icmd" ) @@ -17,3 +19,82 @@ func TestRunGoodArgument(t *testing.T) { Out: "Hello Cleveland!", }) } + +// TestClashWithGlobalArgs ensures correct behaviour when a plugin +// has an argument with the same name as one of the globals. +func TestClashWithGlobalArgs(t *testing.T) { + run, _, cleanup := prepare(t) + defer cleanup() + + for _, tc := range []struct { + name string + args []string + expectedOut, expectedErr string + }{ + { + name: "short-without-val", + args: []string{"-D"}, + expectedOut: "Hello World!", + expectedErr: "Plugin debug mode enabled", + }, + { + name: "long-without-val", + args: []string{"--debug"}, + expectedOut: "Hello World!", + expectedErr: "Plugin debug mode enabled", + }, + { + name: "short-with-val", + args: []string{"-c", "Christmas"}, + expectedOut: "Merry Christmas!", + expectedErr: "", + }, + { + name: "short-with-val", + args: []string{"--context", "Christmas"}, + expectedOut: "Merry Christmas!", + expectedErr: "", + }, + } { + t.Run(tc.name, func(t *testing.T) { + args := append([]string{"helloworld"}, tc.args...) + res := icmd.RunCmd(run(args...)) + res.Assert(t, icmd.Expected{ + ExitCode: 0, + Out: tc.expectedOut, + Err: tc.expectedErr, + }) + }) + } +} + +// TestUnknownGlobal checks that unknown globals report errors +func TestUnknownGlobal(t *testing.T) { + run, _, cleanup := prepare(t) + defer cleanup() + + t.Run("no-val", func(t *testing.T) { + res := icmd.RunCmd(run("--unknown", "helloworld")) + res.Assert(t, icmd.Expected{ + ExitCode: 125, + }) + assert.Assert(t, is.Equal(res.Stdout(), "")) + assert.Assert(t, is.Contains(res.Stderr(), "unknown flag: --unknown")) + }) + t.Run("separate-val", func(t *testing.T) { + res := icmd.RunCmd(run("--unknown", "foo", "helloworld")) + res.Assert(t, icmd.Expected{ + ExitCode: 125, + }) + assert.Assert(t, is.Equal(res.Stdout(), "")) + assert.Assert(t, is.Contains(res.Stderr(), "unknown flag: --unknown")) + }) + t.Run("joined-val", func(t *testing.T) { + res := icmd.RunCmd(run("--unknown=foo", "helloworld")) + res.Assert(t, icmd.Expected{ + ExitCode: 125, + }) + assert.Assert(t, is.Equal(res.Stdout(), "")) + assert.Assert(t, is.Contains(res.Stderr(), "unknown flag: --unknown")) + }) +} diff --git a/e2e/cli-plugins/testdata/docker-help-helloworld.golden b/e2e/cli-plugins/testdata/docker-help-helloworld.golden index 59959637cb..24bc424bd7 100644 --- a/e2e/cli-plugins/testdata/docker-help-helloworld.golden +++ b/e2e/cli-plugins/testdata/docker-help-helloworld.golden @@ -4,7 +4,9 @@ Usage: docker helloworld [OPTIONS] COMMAND A basic Hello World plugin for tests Options: - --who string Who are we addressing? + -c, --context string Is it Christmas? + -D, --debug Enable debug + --who string Who are we addressing? Commands: apiversion Print the API version of the server From d4ced2ef77fccbda407fab2354250b90c7edf909 Mon Sep 17 00:00:00 2001 From: Ian Campbell Date: Wed, 6 Mar 2019 10:29:42 +0000 Subject: [PATCH 3/5] allow plugins to have argument which match a top-level flag. The issue with plugin options clashing with globals is that when cobra is parsing the command line and it comes across an argument which doesn't start with a `-` it (in the absence of plugins) distinguishes between "argument to current command" and "new subcommand" based on the list of registered sub commands. Plugins breaks that model. When presented with `docker -D plugin -c foo` cobra parses up to the `plugin`, sees it isn't a registered sub-command of the top-level docker (because it isn't, it's a plugin) so it accumulates it as an argument to the top-level `docker` command. Then it sees the `-c`, and thinks it is the global `-c` (for AKA `--context`) option and tries to treat it as that, which fails. In the specific case of the top-level `docker` subcommand we know that it has no arguments which aren't `--flags` (or `-f` short flags) and so anything which doesn't start with a `-` must either be a (known) subcommand or an attempt to execute a plugin. We could simply scan for and register all installed plugins at start of day, so that cobra can do the right thing, but we want to avoid that since it would involve executing each plugin to fetch the metadata, even if the command wasn't going to end up hitting a plugin. Instead we can parse the initial set of global arguments separately before hitting the main cobra `Execute` path, which works here exactly because we know that the top-level has no non-flag arguments. One slight wrinkle is that the top-level `PersistentPreRunE` is no longer called on the plugins path (since it no longer goes via `Execute`), so we arrange for the initialisation done there (which has to be done after global flags are parsed to handle e.g. `--config`) to happen explictly after the global flags are parsed. Rather than make `newDockerCommand` return the complicated set of results needed to make this happen, instead return a closure which achieves this. The new functionality is introduced via a common `TopLevelCommand` abstraction which lets us adjust the plugin entrypoint to use the same strategy for parsing the global arguments. This isn't strictly required (in this case the stuff in cobra's `Execute` works fine) but doing it this way avoids the possibility of subtle differences in behaviour. Fixes #1699, and also, as a side-effect, the first item in #1661. Signed-off-by: Ian Campbell --- cli-plugins/examples/helloworld/main.go | 4 - cli-plugins/plugin/plugin.go | 71 ++++--------- cli/cobra.go | 73 +++++++++++++ cmd/docker/docker.go | 133 +++++++++++------------- cmd/docker/docker_test.go | 47 +++++---- e2e/cli-plugins/flags_test.go | 38 +++---- 6 files changed, 193 insertions(+), 173 deletions(-) diff --git a/cli-plugins/examples/helloworld/main.go b/cli-plugins/examples/helloworld/main.go index 6251e85916..eca6acbef9 100644 --- a/cli-plugins/examples/helloworld/main.go +++ b/cli-plugins/examples/helloworld/main.go @@ -51,10 +51,6 @@ func main() { cmd := &cobra.Command{ Use: "helloworld", Short: "A basic Hello World plugin for tests", - // This is redundant but included to exercise - // the path where a plugin overrides this - // hook. - PersistentPreRunE: plugin.PersistentPreRunE, RunE: func(cmd *cobra.Command, args []string) error { if debug { fmt.Fprintf(dockerCli.Err(), "Plugin debug mode enabled") diff --git a/cli-plugins/plugin/plugin.go b/cli-plugins/plugin/plugin.go index 99ef36d845..bad632d1f8 100644 --- a/cli-plugins/plugin/plugin.go +++ b/cli-plugins/plugin/plugin.go @@ -4,18 +4,32 @@ import ( "encoding/json" "fmt" "os" - "sync" "github.com/docker/cli/cli" "github.com/docker/cli/cli-plugins/manager" "github.com/docker/cli/cli/command" "github.com/docker/cli/cli/connhelper" - cliflags "github.com/docker/cli/cli/flags" "github.com/docker/docker/client" "github.com/spf13/cobra" - "github.com/spf13/pflag" ) +func runPlugin(dockerCli *command.DockerCli, plugin *cobra.Command, meta manager.Metadata) error { + tcmd := newPluginCommand(dockerCli, plugin, meta) + + // Doing this here avoids also calling it for the metadata + // command which needlessly initializes the client and tries + // to connect to the daemon. + plugin.PersistentPreRunE = func(_ *cobra.Command, _ []string) error { + return tcmd.Initialize(withPluginClientConn(plugin.Name())) + } + + cmd, _, err := tcmd.HandleGlobalFlags() + if err != nil { + return err + } + return cmd.Execute() +} + // Run is the top-level entry point to the CLI plugin framework. It should be called from your plugin's `main()` function. func Run(makeCmd func(command.Cli) *cobra.Command, meta manager.Metadata) { dockerCli, err := command.NewDockerCli() @@ -26,9 +40,7 @@ func Run(makeCmd func(command.Cli) *cobra.Command, meta manager.Metadata) { plugin := makeCmd(dockerCli) - cmd := newPluginCommand(dockerCli, plugin, meta) - - if err := cmd.Execute(); err != nil { + if err := runPlugin(dockerCli, plugin, meta); err != nil { if sterr, ok := err.(cli.StatusError); ok { if sterr.Status != "" { fmt.Fprintln(dockerCli.Err(), sterr.Status) @@ -45,40 +57,6 @@ func Run(makeCmd func(command.Cli) *cobra.Command, meta manager.Metadata) { } } -// options encapsulates the ClientOptions and FlagSet constructed by -// `newPluginCommand` such that they can be finalized by our -// `PersistentPreRunE`. This is necessary because otherwise a plugin's -// own use of that hook will shadow anything we add to the top-level -// command meaning the CLI is never Initialized. -var options struct { - name string - init, prerun sync.Once - opts *cliflags.ClientOptions - flags *pflag.FlagSet - dockerCli *command.DockerCli -} - -// PersistentPreRunE must be called by any plugin command (or -// subcommand) which uses the cobra `PersistentPreRun*` hook. Plugins -// which do not make use of `PersistentPreRun*` do not need to call -// this (although it remains safe to do so). Plugins are recommended -// to use `PersistenPreRunE` to enable the error to be -// returned. Should not be called outside of a commands -// PersistentPreRunE hook and must not be run unless Run has been -// called. -func PersistentPreRunE(cmd *cobra.Command, args []string) error { - var err error - options.prerun.Do(func() { - if options.opts == nil || options.flags == nil || options.dockerCli == nil { - panic("PersistentPreRunE called without Run successfully called first") - } - // flags must be the original top-level command flags, not cmd.Flags() - options.opts.Common.SetDefaultOptions(options.flags) - err = options.dockerCli.Initialize(options.opts, withPluginClientConn(options.name)) - }) - return err -} - func withPluginClientConn(name string) command.InitializeOpt { return command.WithInitializeClient(func(dockerCli *command.DockerCli) (client.APIClient, error) { cmd := "docker" @@ -111,7 +89,7 @@ func withPluginClientConn(name string) command.InitializeOpt { }) } -func newPluginCommand(dockerCli *command.DockerCli, plugin *cobra.Command, meta manager.Metadata) *cobra.Command { +func newPluginCommand(dockerCli *command.DockerCli, plugin *cobra.Command, meta manager.Metadata) *cli.TopLevelCommand { name := plugin.Name() fullname := manager.NamePrefix + name @@ -121,7 +99,6 @@ func newPluginCommand(dockerCli *command.DockerCli, plugin *cobra.Command, meta SilenceUsage: true, SilenceErrors: true, TraverseChildren: true, - PersistentPreRunE: PersistentPreRunE, DisableFlagsInUseLine: true, } opts, flags := cli.SetupPluginRootCommand(cmd) @@ -135,13 +112,7 @@ func newPluginCommand(dockerCli *command.DockerCli, plugin *cobra.Command, meta cli.DisableFlagsInUseLine(cmd) - options.init.Do(func() { - options.name = name - options.opts = opts - options.flags = flags - options.dockerCli = dockerCli - }) - return cmd + return cli.NewTopLevelCommand(cmd, dockerCli, opts, flags) } func newMetadataSubcommand(plugin *cobra.Command, meta manager.Metadata) *cobra.Command { @@ -151,8 +122,6 @@ func newMetadataSubcommand(plugin *cobra.Command, meta manager.Metadata) *cobra. cmd := &cobra.Command{ Use: manager.MetadataSubcommandName, Hidden: true, - // Suppress the global/parent PersistentPreRunE, which needlessly initializes the client and tries to connect to the daemon. - PersistentPreRun: func(cmd *cobra.Command, args []string) {}, RunE: func(cmd *cobra.Command, args []string) error { enc := json.NewEncoder(os.Stdout) enc.SetEscapeHTML(false) diff --git a/cli/cobra.go b/cli/cobra.go index 803872ee5a..ede2e46c98 100644 --- a/cli/cobra.go +++ b/cli/cobra.go @@ -2,9 +2,11 @@ package cli import ( "fmt" + "os" "strings" pluginmanager "github.com/docker/cli/cli-plugins/manager" + "github.com/docker/cli/cli/command" cliconfig "github.com/docker/cli/cli/config" cliflags "github.com/docker/cli/cli/flags" "github.com/docker/docker/pkg/term" @@ -84,6 +86,77 @@ func FlagErrorFunc(cmd *cobra.Command, err error) error { } } +// TopLevelCommand encapsulates a top-level cobra command (either +// docker CLI or a plugin) and global flag handling logic necessary +// for plugins. +type TopLevelCommand struct { + cmd *cobra.Command + dockerCli *command.DockerCli + opts *cliflags.ClientOptions + flags *pflag.FlagSet + args []string +} + +// NewTopLevelCommand returns a new TopLevelCommand object +func NewTopLevelCommand(cmd *cobra.Command, dockerCli *command.DockerCli, opts *cliflags.ClientOptions, flags *pflag.FlagSet) *TopLevelCommand { + return &TopLevelCommand{cmd, dockerCli, opts, flags, os.Args[1:]} +} + +// SetArgs sets the args (default os.Args[:1] used to invoke the command +func (tcmd *TopLevelCommand) SetArgs(args []string) { + tcmd.args = args + tcmd.cmd.SetArgs(args) +} + +// SetFlag sets a flag in the local flag set of the top-level command +func (tcmd *TopLevelCommand) SetFlag(name, value string) { + tcmd.cmd.Flags().Set(name, value) +} + +// HandleGlobalFlags takes care of parsing global flags defined on the +// command, it returns the underlying cobra command and the args it +// will be called with (or an error). +// +// On success the caller is responsible for calling Initialize() +// before calling `Execute` on the returned command. +func (tcmd *TopLevelCommand) HandleGlobalFlags() (*cobra.Command, []string, error) { + cmd := tcmd.cmd + + // We manually parse the global arguments and find the + // subcommand in order to properly deal with plugins. We rely + // on the root command never having any non-flag arguments. + flags := cmd.Flags() + + // We need !interspersed to ensure we stop at the first + // potential command instead of accumulating it into + // flags.Args() and then continuing on and finding other + // arguments which we try and treat as globals (when they are + // actually arguments to the subcommand). + flags.SetInterspersed(false) + defer flags.SetInterspersed(true) // Undo, any subsequent cmd.Execute() in the caller expects this. + + // We need the single parse to see both sets of flags. + flags.AddFlagSet(cmd.PersistentFlags()) + // Now parse the global flags, up to (but not including) the + // first command. The result will be that all the remaining + // arguments are in `flags.Args()`. + if err := flags.Parse(tcmd.args); err != nil { + // Our FlagErrorFunc uses the cli, make sure it is initialized + if err := tcmd.Initialize(); err != nil { + return nil, nil, err + } + return nil, nil, cmd.FlagErrorFunc()(cmd, err) + } + + return cmd, flags.Args(), nil +} + +// Initialize finalises global option parsing and initializes the docker client. +func (tcmd *TopLevelCommand) Initialize(ops ...command.InitializeOpt) error { + tcmd.opts.Common.SetDefaultOptions(tcmd.flags) + return tcmd.dockerCli.Initialize(tcmd.opts, ops...) +} + // VisitAll will traverse all commands from the root. // This is different from the VisitAll of cobra.Command where only parents // are checked. diff --git a/cmd/docker/docker.go b/cmd/docker/docker.go index dc540430d9..06f074431b 100644 --- a/cmd/docker/docker.go +++ b/cmd/docker/docker.go @@ -21,7 +21,7 @@ import ( "github.com/spf13/pflag" ) -func newDockerCommand(dockerCli *command.DockerCli) *cobra.Command { +func newDockerCommand(dockerCli *command.DockerCli) *cli.TopLevelCommand { var ( opts *cliflags.ClientOptions flags *pflag.FlagSet @@ -34,51 +34,14 @@ func newDockerCommand(dockerCli *command.DockerCli) *cobra.Command { SilenceUsage: true, SilenceErrors: true, TraverseChildren: true, - FParseErrWhitelist: cobra.FParseErrWhitelist{ - // UnknownFlags ignores any unknown - // --arguments on the top-level docker command - // only. This is necessary to allow passing - // --arguments to plugins otherwise - // e.g. `docker plugin --foo` is caught here - // in the monolithic CLI and `foo` is reported - // as an unknown argument. - UnknownFlags: true, - }, RunE: func(cmd *cobra.Command, args []string) error { if len(args) == 0 { return command.ShowHelp(dockerCli.Err())(cmd, args) } - plugincmd, err := pluginmanager.PluginRunCommand(dockerCli, args[0], cmd) - if pluginmanager.IsNotFound(err) { - return fmt.Errorf( - "docker: '%s' is not a docker command.\nSee 'docker --help'", args[0]) - } - if err != nil { - return err - } + return fmt.Errorf("docker: '%s' is not a docker command.\nSee 'docker --help'", args[0]) - err = plugincmd.Run() - if err != nil { - statusCode := 1 - exitErr, ok := err.(*exec.ExitError) - if !ok { - return err - } - if ws, ok := exitErr.Sys().(syscall.WaitStatus); ok { - statusCode = ws.ExitStatus() - } - return cli.StatusError{ - StatusCode: statusCode, - } - } - return nil }, PersistentPreRunE: func(cmd *cobra.Command, args []string) error { - // flags must be the top-level command flags, not cmd.Flags() - opts.Common.SetDefaultOptions(flags) - if err := dockerCli.Initialize(opts); err != nil { - return err - } return isSupported(cmd, dockerCli) }, Version: fmt.Sprintf("%s, build %s", version.Version, version.GitCommit), @@ -87,30 +50,28 @@ func newDockerCommand(dockerCli *command.DockerCli) *cobra.Command { opts, flags, helpCmd = cli.SetupRootCommand(cmd) flags.BoolP("version", "v", false, "Print version information and quit") - setFlagErrorFunc(dockerCli, cmd, flags, opts) + setFlagErrorFunc(dockerCli, cmd) - setupHelpCommand(dockerCli, cmd, helpCmd, flags, opts) - setHelpFunc(dockerCli, cmd, flags, opts) + setupHelpCommand(dockerCli, cmd, helpCmd) + setHelpFunc(dockerCli, cmd) cmd.SetOutput(dockerCli.Out()) commands.AddCommands(cmd, dockerCli) cli.DisableFlagsInUseLine(cmd) - setValidateArgs(dockerCli, cmd, flags, opts) + setValidateArgs(dockerCli, cmd) - return cmd + // flags must be the top-level command flags, not cmd.Flags() + return cli.NewTopLevelCommand(cmd, dockerCli, opts, flags) } -func setFlagErrorFunc(dockerCli *command.DockerCli, cmd *cobra.Command, flags *pflag.FlagSet, opts *cliflags.ClientOptions) { +func setFlagErrorFunc(dockerCli *command.DockerCli, cmd *cobra.Command) { // When invoking `docker stack --nonsense`, we need to make sure FlagErrorFunc return appropriate // output if the feature is not supported. // As above cli.SetupRootCommand(cmd) have already setup the FlagErrorFunc, we will add a pre-check before the FlagErrorFunc // is called. flagErrorFunc := cmd.FlagErrorFunc() cmd.SetFlagErrorFunc(func(cmd *cobra.Command, err error) error { - if err := initializeDockerCli(dockerCli, flags, opts); err != nil { - return err - } if err := isSupported(cmd, dockerCli); err != nil { return err } @@ -118,17 +79,12 @@ func setFlagErrorFunc(dockerCli *command.DockerCli, cmd *cobra.Command, flags *p }) } -func setupHelpCommand(dockerCli *command.DockerCli, rootCmd, helpCmd *cobra.Command, flags *pflag.FlagSet, opts *cliflags.ClientOptions) { +func setupHelpCommand(dockerCli command.Cli, rootCmd, helpCmd *cobra.Command) { origRun := helpCmd.Run origRunE := helpCmd.RunE helpCmd.Run = nil helpCmd.RunE = func(c *cobra.Command, args []string) error { - // No Persistent* hooks are called for help, so we must initialize here. - if err := initializeDockerCli(dockerCli, flags, opts); err != nil { - return err - } - if len(args) > 0 { helpcmd, err := pluginmanager.PluginRunCommand(dockerCli, args[0], rootCmd) if err == nil { @@ -163,14 +119,9 @@ func tryRunPluginHelp(dockerCli command.Cli, ccmd *cobra.Command, cargs []string return helpcmd.Run() } -func setHelpFunc(dockerCli *command.DockerCli, cmd *cobra.Command, flags *pflag.FlagSet, opts *cliflags.ClientOptions) { +func setHelpFunc(dockerCli command.Cli, cmd *cobra.Command) { defaultHelpFunc := cmd.HelpFunc() cmd.SetHelpFunc(func(ccmd *cobra.Command, args []string) { - if err := initializeDockerCli(dockerCli, flags, opts); err != nil { - ccmd.Println(err) - return - } - // Add a stub entry for every plugin so they are // included in the help output and so that // `tryRunPluginHelp` can find them or if we fall @@ -205,7 +156,7 @@ func setHelpFunc(dockerCli *command.DockerCli, cmd *cobra.Command, flags *pflag. }) } -func setValidateArgs(dockerCli *command.DockerCli, cmd *cobra.Command, flags *pflag.FlagSet, opts *cliflags.ClientOptions) { +func setValidateArgs(dockerCli *command.DockerCli, cmd *cobra.Command) { // The Args is handled by ValidateArgs in cobra, which does not allows a pre-hook. // As a result, here we replace the existing Args validation func to a wrapper, // where the wrapper will check to see if the feature is supported or not. @@ -223,9 +174,6 @@ func setValidateArgs(dockerCli *command.DockerCli, cmd *cobra.Command, flags *pf cmdArgs := ccmd.Args ccmd.Args = func(cmd *cobra.Command, args []string) error { - if err := initializeDockerCli(dockerCli, flags, opts); err != nil { - return err - } if err := isSupported(cmd, dockerCli); err != nil { return err } @@ -234,14 +182,53 @@ func setValidateArgs(dockerCli *command.DockerCli, cmd *cobra.Command, flags *pf }) } -func initializeDockerCli(dockerCli *command.DockerCli, flags *pflag.FlagSet, opts *cliflags.ClientOptions) error { - if dockerCli.Client() != nil { - return nil +func tryPluginRun(dockerCli command.Cli, cmd *cobra.Command, subcommand string) error { + plugincmd, err := pluginmanager.PluginRunCommand(dockerCli, subcommand, cmd) + if err != nil { + return err } - // when using --help, PersistentPreRun is not called, so initialization is needed. - // flags must be the top-level command flags, not cmd.Flags() - opts.Common.SetDefaultOptions(flags) - return dockerCli.Initialize(opts) + + if err := plugincmd.Run(); err != nil { + statusCode := 1 + exitErr, ok := err.(*exec.ExitError) + if !ok { + return err + } + if ws, ok := exitErr.Sys().(syscall.WaitStatus); ok { + statusCode = ws.ExitStatus() + } + return cli.StatusError{ + StatusCode: statusCode, + } + } + return nil +} + +func runDocker(dockerCli *command.DockerCli) error { + tcmd := newDockerCommand(dockerCli) + + cmd, args, err := tcmd.HandleGlobalFlags() + if err != nil { + return err + } + + if err := tcmd.Initialize(); err != nil { + return err + } + + if len(args) > 0 { + if _, _, err := cmd.Find(args); err != nil { + err := tryPluginRun(dockerCli, cmd, args[0]) + if !pluginmanager.IsNotFound(err) { + return err + } + // For plugin not found we fall through to + // cmd.Execute() which deals with reporting + // "command not found" in a consistent way. + } + } + + return cmd.Execute() } func main() { @@ -252,9 +239,7 @@ func main() { } logrus.SetOutput(dockerCli.Err()) - cmd := newDockerCommand(dockerCli) - - if err := cmd.Execute(); err != nil { + if err := runDocker(dockerCli); err != nil { if sterr, ok := err.(cli.StatusError); ok { if sterr.Status != "" { fmt.Fprintln(dockerCli.Err(), sterr.Status) diff --git a/cmd/docker/docker_test.go b/cmd/docker/docker_test.go index e0f23747c0..044daa12d3 100644 --- a/cmd/docker/docker_test.go +++ b/cmd/docker/docker_test.go @@ -2,6 +2,7 @@ package main import ( "bytes" + "io" "io/ioutil" "os" "testing" @@ -16,10 +17,12 @@ import ( func TestClientDebugEnabled(t *testing.T) { defer debug.Disable() - cmd := newDockerCommand(&command.DockerCli{}) - cmd.Flags().Set("debug", "true") - - err := cmd.PersistentPreRunE(cmd, []string{}) + tcmd := newDockerCommand(&command.DockerCli{}) + tcmd.SetFlag("debug", "true") + cmd, _, err := tcmd.HandleGlobalFlags() + assert.NilError(t, err) + assert.NilError(t, tcmd.Initialize()) + err = cmd.PersistentPreRunE(cmd, []string{}) assert.NilError(t, err) assert.Check(t, is.Equal("1", os.Getenv("DEBUG"))) assert.Check(t, is.Equal(logrus.DebugLevel, logrus.GetLevel())) @@ -27,31 +30,37 @@ func TestClientDebugEnabled(t *testing.T) { var discard = ioutil.NopCloser(bytes.NewBuffer(nil)) -func TestExitStatusForInvalidSubcommandWithHelpFlag(t *testing.T) { - cli, err := command.NewDockerCli(command.WithInputStream(discard), command.WithCombinedStreams(ioutil.Discard)) +func runCliCommand(t *testing.T, r io.ReadCloser, w io.Writer, args ...string) error { + t.Helper() + if r == nil { + r = discard + } + if w == nil { + w = ioutil.Discard + } + cli, err := command.NewDockerCli(command.WithInputStream(r), command.WithCombinedStreams(w)) assert.NilError(t, err) - cmd := newDockerCommand(cli) - cmd.SetArgs([]string{"help", "invalid"}) - err = cmd.Execute() + tcmd := newDockerCommand(cli) + tcmd.SetArgs(args) + cmd, _, err := tcmd.HandleGlobalFlags() + assert.NilError(t, err) + assert.NilError(t, tcmd.Initialize()) + return cmd.Execute() +} + +func TestExitStatusForInvalidSubcommandWithHelpFlag(t *testing.T) { + err := runCliCommand(t, nil, nil, "help", "invalid") assert.Error(t, err, "unknown help topic: invalid") } func TestExitStatusForInvalidSubcommand(t *testing.T) { - cli, err := command.NewDockerCli(command.WithInputStream(discard), command.WithCombinedStreams(ioutil.Discard)) - assert.NilError(t, err) - cmd := newDockerCommand(cli) - cmd.SetArgs([]string{"invalid"}) - err = cmd.Execute() + err := runCliCommand(t, nil, nil, "invalid") assert.Check(t, is.ErrorContains(err, "docker: 'invalid' is not a docker command.")) } func TestVersion(t *testing.T) { var b bytes.Buffer - cli, err := command.NewDockerCli(command.WithInputStream(discard), command.WithCombinedStreams(&b)) - assert.NilError(t, err) - cmd := newDockerCommand(cli) - cmd.SetArgs([]string{"--version"}) - err = cmd.Execute() + err := runCliCommand(t, nil, &b, "--version") assert.NilError(t, err) assert.Check(t, is.Contains(b.String(), "Docker version")) } diff --git a/e2e/cli-plugins/flags_test.go b/e2e/cli-plugins/flags_test.go index 4bf01b9c47..5a446f6eb8 100644 --- a/e2e/cli-plugins/flags_test.go +++ b/e2e/cli-plugins/flags_test.go @@ -3,8 +3,6 @@ package cliplugins import ( "testing" - "gotest.tools/assert" - is "gotest.tools/assert/cmp" "gotest.tools/icmd" ) @@ -73,28 +71,18 @@ func TestUnknownGlobal(t *testing.T) { run, _, cleanup := prepare(t) defer cleanup() - t.Run("no-val", func(t *testing.T) { - res := icmd.RunCmd(run("--unknown", "helloworld")) - res.Assert(t, icmd.Expected{ - ExitCode: 125, + for name, args := range map[string][]string{ + "no-val": {"--unknown", "helloworld"}, + "separate-val": {"--unknown", "foo", "helloworld"}, + "joined-val": {"--unknown=foo", "helloworld"}, + } { + t.Run(name, func(t *testing.T) { + res := icmd.RunCmd(run(args...)) + res.Assert(t, icmd.Expected{ + ExitCode: 125, + Out: icmd.None, + Err: "unknown flag: --unknown", + }) }) - assert.Assert(t, is.Equal(res.Stdout(), "")) - assert.Assert(t, is.Contains(res.Stderr(), "unknown flag: --unknown")) - }) - t.Run("separate-val", func(t *testing.T) { - res := icmd.RunCmd(run("--unknown", "foo", "helloworld")) - res.Assert(t, icmd.Expected{ - ExitCode: 125, - }) - assert.Assert(t, is.Equal(res.Stdout(), "")) - assert.Assert(t, is.Contains(res.Stderr(), "unknown flag: --unknown")) - }) - t.Run("joined-val", func(t *testing.T) { - res := icmd.RunCmd(run("--unknown=foo", "helloworld")) - res.Assert(t, icmd.Expected{ - ExitCode: 125, - }) - assert.Assert(t, is.Equal(res.Stdout(), "")) - assert.Assert(t, is.Contains(res.Stderr(), "unknown flag: --unknown")) - }) + } } From 2c624e89841a70f7de998e1e46f8139ec7b661ee Mon Sep 17 00:00:00 2001 From: Ian Campbell Date: Fri, 8 Mar 2019 11:16:22 +0000 Subject: [PATCH 4/5] Add e2e test for handling of `version`/`-v`/`--version` with plugins Previous commits fixed the first issue on #1661, this simply adds a test for it. Note that this is testing the current behaviour, without regard for the second issue in #1661 which proposes a different behaviour. Signed-off-by: Ian Campbell --- e2e/cli-plugins/flags_test.go | 115 ++++++++++++++++++++++++++++++++++ 1 file changed, 115 insertions(+) diff --git a/e2e/cli-plugins/flags_test.go b/e2e/cli-plugins/flags_test.go index 5a446f6eb8..a958b18413 100644 --- a/e2e/cli-plugins/flags_test.go +++ b/e2e/cli-plugins/flags_test.go @@ -86,3 +86,118 @@ func TestUnknownGlobal(t *testing.T) { }) } } + +// TestCliPluginsVersion checks that `-v` and friends DTRT +func TestCliPluginsVersion(t *testing.T) { + run, _, cleanup := prepare(t) + defer cleanup() + + for _, tc := range []struct { + name string + args []string + expCode int + expOut, expErr string + }{ + { + name: "global-version", + args: []string{"version"}, + expCode: 0, + expOut: "Client:\n Version:", + expErr: icmd.None, + }, + { + name: "global-version-flag", + args: []string{"--version"}, + expCode: 0, + expOut: "Docker version", + expErr: icmd.None, + }, + { + name: "global-short-version-flag", + args: []string{"-v"}, + expCode: 0, + expOut: "Docker version", + expErr: icmd.None, + }, + { + name: "global-with-unknown-arg", + args: []string{"version", "foo"}, + expCode: 1, + expOut: icmd.None, + expErr: `"docker version" accepts no arguments.`, + }, + { + name: "global-with-plugin-arg", + args: []string{"version", "helloworld"}, + expCode: 1, + expOut: icmd.None, + expErr: `"docker version" accepts no arguments.`, + }, + { + name: "global-version-flag-with-unknown-arg", + args: []string{"--version", "foo"}, + expCode: 0, + expOut: "Docker version", + expErr: icmd.None, + }, + { + name: "global-short-version-flag-with-unknown-arg", + args: []string{"-v", "foo"}, + expCode: 0, + expOut: "Docker version", + expErr: icmd.None, + }, + { + name: "global-version-flag-with-plugin", + args: []string{"--version", "helloworld"}, + expCode: 125, + expOut: icmd.None, + expErr: "unknown flag: --version", + }, + { + name: "global-short-version-flag-with-plugin", + args: []string{"-v", "helloworld"}, + expCode: 125, + expOut: icmd.None, + expErr: "unknown shorthand flag: 'v' in -v", + }, + { + name: "plugin-with-version", + args: []string{"helloworld", "version"}, + expCode: 0, + expOut: "Hello World!", + expErr: icmd.None, + }, + { + name: "plugin-with-version-flag", + args: []string{"helloworld", "--version"}, + expCode: 125, + expOut: icmd.None, + expErr: "unknown flag: --version", + }, + { + name: "plugin-with-short-version-flag", + args: []string{"helloworld", "-v"}, + expCode: 125, + expOut: icmd.None, + expErr: "unknown shorthand flag: 'v' in -v", + }, + { + name: "", + args: []string{}, + expCode: 0, + expOut: "", + expErr: "", + }, + } { + t.Run(tc.name, func(t *testing.T) { + res := icmd.RunCmd(run(tc.args...)) + res.Assert(t, icmd.Expected{ + ExitCode: tc.expCode, + Out: tc.expOut, + Err: tc.expErr, + }) + }) + } + +} From e824bc86f3889b2715acf11bf23edd11d144cfb5 Mon Sep 17 00:00:00 2001 From: Ian Campbell Date: Tue, 12 Mar 2019 11:13:50 +0000 Subject: [PATCH 5/5] Use a copy of root flagset in `HandleGlobalFlags` This makes things more idempotent, rather than relying on undoing the interspersed settings. Note that the underlying `Flag`s remain shared, it's just the `FlagSet` which is duplicated. Signed-off-by: Ian Campbell --- cli/cobra.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/cli/cobra.go b/cli/cobra.go index ede2e46c98..1385743f4b 100644 --- a/cli/cobra.go +++ b/cli/cobra.go @@ -124,8 +124,10 @@ func (tcmd *TopLevelCommand) HandleGlobalFlags() (*cobra.Command, []string, erro // We manually parse the global arguments and find the // subcommand in order to properly deal with plugins. We rely - // on the root command never having any non-flag arguments. - flags := cmd.Flags() + // on the root command never having any non-flag arguments. We + // create our own FlagSet so that we can configure it + // (e.g. `SetInterspersed` below) in an idempotent way. + flags := pflag.NewFlagSet(cmd.Name(), pflag.ContinueOnError) // We need !interspersed to ensure we stop at the first // potential command instead of accumulating it into @@ -133,9 +135,9 @@ func (tcmd *TopLevelCommand) HandleGlobalFlags() (*cobra.Command, []string, erro // arguments which we try and treat as globals (when they are // actually arguments to the subcommand). flags.SetInterspersed(false) - defer flags.SetInterspersed(true) // Undo, any subsequent cmd.Execute() in the caller expects this. // We need the single parse to see both sets of flags. + flags.AddFlagSet(cmd.Flags()) flags.AddFlagSet(cmd.PersistentFlags()) // Now parse the global flags, up to (but not including) the // first command. The result will be that all the remaining