From 8f3798cf04296e795204f75ba0a8c6a074c57e51 Mon Sep 17 00:00:00 2001 From: Ian Campbell Date: Thu, 28 Mar 2019 15:32:23 +0000 Subject: [PATCH] cli-plugins: Reinstate deprecated `-h` short form of `--help`. In the initial implementation I thought it would be good to not pass on the deprecation to plugins (since they are new). However it turns out this causes `docker helloworld -h` to print a spurious "pflag: help requested" line: $ docker helloworld -h pflag: help requested See 'docker helloworld --help'. Usage: docker helloworld [OPTIONS] COMMAND A basic Hello World plugin for tests ... Compared with: $ docker ps -h Flag shorthand -h has been deprecated, please use --help Usage: docker ps [OPTIONS] This is in essence because having the flag undefined hits a different path within cobra, causing `c.execute()` to return early due to getting an error (`flag.ErrHelp`) from `c.ParseFlags`, which launders the error through our `FlagErrorFunc` which wraps it in a `StatusError` which in turn defeats an `if err == flag.ErrHelp` check further up the call chain. If the flag is defined we instead hit a path which returns a bare `flag.ErrHelp` without wrapping it. I considered updating our `FlagErrorFunc` to not wrap `flag.ErrHelp` (and then following the chain to the next thing) however while doing that I realised that the code for `-h` (and `--help`) is deeply embedded into cobra (and its flags library) such that actually using `-h` as a plugin argument meaning something other than `help` is basically impossible/impractical. Therefore we may as well have plugins behave identically to the monolithic CLI and support (deprecated) the `-h` argument. With this changed the help related blocks of `SetupRootCommand` and `SetupPluginRootCommand` are now identical, so consolidate into `setupCommonRootCommand`. Tests are updated to check `-h` in a variety of scenarios, including the happy case here. Signed-off-by: Ian Campbell --- cli/cobra.go | 12 ++++-------- e2e/cli-plugins/run_test.go | 29 +++++++++++++++++++++++++++++ 2 files changed, 33 insertions(+), 8 deletions(-) diff --git a/cli/cobra.go b/cli/cobra.go index 1385743f4b..ed9c9b5bac 100644 --- a/cli/cobra.go +++ b/cli/cobra.go @@ -42,6 +42,10 @@ func setupCommonRootCommand(rootCmd *cobra.Command) (*cliflags.ClientOptions, *p rootCmd.SetFlagErrorFunc(FlagErrorFunc) rootCmd.SetHelpCommand(helpCommand) + rootCmd.PersistentFlags().BoolP("help", "h", false, "Print usage") + rootCmd.PersistentFlags().MarkShorthandDeprecated("help", "please use --help") + rootCmd.PersistentFlags().Lookup("help").Hidden = true + return opts, flags, helpCommand } @@ -52,20 +56,12 @@ func SetupRootCommand(rootCmd *cobra.Command) (*cliflags.ClientOptions, *pflag.F rootCmd.SetVersionTemplate("Docker version {{.Version}}\n") - rootCmd.PersistentFlags().BoolP("help", "h", false, "Print usage") - rootCmd.PersistentFlags().MarkShorthandDeprecated("help", "please use --help") - rootCmd.PersistentFlags().Lookup("help").Hidden = true - return opts, flags, helpCmd } // SetupPluginRootCommand sets default usage, help and error handling for a plugin root command. func SetupPluginRootCommand(rootCmd *cobra.Command) (*cliflags.ClientOptions, *pflag.FlagSet) { opts, flags, _ := setupCommonRootCommand(rootCmd) - - rootCmd.PersistentFlags().BoolP("help", "", false, "Print usage") - rootCmd.PersistentFlags().Lookup("help").Hidden = true - return opts, flags } diff --git a/e2e/cli-plugins/run_test.go b/e2e/cli-plugins/run_test.go index f337a99d18..1309cc0d58 100644 --- a/e2e/cli-plugins/run_test.go +++ b/e2e/cli-plugins/run_test.go @@ -9,6 +9,8 @@ import ( "gotest.tools/icmd" ) +const shortHFlagDeprecated = "Flag shorthand -h has been deprecated, please use --help\n" + // TestRunNonexisting ensures correct behaviour when running a nonexistent plugin. func TestRunNonexisting(t *testing.T) { run, _, cleanup := prepare(t) @@ -50,6 +52,15 @@ func TestNonexistingHelp(t *testing.T) { Out: "Usage: docker [OPTIONS] COMMAND\n\nA self-sufficient runtime for containers", Err: icmd.None, }) + // Short -h should be the same, modulo the deprecation message + exp := shortHFlagDeprecated + res.Stdout() + res = icmd.RunCmd(run("nonexistent", "-h")) + res.Assert(t, icmd.Expected{ + ExitCode: 0, + // This should be identical to the --help case above + Out: exp, + Err: icmd.None, + }) } // TestRunBad ensures correct behaviour when running an existent but invalid plugin @@ -93,6 +104,15 @@ func TestBadHelp(t *testing.T) { Out: "Usage: docker [OPTIONS] COMMAND\n\nA self-sufficient runtime for containers", Err: icmd.None, }) + // Short -h should be the same, modulo the deprecation message + exp := shortHFlagDeprecated + res.Stdout() + res = icmd.RunCmd(run("badmeta", "-h")) + res.Assert(t, icmd.Expected{ + ExitCode: 0, + // This should be identical to the --help case above + Out: exp, + Err: icmd.None, + }) } // TestRunGood ensures correct behaviour when running a valid plugin @@ -137,6 +157,15 @@ func TestGoodHelp(t *testing.T) { }) // This is the same golden file as `TestHelpGood`, above. golden.Assert(t, res.Stdout(), "docker-help-helloworld.golden") + // Short -h should be the same, modulo the deprecation message + exp := shortHFlagDeprecated + res.Stdout() + res = icmd.RunCmd(run("-D", "helloworld", "-h")) + res.Assert(t, icmd.Expected{ + ExitCode: 0, + // This should be identical to the --help case above + Out: exp, + Err: icmd.None, + }) } // TestRunGoodSubcommand ensures correct behaviour when running a valid plugin with a subcommand