From b8bfba8dc695cae8d2d58573c98c27ca34407411 Mon Sep 17 00:00:00 2001
From: Ian Campbell <ijc@docker.com>
Date: Tue, 30 Apr 2019 10:19:58 +0100
Subject: [PATCH] cli-plugins: fix when plugin does not use PersistentPreRun*
 hooks

This regressed in 3af168c7df4f ("Ensure plugins can use `PersistentPreRunE`
again.") but this wasn't noticed because the helloworld test plugin has it's
own `PersistentPreRunE` which has the effect of deferring the resolution of the
global variable. In the case where the hook isn't used the variable is resolved
during `newPluginCommand` which is before the global variable was set.

Initialize the plugin command with a stub function wrapping the call to the
(global) hook, this defers resolving the variable until after it has been set,
otherwise the initial value (`nil`) is used in the struct.

Signed-off-by: Ian Campbell <ijc@docker.com>
(cherry picked from commit af200f14ed0ac9b7cecf231c539d09cc13686346)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
---
 cli-plugins/plugin/plugin.go                  | 13 ++++---
 .../plugins/nopersistentprerun/main.go        | 36 +++++++++++++++++++
 e2e/cli-plugins/run_test.go                   | 19 +++++++---
 3 files changed, 58 insertions(+), 10 deletions(-)
 create mode 100644 e2e/cli-plugins/plugins/nopersistentprerun/main.go

diff --git a/cli-plugins/plugin/plugin.go b/cli-plugins/plugin/plugin.go
index 62ab11c9e6..7bd5f50c60 100644
--- a/cli-plugins/plugin/plugin.go
+++ b/cli-plugins/plugin/plugin.go
@@ -114,11 +114,14 @@ func newPluginCommand(dockerCli *command.DockerCli, plugin *cobra.Command, meta
 	fullname := manager.NamePrefix + name
 
 	cmd := &cobra.Command{
-		Use:                   fmt.Sprintf("docker [OPTIONS] %s [ARG...]", name),
-		Short:                 fullname + " is a Docker CLI plugin",
-		SilenceUsage:          true,
-		SilenceErrors:         true,
-		PersistentPreRunE:     PersistentPreRunE,
+		Use:           fmt.Sprintf("docker [OPTIONS] %s [ARG...]", name),
+		Short:         fullname + " is a Docker CLI plugin",
+		SilenceUsage:  true,
+		SilenceErrors: true,
+		PersistentPreRunE: func(cmd *cobra.Command, args []string) error {
+			// We can't use this as the hook directly since it is initialised later (in runPlugin)
+			return PersistentPreRunE(cmd, args)
+		},
 		TraverseChildren:      true,
 		DisableFlagsInUseLine: true,
 	}
diff --git a/e2e/cli-plugins/plugins/nopersistentprerun/main.go b/e2e/cli-plugins/plugins/nopersistentprerun/main.go
new file mode 100644
index 0000000000..caaddb69c4
--- /dev/null
+++ b/e2e/cli-plugins/plugins/nopersistentprerun/main.go
@@ -0,0 +1,36 @@
+package main
+
+import (
+	"context"
+	"fmt"
+
+	"github.com/docker/cli/cli-plugins/manager"
+	"github.com/docker/cli/cli-plugins/plugin"
+	"github.com/docker/cli/cli/command"
+	"github.com/spf13/cobra"
+)
+
+func main() {
+	plugin.Run(func(dockerCli command.Cli) *cobra.Command {
+		cmd := &cobra.Command{
+			Use:   "nopersistentprerun",
+			Short: "Testing without PersistentPreRun hooks",
+			//PersistentPreRunE: Not specified, we need to test that it works in the absence of an explicit call
+			RunE: func(cmd *cobra.Command, args []string) error {
+				cli := dockerCli.Client()
+				ping, err := cli.Ping(context.Background())
+				if err != nil {
+					return err
+				}
+				fmt.Println(ping.APIVersion)
+				return nil
+			},
+		}
+		return cmd
+	},
+		manager.Metadata{
+			SchemaVersion: "0.1.0",
+			Vendor:        "Docker Inc.",
+			Version:       "testing",
+		})
+}
diff --git a/e2e/cli-plugins/run_test.go b/e2e/cli-plugins/run_test.go
index 3c9426e042..91d384645e 100644
--- a/e2e/cli-plugins/run_test.go
+++ b/e2e/cli-plugins/run_test.go
@@ -213,15 +213,24 @@ func TestGoodSubcommandHelp(t *testing.T) {
 }
 
 // TestCliInitialized tests the code paths which ensure that the Cli
-// object is initialized even if the plugin uses PersistentRunE
+// object is initialized whether the plugin uses PersistentRunE or not
 func TestCliInitialized(t *testing.T) {
 	run, _, cleanup := prepare(t)
 	defer cleanup()
 
-	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(), "Plugin PersistentPreRunE called"))
+	var apiversion string
+	t.Run("withhook", func(t *testing.T) {
+		res := icmd.RunCmd(run("helloworld", "--pre-run", "apiversion"))
+		res.Assert(t, icmd.Success)
+		assert.Assert(t, res.Stdout() != "")
+		apiversion = res.Stdout()
+		assert.Assert(t, is.Equal(res.Stderr(), "Plugin PersistentPreRunE called"))
+	})
+	t.Run("withouthook", func(t *testing.T) {
+		res := icmd.RunCmd(run("nopersistentprerun"))
+		res.Assert(t, icmd.Success)
+		assert.Assert(t, is.Equal(res.Stdout(), apiversion))
+	})
 }
 
 // TestPluginErrorCode tests when the plugin return with a given exit status.