From 3af168c7df4f623a84a66a0ee809904e943e1c01 Mon Sep 17 00:00:00 2001 From: Ian Campbell Date: Thu, 14 Mar 2019 14:02:49 +0000 Subject: [PATCH] Ensure plugins can use PersistentPreRunE again. I got a bit carried away in d4ced2ef77fcc ("allow plugins to have argument which match a top-level flag.") and broke the ability of a plugin to use the `PersistentPreRun(E)` hook on its top-level command (by unconditionally overwriting it) and also broke the plugin framework if a plugin's subcommand used those hooks (because they would shadow the root one). This could result in either `dockerCli.Client()` returning `nil` or whatever initialisation the plugin hoped to do not occuring. This change revert the relevant bits and reinstates the requirement that a plugin calls `plugin.PersistentPreRunE` if it uses that hook itself. It is at least a bit nicer now since we avoid the need for the global struct since the interesting state is now encapsulated in `tcmd` (and the closure). In principal this could be done even more simply (by calling `tcmd.Initialize` statically between `tcmd.HandleGlobalFlags` and `cmd.Execute`) however this has the downside of _always_ initialising the cli (and therefore dialing the daemon) even for the `docker-cli-plugin-metadata` command but also for the `help foo` and `foo --help` commands (Cobra short-circuits the hooks in this case). Signed-off-by: Ian Campbell --- cli-plugins/examples/helloworld/main.go | 5 ++++- cli-plugins/plugin/plugin.go | 28 ++++++++++++++++++++----- 2 files changed, 27 insertions(+), 6 deletions(-) diff --git a/cli-plugins/examples/helloworld/main.go b/cli-plugins/examples/helloworld/main.go index a99c59122e..ba23f4954e 100644 --- a/cli-plugins/examples/helloworld/main.go +++ b/cli-plugins/examples/helloworld/main.go @@ -51,7 +51,10 @@ func main() { cmd := &cobra.Command{ Use: "helloworld", Short: "A basic Hello World plugin for tests", - PersistentPreRunE: func(_ *cobra.Command, _ []string) error { + PersistentPreRunE: func(cmd *cobra.Command, args []string) error { + if err := plugin.PersistentPreRunE(cmd, args); err != nil { + return err + } if preRun { fmt.Fprintf(dockerCli.Err(), "Plugin PersistentPreRunE called") } diff --git a/cli-plugins/plugin/plugin.go b/cli-plugins/plugin/plugin.go index bad632d1f8..934baed884 100644 --- a/cli-plugins/plugin/plugin.go +++ b/cli-plugins/plugin/plugin.go @@ -4,6 +4,7 @@ import ( "encoding/json" "fmt" "os" + "sync" "github.com/docker/cli/cli" "github.com/docker/cli/cli-plugins/manager" @@ -13,14 +14,26 @@ import ( "github.com/spf13/cobra" ) +// 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 command's +// PersistentPreRunE hook and must not be run unless Run has been +// called. +var PersistentPreRunE func(*cobra.Command, []string) error + 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())) + var persistentPreRunOnce sync.Once + PersistentPreRunE = func(_ *cobra.Command, _ []string) error { + var err error + persistentPreRunOnce.Do(func() { + err = tcmd.Initialize(withPluginClientConn(plugin.Name())) + }) + return err } cmd, _, err := tcmd.HandleGlobalFlags() @@ -98,6 +111,7 @@ func newPluginCommand(dockerCli *command.DockerCli, plugin *cobra.Command, meta Short: fullname + " is a Docker CLI plugin", SilenceUsage: true, SilenceErrors: true, + PersistentPreRunE: PersistentPreRunE, TraverseChildren: true, DisableFlagsInUseLine: true, } @@ -122,6 +136,10 @@ 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)