[19.03 backport] plugin fixes

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,
 	}

cmd/docker/docker.go

@@ -69,16 +69,22 @@ func newDockerCommand(dockerCli *command.DockerCli) *cli.TopLevelCommand {
 	return cli.NewTopLevelCommand(cmd, dockerCli, opts, flags)
 }
 
-func setFlagErrorFunc(dockerCli *command.DockerCli, cmd *cobra.Command) {
+func setFlagErrorFunc(dockerCli command.Cli, 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 := pluginmanager.AddPluginCommandStubs(dockerCli, cmd.Root()); err != nil {
+			return err
+		}
 		if err := isSupported(cmd, dockerCli); err != nil {
 			return err
 		}
+		if err := hideUnsupportedFeatures(cmd, dockerCli); err != nil {
+			return err
+		}
 		return flagErrorFunc(cmd, err)
 	})
 }

e2e/cli-plugins/help_test.go

@@ -74,18 +74,36 @@ func TestGlobalHelp(t *testing.T) {
 	assert.Assert(t, is.Equal(badmetacount, 1))
 
 	// Running with `--help` should produce the same.
-	res2 := icmd.RunCmd(run("--help"))
-	res2.Assert(t, icmd.Expected{
-		ExitCode: 0,
+	t.Run("help_flag", func(t *testing.T) {
+		res2 := icmd.RunCmd(run("--help"))
+		res2.Assert(t, icmd.Expected{
+			ExitCode: 0,
+		})
+		assert.Assert(t, is.Equal(res2.Stdout(), res.Stdout()))
+		assert.Assert(t, is.Equal(res2.Stderr(), ""))
 	})
-	assert.Assert(t, is.Equal(res2.Stdout(), res.Stdout()))
-	assert.Assert(t, is.Equal(res2.Stderr(), ""))
 
 	// Running just `docker` (without `help` nor `--help`) should produce the same thing, except on Stderr.
-	res2 = icmd.RunCmd(run())
-	res2.Assert(t, icmd.Expected{
-		ExitCode: 0,
+	t.Run("bare", func(t *testing.T) {
+		res2 := icmd.RunCmd(run())
+		res2.Assert(t, icmd.Expected{
+			ExitCode: 0,
+		})
+		assert.Assert(t, is.Equal(res2.Stdout(), ""))
+		assert.Assert(t, is.Equal(res2.Stderr(), res.Stdout()))
+	})
+
+	t.Run("badopt", func(t *testing.T) {
+		// Running `docker --badopt` should also produce the
+		// same thing, give or take the leading error message
+		// and a trailing carriage return (due to main() using
+		// Println in the error case).
+		res2 := icmd.RunCmd(run("--badopt"))
+		res2.Assert(t, icmd.Expected{
+			ExitCode: 125,
+		})
+		assert.Assert(t, is.Equal(res2.Stdout(), ""))
+		exp := "unknown flag: --badopt\nSee 'docker --help'.\n" + res.Stdout() + "\n"
+		assert.Assert(t, is.Equal(res2.Stderr(), exp))
 	})
-	assert.Assert(t, is.Equal(res2.Stdout(), ""))
-	assert.Assert(t, is.Equal(res2.Stderr(), res.Stdout()))
 }

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",
+		})
+}

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.