From 941a493f49723dfa7c5311705449d596973e16b9 Mon Sep 17 00:00:00 2001 From: Ian Campbell Date: Fri, 26 Apr 2019 10:37:26 +0100 Subject: [PATCH 1/5] Move subtests of TestGlobalHelp into actual subtests Signed-off-by: Ian Campbell (cherry picked from commit d57175aa2e42c2e5d4cc14bf0d66d5779ccd1617) Signed-off-by: Sebastiaan van Stijn --- e2e/cli-plugins/help_test.go | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/e2e/cli-plugins/help_test.go b/e2e/cli-plugins/help_test.go index 8e57ccc876..f023b0f2f1 100644 --- a/e2e/cli-plugins/help_test.go +++ b/e2e/cli-plugins/help_test.go @@ -74,18 +74,22 @@ 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())) }) - assert.Assert(t, is.Equal(res2.Stdout(), "")) - assert.Assert(t, is.Equal(res2.Stderr(), res.Stdout())) } From 6a6cd35985c0fa6d1ff28eb11995fe984e26275a Mon Sep 17 00:00:00 2001 From: Ian Campbell Date: Fri, 26 Apr 2019 15:09:09 +0100 Subject: [PATCH 2/5] Hide experimental builtin commands in help output on unknown flag. Previously `docker --badopt` would always include experimental commands even if experimental was not enabled. Signed-off-by: Ian Campbell (cherry picked from commit 79a75da0fd97b02311676028c3406b242c785f7c) Signed-off-by: Sebastiaan van Stijn --- cmd/docker/docker.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/cmd/docker/docker.go b/cmd/docker/docker.go index 22b978de9b..f522a29a03 100644 --- a/cmd/docker/docker.go +++ b/cmd/docker/docker.go @@ -79,6 +79,9 @@ func setFlagErrorFunc(dockerCli *command.DockerCli, cmd *cobra.Command) { if err := isSupported(cmd, dockerCli); err != nil { return err } + if err := hideUnsupportedFeatures(cmd, dockerCli); err != nil { + return err + } return flagErrorFunc(cmd, err) }) } From 7380aae6014ef848ae09f69cdf262b8d6f49f4db Mon Sep 17 00:00:00 2001 From: Ian Campbell Date: Fri, 26 Apr 2019 15:08:22 +0100 Subject: [PATCH 3/5] Include CLI plugins in help output on unknown flag. Previously `docker --badopt` output would not include CLI plugins. Fixes #1813 Signed-off-by: Ian Campbell (cherry picked from commit 40a6cf7c477cf328134b0b8dcdbd9a09d02f918b) Signed-off-by: Sebastiaan van Stijn --- cmd/docker/docker.go | 3 +++ e2e/cli-plugins/help_test.go | 14 ++++++++++++++ 2 files changed, 17 insertions(+) diff --git a/cmd/docker/docker.go b/cmd/docker/docker.go index f522a29a03..5b0fffb004 100644 --- a/cmd/docker/docker.go +++ b/cmd/docker/docker.go @@ -76,6 +76,9 @@ func setFlagErrorFunc(dockerCli *command.DockerCli, cmd *cobra.Command) { // 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 } diff --git a/e2e/cli-plugins/help_test.go b/e2e/cli-plugins/help_test.go index f023b0f2f1..b73054d4a3 100644 --- a/e2e/cli-plugins/help_test.go +++ b/e2e/cli-plugins/help_test.go @@ -92,4 +92,18 @@ func TestGlobalHelp(t *testing.T) { 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)) + }) } From d6ddcdfa6af7708d225f240bab7614bab6295060 Mon Sep 17 00:00:00 2001 From: Ian Campbell Date: Fri, 26 Apr 2019 15:43:03 +0100 Subject: [PATCH 4/5] Use command.Cli instead of command.DockerCli The linter is complaining: cmd/docker/docker.go:72:23:warning: dockerCli can be github.com/docker/cli/cli/command.Cli (interfacer) Unclear precisely which change in the preceeding commits caused it to notice this possibility. Signed-off-by: Ian Campbell (cherry picked from commit 7d0645c5fe817ca4e8677583e845b95f6723fe97) Signed-off-by: Sebastiaan van Stijn --- cmd/docker/docker.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/docker/docker.go b/cmd/docker/docker.go index 5b0fffb004..dafaea0703 100644 --- a/cmd/docker/docker.go +++ b/cmd/docker/docker.go @@ -69,7 +69,7 @@ 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 From b8bfba8dc695cae8d2d58573c98c27ca34407411 Mon Sep 17 00:00:00 2001 From: Ian Campbell Date: Tue, 30 Apr 2019 10:19:58 +0100 Subject: [PATCH 5/5] 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 (cherry picked from commit af200f14ed0ac9b7cecf231c539d09cc13686346) Signed-off-by: Sebastiaan van Stijn --- 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.