From 0b794e0620b93a304cb2ad48112582489467812d Mon Sep 17 00:00:00 2001 From: Ian Campbell Date: Wed, 13 Mar 2019 17:50:51 +0000 Subject: [PATCH 1/2] e2e/cli-plugins: explicitly check that PersistentPreRunE works I regressed this in d4ced2ef77fc ("allow plugins to have argument which match a top-level flag.") by unconditionally overwriting any `PersistentRunE` that the user may have supplied. We need to ensure two things: 1. That the user can use `PersistentRunE` (or `PersistentRun`) for their own purposes. 2. That our initialisation always runs, even if the user has used `PersistentRun*`, since that will shadow the root. To do this add a `PersistentRunE` to the helloworld plugin which logs (covers 1 above) and then use it when calling the `apiversion` subcommand (which covers 2 since that uses the client) Signed-off-by: Ian Campbell --- cli-plugins/examples/helloworld/main.go | 11 +++++++++-- e2e/cli-plugins/run_test.go | 4 ++-- .../testdata/docker-help-helloworld.golden | 1 + 3 files changed, 12 insertions(+), 4 deletions(-) diff --git a/cli-plugins/examples/helloworld/main.go b/cli-plugins/examples/helloworld/main.go index eca6acbef9..a99c59122e 100644 --- a/cli-plugins/examples/helloworld/main.go +++ b/cli-plugins/examples/helloworld/main.go @@ -45,12 +45,18 @@ func main() { } var ( - who, context string - debug bool + who, context string + preRun, debug bool ) cmd := &cobra.Command{ Use: "helloworld", Short: "A basic Hello World plugin for tests", + PersistentPreRunE: func(_ *cobra.Command, _ []string) error { + if preRun { + fmt.Fprintf(dockerCli.Err(), "Plugin PersistentPreRunE called") + } + return nil + }, RunE: func(cmd *cobra.Command, args []string) error { if debug { fmt.Fprintf(dockerCli.Err(), "Plugin debug mode enabled") @@ -79,6 +85,7 @@ func main() { flags := cmd.Flags() flags.StringVar(&who, "who", "", "Who are we addressing?") + flags.BoolVar(&preRun, "pre-run", false, "Log from prerun hook") // These are intended to deliberately clash with the CLIs own top // level arguments. flags.BoolVarP(&debug, "debug", "D", false, "Enable debug") diff --git a/e2e/cli-plugins/run_test.go b/e2e/cli-plugins/run_test.go index 171ed81c83..96cbead4a0 100644 --- a/e2e/cli-plugins/run_test.go +++ b/e2e/cli-plugins/run_test.go @@ -177,10 +177,10 @@ func TestCliInitialized(t *testing.T) { run, _, cleanup := prepare(t) defer cleanup() - res := icmd.RunCmd(run("helloworld", "apiversion")) + res := icmd.RunCmd(run("helloworld", "--pre-run", "apiversion")) res.Assert(t, icmd.Success) assert.Assert(t, res.Stdout() != "") - assert.Assert(t, is.Equal(res.Stderr(), "")) + assert.Assert(t, is.Equal(res.Stderr(), "Plugin PersistentPreRunE called")) } // TestPluginErrorCode tests when the plugin return with a given exit status. diff --git a/e2e/cli-plugins/testdata/docker-help-helloworld.golden b/e2e/cli-plugins/testdata/docker-help-helloworld.golden index 24bc424bd7..9d2f6673ee 100644 --- a/e2e/cli-plugins/testdata/docker-help-helloworld.golden +++ b/e2e/cli-plugins/testdata/docker-help-helloworld.golden @@ -6,6 +6,7 @@ A basic Hello World plugin for tests Options: -c, --context string Is it Christmas? -D, --debug Enable debug + --pre-run Log from prerun hook --who string Who are we addressing? Commands: From 3af168c7df4f623a84a66a0ee809904e943e1c01 Mon Sep 17 00:00:00 2001 From: Ian Campbell Date: Thu, 14 Mar 2019 14:02:49 +0000 Subject: [PATCH 2/2] 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)