Merge pull request #1722 from ijc/plugins-global-arg-clash

Allow plugins to have --flags which are the same as the top-level
2019-03-13 15:55:02 +01:00 · 2019-03-13 15:55:02 +01:00 · 237bdbf5f6
parent d6a230606c e824bc86f3
commit 237bdbf5f6
8 changed files with 408 additions and 162 deletions
--- a/cli-plugins/examples/helloworld/main.go
+++ b/cli-plugins/examples/helloworld/main.go
@ -44,15 +44,26 @@ func main() {
 			},
 		}

-		var who string
+		var (
+			who, context string
+			debug        bool
+		)
 		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")
+				}
+
+				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 +79,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
--- 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)
--- 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,79 @@ 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. 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
+	// 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)
+
+	// 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
+	// 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.
--- 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)
--- 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"))
 }
--- a/e2e/cli-plugins/flags_test.go
+++ b/e2e/cli-plugins/flags_test.go
@ -0,0 +1,203 @@
+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!",
+	})
+}
+
+// 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()
+
+	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",
+			})
+		})
+	}
+}
+
+// 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,
+			})
+		})
+	}
+
+}
--- 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.
--- 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