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

Allow plugins to have --flags which are the same as the top-level

Author: silvin-lubecki

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

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)

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.