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 <volodymyr.khoroz@foundries.io>
This commit is contained in:
vkhoroz 2023-10-22 03:36:12 +03:00 committed by GitHub
parent 5c962a221e
commit 4cafa37bc4
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
4 changed files with 83 additions and 66 deletions

View file

@ -43,9 +43,10 @@ var initializers []func()
var finalizers []func() var finalizers []func()
const ( const (
defaultPrefixMatching = false defaultPrefixMatching = false
defaultCommandSorting = true defaultCommandSorting = true
defaultCaseInsensitive = false defaultCaseInsensitive = false
defaultTraverseRunHooks = false
) )
// EnablePrefixMatching allows setting automatic prefix matching. Automatic prefix matching can be a dangerous thing // 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) // EnableCaseInsensitive allows case-insensitive commands names. (case sensitive by default)
var EnableCaseInsensitive = defaultCaseInsensitive 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 // MousetrapHelpText enables an information splash screen on Windows
// if the CLI is started from explorer.exe. // if the CLI is started from explorer.exe.
// To disable the mousetrap, just set this variable to blank string (""). // To disable the mousetrap, just set this variable to blank string ("").

View file

@ -934,15 +934,31 @@ func (c *Command) execute(a []string) (err error) {
return err return err
} }
parents := make([]*Command, 0, 5)
for p := c; p != nil; p = p.Parent() { 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 p.PersistentPreRunE != nil {
if err := p.PersistentPreRunE(c, argWoFlags); err != nil { if err := p.PersistentPreRunE(c, argWoFlags); err != nil {
return err return err
} }
break if !EnableTraverseRunHooks {
break
}
} else if p.PersistentPreRun != nil { } else if p.PersistentPreRun != nil {
p.PersistentPreRun(c, argWoFlags) p.PersistentPreRun(c, argWoFlags)
break if !EnableTraverseRunHooks {
break
}
} }
} }
if c.PreRunE != nil { if c.PreRunE != nil {
@ -979,10 +995,14 @@ func (c *Command) execute(a []string) (err error) {
if err := p.PersistentPostRunE(c, argWoFlags); err != nil { if err := p.PersistentPostRunE(c, argWoFlags); err != nil {
return err return err
} }
break if !EnableTraverseRunHooks {
break
}
} else if p.PersistentPostRun != nil { } else if p.PersistentPostRun != nil {
p.PersistentPostRun(c, argWoFlags) p.PersistentPostRun(c, argWoFlags)
break if !EnableTraverseRunHooks {
break
}
} }
} }

View file

@ -1530,57 +1530,73 @@ func TestHooks(t *testing.T) {
} }
func TestPersistentHooks(t *testing.T) { func TestPersistentHooks(t *testing.T) {
var ( EnableTraverseRunHooks = true
parentPersPreArgs string testPersistentHooks(t, []string{
parentPreArgs string "parent PersistentPreRun",
parentRunArgs string "child PersistentPreRun",
parentPostArgs string "child PreRun",
parentPersPostArgs string "child Run",
) "child PostRun",
"child PersistentPostRun",
"parent PersistentPostRun",
})
var ( EnableTraverseRunHooks = false
childPersPreArgs string testPersistentHooks(t, []string{
childPreArgs string "child PersistentPreRun",
childRunArgs string "child PreRun",
childPostArgs string "child Run",
childPersPostArgs string "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{ parentCmd := &Command{
Use: "parent", Use: "parent",
PersistentPreRun: func(_ *Command, args []string) { PersistentPreRun: func(_ *Command, args []string) {
parentPersPreArgs = strings.Join(args, " ") validateHook(args, "parent PersistentPreRun")
}, },
PreRun: func(_ *Command, args []string) { PreRun: func(_ *Command, args []string) {
parentPreArgs = strings.Join(args, " ") validateHook(args, "parent PreRun")
}, },
Run: func(_ *Command, args []string) { Run: func(_ *Command, args []string) {
parentRunArgs = strings.Join(args, " ") validateHook(args, "parent Run")
}, },
PostRun: func(_ *Command, args []string) { PostRun: func(_ *Command, args []string) {
parentPostArgs = strings.Join(args, " ") validateHook(args, "parent PostRun")
}, },
PersistentPostRun: func(_ *Command, args []string) { PersistentPostRun: func(_ *Command, args []string) {
parentPersPostArgs = strings.Join(args, " ") validateHook(args, "parent PersistentPostRun")
}, },
} }
childCmd := &Command{ childCmd := &Command{
Use: "child", Use: "child",
PersistentPreRun: func(_ *Command, args []string) { PersistentPreRun: func(_ *Command, args []string) {
childPersPreArgs = strings.Join(args, " ") validateHook(args, "child PersistentPreRun")
}, },
PreRun: func(_ *Command, args []string) { PreRun: func(_ *Command, args []string) {
childPreArgs = strings.Join(args, " ") validateHook(args, "child PreRun")
}, },
Run: func(_ *Command, args []string) { Run: func(_ *Command, args []string) {
childRunArgs = strings.Join(args, " ") validateHook(args, "child Run")
}, },
PostRun: func(_ *Command, args []string) { PostRun: func(_ *Command, args []string) {
childPostArgs = strings.Join(args, " ") validateHook(args, "child PostRun")
}, },
PersistentPostRun: func(_ *Command, args []string) { PersistentPostRun: func(_ *Command, args []string) {
childPersPostArgs = strings.Join(args, " ") validateHook(args, "child PersistentPostRun")
}, },
} }
parentCmd.AddCommand(childCmd) parentCmd.AddCommand(childCmd)
@ -1593,41 +1609,13 @@ func TestPersistentHooks(t *testing.T) {
t.Errorf("Unexpected error: %v", err) t.Errorf("Unexpected error: %v", err)
} }
for _, v := range []struct { for idx, exp := range expectedHookRunOrder {
name string if len(hookRunOrder) > idx {
got string if act := hookRunOrder[idx]; act != exp {
}{ t.Errorf("Expected %q at %d, got %q", exp, idx, act)
// TODO: currently PersistentPreRun* defined in parent does not }
// run if the matching child subcommand has PersistentPreRun. } else {
// If the behavior changes (https://github.com/spf13/cobra/issues/252) t.Errorf("Expected %q at %d, got nothing", exp, idx)
// 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)
} }
} }
} }

View file

@ -687,6 +687,10 @@ Inside subCmd PostRun with args: [arg1 arg2]
Inside subCmd PersistentPostRun 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 ## 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: 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: