From 95d8a1e45d7719c56dc017e075d3e6099deba85d Mon Sep 17 00:00:00 2001 From: Haoming Meng <41393704+Techming@users.noreply.github.com> Date: Mon, 9 Oct 2023 07:50:40 -0500 Subject: [PATCH 01/10] Add notes to doc on preRun and postRun condition (#2041) --- command.go | 2 ++ site/content/user_guide.md | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/command.go b/command.go index 6866f7d0..909d8585 100644 --- a/command.go +++ b/command.go @@ -115,6 +115,8 @@ type Command struct { // * PostRun() // * PersistentPostRun() // All functions get the same args, the arguments after the command name. + // The *PreRun and *PostRun functions will only be executed if the Run function of the current + // command has been declared. // // PersistentPreRun: children of this command will inherit and execute. PersistentPreRun func(cmd *Command, args []string) diff --git a/site/content/user_guide.md b/site/content/user_guide.md index 93daadf5..55cc252b 100644 --- a/site/content/user_guide.md +++ b/site/content/user_guide.md @@ -604,7 +604,7 @@ The Prefix, `Error:` can be customized using the `cmd.SetErrPrefix(s string)` fu ## PreRun and PostRun Hooks -It is possible to run functions before or after the main `Run` function of your command. The `PersistentPreRun` and `PreRun` functions will be executed before `Run`. `PersistentPostRun` and `PostRun` will be executed after `Run`. The `Persistent*Run` functions will be inherited by children if they do not declare their own. These functions are run in the following order: +It is possible to run functions before or after the main `Run` function of your command. The `PersistentPreRun` and `PreRun` functions will be executed before `Run`. `PersistentPostRun` and `PostRun` will be executed after `Run`. The `Persistent*Run` functions will be inherited by children if they do not declare their own. The `*PreRun` and `*PostRun` functions will only be executed if the `Run` function of the current command has been declared. These functions are run in the following order: - `PersistentPreRun` - `PreRun` From efe8fa3e4453e41d6419b26c9769a51e42825632 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Sun, 15 Oct 2023 11:16:50 +0000 Subject: [PATCH 02/10] build(deps): bump actions/setup-go from 3 to 4 (#1934) --- .github/workflows/test.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 6b4c165d..55085eaf 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -41,7 +41,7 @@ jobs: - uses: actions/checkout@v3 - - uses: actions/setup-go@v3 + - uses: actions/setup-go@v4 with: go-version: '^1.21' check-latest: true @@ -74,7 +74,7 @@ jobs: - uses: actions/checkout@v3 - - uses: actions/setup-go@v3 + - uses: actions/setup-go@v4 with: go-version: 1.${{ matrix.go }}.x cache: true From 5c962a221e70fd6b12296e5d7075f28b422f98b2 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 16 Oct 2023 10:50:33 +0000 Subject: [PATCH 03/10] build(deps): bump github.com/cpuguy83/go-md2man/v2 from 2.0.2 to 2.0.3 (#2047) --- go.mod | 2 +- go.sum | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/go.mod b/go.mod index 6361d742..a79e66a1 100644 --- a/go.mod +++ b/go.mod @@ -3,7 +3,7 @@ module github.com/spf13/cobra go 1.15 require ( - github.com/cpuguy83/go-md2man/v2 v2.0.2 + github.com/cpuguy83/go-md2man/v2 v2.0.3 github.com/inconshreveable/mousetrap v1.1.0 github.com/spf13/pflag v1.0.5 gopkg.in/yaml.v3 v3.0.1 diff --git a/go.sum b/go.sum index 5ccb69dd..871c3a8a 100644 --- a/go.sum +++ b/go.sum @@ -1,5 +1,5 @@ -github.com/cpuguy83/go-md2man/v2 v2.0.2 h1:p1EgwI/C7NhT0JmVkwCD2ZBK8j4aeHQX2pMHHBfMQ6w= -github.com/cpuguy83/go-md2man/v2 v2.0.2/go.mod h1:tgQtvFlXSQOSOSIRvRPT7W67SCa46tRHOmNcaadrF8o= +github.com/cpuguy83/go-md2man/v2 v2.0.3 h1:qMCsGGgs+MAzDFyp9LpAe1Lqy/fY/qCovCm0qnXZOBM= +github.com/cpuguy83/go-md2man/v2 v2.0.3/go.mod h1:tgQtvFlXSQOSOSIRvRPT7W67SCa46tRHOmNcaadrF8o= github.com/inconshreveable/mousetrap v1.1.0 h1:wN+x4NVGpMsO7ErUn/mUI3vEoE6Jt13X2s0bqwp9tc8= github.com/inconshreveable/mousetrap v1.1.0/go.mod h1:vpF70FUmC8bwa3OWnCshd2FqLfsEA9PFc4w1p2J65bw= github.com/russross/blackfriday/v2 v2.1.0 h1:JIOH55/0cWyOuilr9/qlrm0BSXldqnqwMsf35Ld67mk= From 4cafa37bc4bb85633b4245aa118280fe5a9edcd5 Mon Sep 17 00:00:00 2001 From: vkhoroz Date: Sun, 22 Oct 2023 03:36:12 +0300 Subject: [PATCH 04/10] Allow running persistent run hooks of all parents (#2044) Currently, only one of the persistent pre-runs and post-runs is executed. It is always the first one found in the parents chain, starting at this command. Expected behavior is to execute all parents' persistent pre-runs and post-runs. Dependent projects implemented various workarounds for this: - manually building persistent hook chains (in every hook). - applying some kind of monkey-patching on top of Cobra. This change eliminates the necessity for such workarounds by allowing to set a global variable EnableTraverseRunHooks. Tickets: - https://github.com/spf13/cobra/issues/216 - https://github.com/spf13/cobra/issues/252 Signed-off-by: Volodymyr Khoroz --- cobra.go | 11 ++-- command.go | 28 ++++++++-- command_test.go | 106 ++++++++++++++++--------------------- site/content/user_guide.md | 4 ++ 4 files changed, 83 insertions(+), 66 deletions(-) diff --git a/cobra.go b/cobra.go index f23f5092..a6b160ce 100644 --- a/cobra.go +++ b/cobra.go @@ -43,9 +43,10 @@ var initializers []func() var finalizers []func() const ( - defaultPrefixMatching = false - defaultCommandSorting = true - defaultCaseInsensitive = false + defaultPrefixMatching = false + defaultCommandSorting = true + defaultCaseInsensitive = false + defaultTraverseRunHooks = false ) // EnablePrefixMatching allows setting automatic prefix matching. Automatic prefix matching can be a dangerous thing @@ -60,6 +61,10 @@ var EnableCommandSorting = defaultCommandSorting // EnableCaseInsensitive allows case-insensitive commands names. (case sensitive by default) var EnableCaseInsensitive = defaultCaseInsensitive +// EnableTraverseRunHooks executes persistent pre-run and post-run hooks from all parents. +// By default this is disabled, which means only the first run hook to be found is executed. +var EnableTraverseRunHooks = defaultTraverseRunHooks + // MousetrapHelpText enables an information splash screen on Windows // if the CLI is started from explorer.exe. // To disable the mousetrap, just set this variable to blank string (""). diff --git a/command.go b/command.go index 909d8585..ae3e4e00 100644 --- a/command.go +++ b/command.go @@ -934,15 +934,31 @@ func (c *Command) execute(a []string) (err error) { return err } + parents := make([]*Command, 0, 5) for p := c; p != nil; p = p.Parent() { + if EnableTraverseRunHooks { + // When EnableTraverseRunHooks is set: + // - Execute all persistent pre-runs from the root parent till this command. + // - Execute all persistent post-runs from this command till the root parent. + parents = append([]*Command{p}, parents...) + } else { + // Otherwise, execute only the first found persistent hook. + parents = append(parents, p) + } + } + for _, p := range parents { if p.PersistentPreRunE != nil { if err := p.PersistentPreRunE(c, argWoFlags); err != nil { return err } - break + if !EnableTraverseRunHooks { + break + } } else if p.PersistentPreRun != nil { p.PersistentPreRun(c, argWoFlags) - break + if !EnableTraverseRunHooks { + break + } } } if c.PreRunE != nil { @@ -979,10 +995,14 @@ func (c *Command) execute(a []string) (err error) { if err := p.PersistentPostRunE(c, argWoFlags); err != nil { return err } - break + if !EnableTraverseRunHooks { + break + } } else if p.PersistentPostRun != nil { p.PersistentPostRun(c, argWoFlags) - break + if !EnableTraverseRunHooks { + break + } } } diff --git a/command_test.go b/command_test.go index 4afb7f7b..4d8908d2 100644 --- a/command_test.go +++ b/command_test.go @@ -1530,57 +1530,73 @@ func TestHooks(t *testing.T) { } func TestPersistentHooks(t *testing.T) { - var ( - parentPersPreArgs string - parentPreArgs string - parentRunArgs string - parentPostArgs string - parentPersPostArgs string - ) + EnableTraverseRunHooks = true + testPersistentHooks(t, []string{ + "parent PersistentPreRun", + "child PersistentPreRun", + "child PreRun", + "child Run", + "child PostRun", + "child PersistentPostRun", + "parent PersistentPostRun", + }) - var ( - childPersPreArgs string - childPreArgs string - childRunArgs string - childPostArgs string - childPersPostArgs string - ) + EnableTraverseRunHooks = false + testPersistentHooks(t, []string{ + "child PersistentPreRun", + "child PreRun", + "child Run", + "child PostRun", + "child PersistentPostRun", + }) +} + +func testPersistentHooks(t *testing.T, expectedHookRunOrder []string) { + var hookRunOrder []string + + validateHook := func(args []string, hookName string) { + hookRunOrder = append(hookRunOrder, hookName) + got := strings.Join(args, " ") + if onetwo != got { + t.Errorf("Expected %s %q, got %q", hookName, onetwo, got) + } + } parentCmd := &Command{ Use: "parent", PersistentPreRun: func(_ *Command, args []string) { - parentPersPreArgs = strings.Join(args, " ") + validateHook(args, "parent PersistentPreRun") }, PreRun: func(_ *Command, args []string) { - parentPreArgs = strings.Join(args, " ") + validateHook(args, "parent PreRun") }, Run: func(_ *Command, args []string) { - parentRunArgs = strings.Join(args, " ") + validateHook(args, "parent Run") }, PostRun: func(_ *Command, args []string) { - parentPostArgs = strings.Join(args, " ") + validateHook(args, "parent PostRun") }, PersistentPostRun: func(_ *Command, args []string) { - parentPersPostArgs = strings.Join(args, " ") + validateHook(args, "parent PersistentPostRun") }, } childCmd := &Command{ Use: "child", PersistentPreRun: func(_ *Command, args []string) { - childPersPreArgs = strings.Join(args, " ") + validateHook(args, "child PersistentPreRun") }, PreRun: func(_ *Command, args []string) { - childPreArgs = strings.Join(args, " ") + validateHook(args, "child PreRun") }, Run: func(_ *Command, args []string) { - childRunArgs = strings.Join(args, " ") + validateHook(args, "child Run") }, PostRun: func(_ *Command, args []string) { - childPostArgs = strings.Join(args, " ") + validateHook(args, "child PostRun") }, PersistentPostRun: func(_ *Command, args []string) { - childPersPostArgs = strings.Join(args, " ") + validateHook(args, "child PersistentPostRun") }, } parentCmd.AddCommand(childCmd) @@ -1593,41 +1609,13 @@ func TestPersistentHooks(t *testing.T) { t.Errorf("Unexpected error: %v", err) } - for _, v := range []struct { - name string - got string - }{ - // TODO: currently PersistentPreRun* defined in parent does not - // run if the matching child subcommand has PersistentPreRun. - // If the behavior changes (https://github.com/spf13/cobra/issues/252) - // this test must be fixed. - {"parentPersPreArgs", parentPersPreArgs}, - {"parentPreArgs", parentPreArgs}, - {"parentRunArgs", parentRunArgs}, - {"parentPostArgs", parentPostArgs}, - // TODO: currently PersistentPostRun* defined in parent does not - // run if the matching child subcommand has PersistentPostRun. - // If the behavior changes (https://github.com/spf13/cobra/issues/252) - // this test must be fixed. - {"parentPersPostArgs", parentPersPostArgs}, - } { - if v.got != "" { - t.Errorf("Expected blank %s, got %q", v.name, v.got) - } - } - - for _, v := range []struct { - name string - got string - }{ - {"childPersPreArgs", childPersPreArgs}, - {"childPreArgs", childPreArgs}, - {"childRunArgs", childRunArgs}, - {"childPostArgs", childPostArgs}, - {"childPersPostArgs", childPersPostArgs}, - } { - if v.got != onetwo { - t.Errorf("Expected %s %q, got %q", v.name, onetwo, v.got) + for idx, exp := range expectedHookRunOrder { + if len(hookRunOrder) > idx { + if act := hookRunOrder[idx]; act != exp { + t.Errorf("Expected %q at %d, got %q", exp, idx, act) + } + } else { + t.Errorf("Expected %q at %d, got nothing", exp, idx) } } } diff --git a/site/content/user_guide.md b/site/content/user_guide.md index 55cc252b..4116e8dc 100644 --- a/site/content/user_guide.md +++ b/site/content/user_guide.md @@ -687,6 +687,10 @@ Inside subCmd PostRun with args: [arg1 arg2] Inside subCmd PersistentPostRun with args: [arg1 arg2] ``` +By default, only the first persistent hook found in the command chain is executed. +That is why in the above output, the `rootCmd PersistentPostRun` was not called for a child command. +Set `EnableTraverseRunHooks` global variable to `true` if you want to execute all parents' persistent hooks. + ## Suggestions when "unknown command" happens Cobra will print automatic suggestions when "unknown command" errors happen. This allows Cobra to behave similarly to the `git` command when a typo happens. For example: From 8b1eba47616566fc4d258a93da48d5d8741865f0 Mon Sep 17 00:00:00 2001 From: Marc Khouzam Date: Fri, 27 Oct 2023 06:23:45 -0400 Subject: [PATCH 05/10] Fix linter errors (#2052) When using golangci-lint v1.55.0 some new errors were being reported. Signed-off-by: Marc Khouzam --- command.go | 1 + doc/md_docs.go | 8 +++++--- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/command.go b/command.go index ae3e4e00..36494df5 100644 --- a/command.go +++ b/command.go @@ -1446,6 +1446,7 @@ func (c *Command) UseLine() string { // DebugFlags used to determine which flags have been assigned to which commands // and which persist. +// nolint:goconst func (c *Command) DebugFlags() { c.Println("DebugFlags called on", c.Name()) var debugflags func(*Command) diff --git a/doc/md_docs.go b/doc/md_docs.go index c4a27c00..f98fe2a3 100644 --- a/doc/md_docs.go +++ b/doc/md_docs.go @@ -27,6 +27,8 @@ import ( "github.com/spf13/cobra" ) +const markdownExtension = ".md" + func printOptions(buf *bytes.Buffer, cmd *cobra.Command, name string) error { flags := cmd.NonInheritedFlags() flags.SetOutput(buf) @@ -83,7 +85,7 @@ func GenMarkdownCustom(cmd *cobra.Command, w io.Writer, linkHandler func(string) if cmd.HasParent() { parent := cmd.Parent() pname := parent.CommandPath() - link := pname + ".md" + link := pname + markdownExtension link = strings.ReplaceAll(link, " ", "_") buf.WriteString(fmt.Sprintf("* [%s](%s)\t - %s\n", pname, linkHandler(link), parent.Short)) cmd.VisitParents(func(c *cobra.Command) { @@ -101,7 +103,7 @@ func GenMarkdownCustom(cmd *cobra.Command, w io.Writer, linkHandler func(string) continue } cname := name + " " + child.Name() - link := cname + ".md" + link := cname + markdownExtension link = strings.ReplaceAll(link, " ", "_") buf.WriteString(fmt.Sprintf("* [%s](%s)\t - %s\n", cname, linkHandler(link), child.Short)) } @@ -138,7 +140,7 @@ func GenMarkdownTreeCustom(cmd *cobra.Command, dir string, filePrepender, linkHa } } - basename := strings.ReplaceAll(cmd.CommandPath(), " ", "_") + ".md" + basename := strings.ReplaceAll(cmd.CommandPath(), " ", "_") + markdownExtension filename := filepath.Join(dir, basename) f, err := os.Create(filename) if err != nil { From b711e8760b73c6aa1b4aa1bef3a26da5926f175d Mon Sep 17 00:00:00 2001 From: Marc Khouzam Date: Sat, 28 Oct 2023 16:10:06 -0400 Subject: [PATCH 06/10] Don't complete --help flag when flag parsing disabled (#2061) Fixes #2060 When a command sets `DisableFlagParsing = true` it requests the responsibility of doing all the flag parsing. Therefore even the `--help/-f/--version/-v` flags should not be automatically completed by Cobra in such a case. Without this change the `--help/-h/--version/-v` flags can end up being completed twice for plugins: one time from cobra and one time from the plugin (which has set `DisableFlagParsing = true`). Signed-off-by: Marc Khouzam --- completions.go | 15 ++++++++++++--- completions_test.go | 46 ++++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 55 insertions(+), 6 deletions(-) diff --git a/completions.go b/completions.go index 55a9f62c..368b92a9 100644 --- a/completions.go +++ b/completions.go @@ -302,9 +302,13 @@ func (c *Command) getCompletions(args []string) (*Command, []string, ShellCompDi // These flags are normally added when `execute()` is called on `finalCmd`, // however, when doing completion, we don't call `finalCmd.execute()`. - // Let's add the --help and --version flag ourselves. - finalCmd.InitDefaultHelpFlag() - finalCmd.InitDefaultVersionFlag() + // Let's add the --help and --version flag ourselves but only if the finalCmd + // has not disabled flag parsing; if flag parsing is disabled, it is up to the + // finalCmd itself to handle the completion of *all* flags. + if !finalCmd.DisableFlagParsing { + finalCmd.InitDefaultHelpFlag() + finalCmd.InitDefaultVersionFlag() + } // Check if we are doing flag value completion before parsing the flags. // This is important because if we are completing a flag value, we need to also @@ -408,6 +412,11 @@ func (c *Command) getCompletions(args []string) (*Command, []string, ShellCompDi finalCmd.InheritedFlags().VisitAll(func(flag *pflag.Flag) { doCompleteFlags(flag) }) + // Try to complete non-inherited flags even if DisableFlagParsing==true. + // This allows programs to tell Cobra about flags for completion even + // if the actual parsing of flags is not done by Cobra. + // For instance, Helm uses this to provide flag name completion for + // some of its plugins. finalCmd.NonInheritedFlags().VisitAll(func(flag *pflag.Flag) { doCompleteFlags(flag) }) diff --git a/completions_test.go b/completions_test.go index 01724660..4c6f41bd 100644 --- a/completions_test.go +++ b/completions_test.go @@ -2622,8 +2622,6 @@ func TestCompleteWithDisableFlagParsing(t *testing.T) { expected := strings.Join([]string{ "--persistent", "-p", - "--help", - "-h", "--nonPersistent", "-n", "--flag", @@ -3053,8 +3051,26 @@ func TestCompletionCobraFlags(t *testing.T) { return []string{"extra3"}, ShellCompDirectiveNoFileComp }, } + childCmd4 := &Command{ + Use: "child4", + Version: "1.1.1", + Run: emptyRun, + ValidArgsFunction: func(cmd *Command, args []string, toComplete string) ([]string, ShellCompDirective) { + return []string{"extra4"}, ShellCompDirectiveNoFileComp + }, + DisableFlagParsing: true, + } + childCmd5 := &Command{ + Use: "child5", + Version: "1.1.1", + Run: emptyRun, + ValidArgsFunction: func(cmd *Command, args []string, toComplete string) ([]string, ShellCompDirective) { + return []string{"extra5"}, ShellCompDirectiveNoFileComp + }, + DisableFlagParsing: true, + } - rootCmd.AddCommand(childCmd, childCmd2, childCmd3) + rootCmd.AddCommand(childCmd, childCmd2, childCmd3, childCmd4, childCmd5) _ = childCmd.Flags().Bool("bool", false, "A bool flag") _ = childCmd.MarkFlagRequired("bool") @@ -3066,6 +3082,10 @@ func TestCompletionCobraFlags(t *testing.T) { // Have a command that only adds its own -v flag _ = childCmd3.Flags().BoolP("verbose", "v", false, "Not a version flag") + // Have a command that DisablesFlagParsing but that also adds its own help and version flags + _ = childCmd5.Flags().BoolP("help", "h", false, "My own help") + _ = childCmd5.Flags().BoolP("version", "v", false, "My own version") + return rootCmd } @@ -3196,6 +3216,26 @@ func TestCompletionCobraFlags(t *testing.T) { ":4", "Completion ended with directive: ShellCompDirectiveNoFileComp", ""}, "\n"), }, + { + desc: "no completion for --help/-h and --version/-v flags when DisableFlagParsing=true", + args: []string{"child4", "-"}, + expectedOutput: strings.Join([]string{ + "extra4", + ":4", + "Completion ended with directive: ShellCompDirectiveNoFileComp", ""}, "\n"), + }, + { + desc: "completions for program-defined --help/-h and --version/-v flags even when DisableFlagParsing=true", + args: []string{"child5", "-"}, + expectedOutput: strings.Join([]string{ + "--help", + "-h", + "--version", + "-v", + "extra5", + ":4", + "Completion ended with directive: ShellCompDirectiveNoFileComp", ""}, "\n"), + }, } for _, tc := range testcases { From 00b68a1c260eaf2b9bcb10a3178d36cec81548ca Mon Sep 17 00:00:00 2001 From: Marc Khouzam Date: Sat, 28 Oct 2023 16:11:59 -0400 Subject: [PATCH 07/10] Add tests for flag completion registration (#2053) Different problems have been reported about flag completion registration. These two tests are the cases that were not being verified but had been mentioned as problematic. Ref: - https://github.com/spf13/cobra/issues/1320 - https://github.com/spf13/cobra/pull/1438#issuecomment-872928669 Signed-off-by: Marc Khouzam --- completions_test.go | 110 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 110 insertions(+) diff --git a/completions_test.go b/completions_test.go index 4c6f41bd..7585f88c 100644 --- a/completions_test.go +++ b/completions_test.go @@ -17,7 +17,9 @@ package cobra import ( "bytes" "context" + "fmt" "strings" + "sync" "testing" ) @@ -2040,6 +2042,114 @@ func TestFlagCompletionWorksRootCommandAddedAfterFlags(t *testing.T) { } } +func TestFlagCompletionForPersistentFlagsCalledFromSubCmd(t *testing.T) { + rootCmd := &Command{Use: "root", Run: emptyRun} + rootCmd.PersistentFlags().String("string", "", "test string flag") + _ = rootCmd.RegisterFlagCompletionFunc("string", func(cmd *Command, args []string, toComplete string) ([]string, ShellCompDirective) { + return []string{"myval"}, ShellCompDirectiveDefault + }) + + childCmd := &Command{ + Use: "child", + Run: emptyRun, + ValidArgsFunction: func(cmd *Command, args []string, toComplete string) ([]string, ShellCompDirective) { + return []string{"--validarg", "test"}, ShellCompDirectiveDefault + }, + } + childCmd.Flags().Bool("bool", false, "test bool flag") + rootCmd.AddCommand(childCmd) + + // Test that persistent flag completion works for the subcmd + output, err := executeCommand(rootCmd, ShellCompRequestCmd, "child", "--string", "") + if err != nil { + t.Errorf("Unexpected error: %v", err) + } + + expected := strings.Join([]string{ + "myval", + ":0", + "Completion ended with directive: ShellCompDirectiveDefault", ""}, "\n") + + if output != expected { + t.Errorf("expected: %q, got: %q", expected, output) + } +} + +// This test tries to register flag completion concurrently to make sure the +// code handles concurrency properly. +// This was reported as a problem when tests are run concurrently: +// https://github.com/spf13/cobra/issues/1320 +// +// NOTE: this test can sometimes pass even if the code were to not handle +// concurrency properly. This is not great but the important part is that +// it should never fail. Therefore, if the tests fails sometimes, we will +// still be able to know there is a problem. +func TestFlagCompletionConcurrentRegistration(t *testing.T) { + rootCmd := &Command{Use: "root", Run: emptyRun} + const maxFlags = 50 + for i := 1; i < maxFlags; i += 2 { + flagName := fmt.Sprintf("flag%d", i) + rootCmd.Flags().String(flagName, "", fmt.Sprintf("test %s flag on root", flagName)) + } + + childCmd := &Command{ + Use: "child", + Run: emptyRun, + } + for i := 2; i <= maxFlags; i += 2 { + flagName := fmt.Sprintf("flag%d", i) + childCmd.Flags().String(flagName, "", fmt.Sprintf("test %s flag on child", flagName)) + } + + rootCmd.AddCommand(childCmd) + + // Register completion in different threads to test concurrency. + var wg sync.WaitGroup + for i := 1; i <= maxFlags; i++ { + index := i + flagName := fmt.Sprintf("flag%d", i) + wg.Add(1) + go func() { + defer wg.Done() + cmd := rootCmd + if index%2 == 0 { + cmd = childCmd + } + _ = cmd.RegisterFlagCompletionFunc(flagName, func(cmd *Command, args []string, toComplete string) ([]string, ShellCompDirective) { + return []string{fmt.Sprintf("flag%d", index)}, ShellCompDirectiveDefault + }) + }() + } + + wg.Wait() + + // Test that flag completion works for each flag + for i := 1; i <= 6; i++ { + var output string + var err error + flagName := fmt.Sprintf("flag%d", i) + + if i%2 == 1 { + output, err = executeCommand(rootCmd, ShellCompRequestCmd, "--"+flagName, "") + } else { + output, err = executeCommand(rootCmd, ShellCompRequestCmd, "child", "--"+flagName, "") + } + + if err != nil { + t.Errorf("Unexpected error: %v", err) + } + + expected := strings.Join([]string{ + flagName, + ":0", + "Completion ended with directive: ShellCompDirectiveDefault", ""}, "\n") + + if output != expected { + t.Errorf("expected: %q, got: %q", expected, output) + } + } +} + func TestFlagCompletionInGoWithDesc(t *testing.T) { rootCmd := &Command{ Use: "root", From 22953d88453ec9343b4a78b9d67400a3326f3138 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ville=20Skytt=C3=A4?= Date: Sun, 29 Oct 2023 20:06:51 +0200 Subject: [PATCH 08/10] Replace all non-alphanumerics in active help env var program prefix (#1940) * Replace all non-alphanumerics in active help env var program prefix There are other characters besides the dash that are fine in program names, but are problematic in environment variable names. These include (but are not limited to) period, space, and non-ASCII letters. * Another change in docs to mention non-ASCII-alphanumeric instead of just dash --- active_help.go | 10 +++++++--- site/content/active_help.md | 4 ++-- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/active_help.go b/active_help.go index 2d023943..5f965e05 100644 --- a/active_help.go +++ b/active_help.go @@ -17,6 +17,7 @@ package cobra import ( "fmt" "os" + "regexp" "strings" ) @@ -29,6 +30,8 @@ const ( activeHelpGlobalDisable = "0" ) +var activeHelpEnvVarPrefixSubstRegexp = regexp.MustCompile(`[^A-Z0-9_]`) + // AppendActiveHelp adds the specified string to the specified array to be used as ActiveHelp. // Such strings will be processed by the completion script and will be shown as ActiveHelp // to the user. @@ -42,7 +45,7 @@ func AppendActiveHelp(compArray []string, activeHelpStr string) []string { // GetActiveHelpConfig returns the value of the ActiveHelp environment variable // _ACTIVE_HELP where is the name of the root command in upper -// case, with all - replaced by _. +// case, with all non-ASCII-alphanumeric characters replaced by `_`. // It will always return "0" if the global environment variable COBRA_ACTIVE_HELP // is set to "0". func GetActiveHelpConfig(cmd *Command) string { @@ -55,9 +58,10 @@ func GetActiveHelpConfig(cmd *Command) string { // activeHelpEnvVar returns the name of the program-specific ActiveHelp environment // variable. It has the format _ACTIVE_HELP where is the name of the -// root command in upper case, with all - replaced by _. +// root command in upper case, with all non-ASCII-alphanumeric characters replaced by `_`. func activeHelpEnvVar(name string) string { // This format should not be changed: users will be using it explicitly. activeHelpEnvVar := strings.ToUpper(fmt.Sprintf("%s%s", name, activeHelpEnvVarSuffix)) - return strings.ReplaceAll(activeHelpEnvVar, "-", "_") + activeHelpEnvVar = activeHelpEnvVarPrefixSubstRegexp.ReplaceAllString(activeHelpEnvVar, "_") + return activeHelpEnvVar } diff --git a/site/content/active_help.md b/site/content/active_help.md index 5e7f59af..d72acc72 100644 --- a/site/content/active_help.md +++ b/site/content/active_help.md @@ -92,7 +92,7 @@ Allowing to configure Active Help is entirely optional; you can use Active Help The way to configure Active Help is to use the program's Active Help environment variable. That variable is named `_ACTIVE_HELP` where `` is the name of your -program in uppercase with any `-` replaced by an `_`. The variable should be set by the user to whatever +program in uppercase with any non-ASCII-alphanumeric characters replaced by an `_`. The variable should be set by the user to whatever Active Help configuration values are supported by the program. For example, say `helm` has chosen to support three levels for Active Help: `on`, `off`, `local`. Then a user @@ -140,7 +140,7 @@ details for your users. Debugging your Active Help code is done in the same way as debugging your dynamic completion code, which is with Cobra's hidden `__complete` command. Please refer to [debugging shell completion](shell_completions.md#debugging) for details. -When debugging with the `__complete` command, if you want to specify different Active Help configurations, you should use the active help environment variable. That variable is named `_ACTIVE_HELP` where any `-` is replaced by an `_`. For example, we can test deactivating some Active Help as shown below: +When debugging with the `__complete` command, if you want to specify different Active Help configurations, you should use the active help environment variable. That variable is named `_ACTIVE_HELP` where any non-ASCII-alphanumeric characters are replaced by an `_`. For example, we can test deactivating some Active Help as shown below: ``` $ HELM_ACTIVE_HELP=1 bin/helm __complete install wordpress bitnami/h bitnami/haproxy From 48cea5c87b5299b68c3f5b7f2c67ea948717276f Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 30 Oct 2023 10:21:48 +0000 Subject: [PATCH 09/10] build(deps): bump actions/checkout from 3 to 4 (#2028) --- .github/workflows/test.yml | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 55085eaf..a9245322 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -18,7 +18,7 @@ jobs: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v3 + - uses: actions/checkout@v4 - run: >- docker run @@ -39,7 +39,7 @@ jobs: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v3 + - uses: actions/checkout@v4 - uses: actions/setup-go@v4 with: @@ -47,7 +47,7 @@ jobs: check-latest: true cache: true - - uses: actions/checkout@v3 + - uses: actions/checkout@v4 - uses: golangci/golangci-lint-action@v3.7.0 with: @@ -72,7 +72,7 @@ jobs: runs-on: ${{ matrix.platform }}-latest steps: - - uses: actions/checkout@v3 + - uses: actions/checkout@v4 - uses: actions/setup-go@v4 with: @@ -108,7 +108,7 @@ jobs: unzip mingw-w64-x86_64-go - - uses: actions/checkout@v3 + - uses: actions/checkout@v4 - uses: actions/cache@v3 with: From 890302a35f578311404a462b3cdd404f34db3720 Mon Sep 17 00:00:00 2001 From: Nir Soffer Date: Thu, 2 Nov 2023 14:15:26 +0200 Subject: [PATCH 10/10] Support usage as plugin for tools like kubectl (#2018) In this case the executable is `kubectl-plugin`, but we run it as: kubectl plugin And the help text should reflect the actual usage of the command. To create a plugin, add the cobra.CommandDisplayNameAnnotation: rootCmd := &cobra.Command{ Use: "plugin", Annotations: map[string]string{ cobra.CommandDisplayNameAnnotation: "kubectl plugin", } } Internally this change modifies CommandPath() for the root command to return the command display name instead of the command name. This is used for error messages, help text generation, and completions. CommandPath() is expected to have spaces and code using it already handle spaces (e.g replacing with _), so hopefully this does not break anything. Fixes: #2017 Signed-off-by: Nir Soffer --- command.go | 10 ++++++++-- command_test.go | 30 ++++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 2 deletions(-) diff --git a/command.go b/command.go index 36494df5..2fbe6c13 100644 --- a/command.go +++ b/command.go @@ -30,7 +30,10 @@ import ( flag "github.com/spf13/pflag" ) -const FlagSetByCobraAnnotation = "cobra_annotation_flag_set_by_cobra" +const ( + FlagSetByCobraAnnotation = "cobra_annotation_flag_set_by_cobra" + CommandDisplayNameAnnotation = "cobra_annotation_command_display_name" +) // FParseErrWhitelist configures Flag parse errors to be ignored type FParseErrWhitelist flag.ParseErrorsWhitelist @@ -99,7 +102,7 @@ type Command struct { Deprecated string // Annotations are key/value pairs that can be used by applications to identify or - // group commands. + // group commands or set special options. Annotations map[string]string // Version defines the version for this command. If this value is non-empty and the command does not @@ -1424,6 +1427,9 @@ func (c *Command) CommandPath() string { if c.HasParent() { return c.Parent().CommandPath() + " " + c.Name() } + if displayName, ok := c.Annotations[CommandDisplayNameAnnotation]; ok { + return displayName + } return c.Name() } diff --git a/command_test.go b/command_test.go index 4d8908d2..9f686d65 100644 --- a/command_test.go +++ b/command_test.go @@ -366,6 +366,36 @@ func TestAliasPrefixMatching(t *testing.T) { EnablePrefixMatching = defaultPrefixMatching } +// TestPlugin checks usage as plugin for another command such as kubectl. The +// executable is `kubectl-plugin`, but we run it as `kubectl plugin`. The help +// text should reflect the way we run the command. +func TestPlugin(t *testing.T) { + rootCmd := &Command{ + Use: "plugin", + Args: NoArgs, + Annotations: map[string]string{ + CommandDisplayNameAnnotation: "kubectl plugin", + }, + } + + subCmd := &Command{Use: "sub [flags]", Args: NoArgs, Run: emptyRun} + rootCmd.AddCommand(subCmd) + + rootHelp, err := executeCommand(rootCmd, "-h") + if err != nil { + t.Errorf("Unexpected error: %v", err) + } + + checkStringContains(t, rootHelp, "kubectl plugin [command]") + + childHelp, err := executeCommand(rootCmd, "sub", "-h") + if err != nil { + t.Errorf("Unexpected error: %v", err) + } + + checkStringContains(t, childHelp, "kubectl plugin sub [flags]") +} + // TestChildSameName checks the correct behaviour of cobra in cases, // when an application with name "foo" and with subcommand "foo" // is executed with args "foo foo".