PersistentPreRun only allowed once

[xh3b4sd]

Hello there. I am playing around with a command that defines a PersistentPreRun. This command has a sub command. This sub command as well wants to use PersistentPreRun. When the root command is executed its PersistentPreRun is executed as expected. When executing the sub command I expected both PersistentPreRuns to be executed, but only the sub command's PersistentPreRun is executed. I expect this as a bug. What do you guys think? Thanks for listening.

[xh3b4sd]

FYI I can work around this using PreRun for the sub command.

[amanhigh]

Yes even i felt that PersistantPreRun chaining is missing hence code get duplicated in children in order to call previous Prerun.

I feel before leaf node execution all parent PersistentPreRun should be run in order.

[kurtpeek]

In my case, the parent is the rootCmd and its PersistentPreRun() doesn't actually use the cmd or args passed into it, so I was able to work around this by calling rootCmd.PersistentPreRun(cmd, args) explicitly in the child command:

	PersistentPreRun: func(cmd *cobra.Command, args []string) {
		rootCmd.PersistentPreRun(cmd, args)
		...
	},

[github-actions]

This issue is being marked as stale due to a long period of inactivity

[mateimicu]

I also have this use case, the PersistentPreRun of the rootCmd validates some input, and for a subcommand group, I validate another set of inputs.

I worked around this with

PersistentPreRunE: func(cmd *cobra.Command, args []string) error {
	root := cmd
	for ; root.HasParent(); root = root.Parent() {
	}
	err := root.PersistentPreRunE(cmd, args)
	...
}

I think changing now the implementations to run all the PersistentPreRunE would be a major breaking change ... If this topic develops I may have some time to contribute.

Maybe a flag can be introduced like TraverseChildren with the default on the current behaviors ?

[johnSchnake]

I agree with the idea that some people may want them chained, others not. The docs are clear that

The Persistent*Run functions will be inherited by children if they do not declare their own.

So at the least its by definition not a bug at this point.

I can imagine this getting complicated if you have a series of commands root childA childB and we introduce the ability to chain them (instead of overwrite) then childB may become dependent on root.PersistentPreRun but then its behavior gets broken if someone else changes childA.

The PR that @poy put up (#1253) introduces a new field to trigger this behavior so at least it isn't a breaking change. It does still get potentially weird when different commands set different values for TraverseChildrenHooks. I'm on the fence but this one did have a number of reactions and actually has someone with a PR so it should be considered.

[amanhigh]

I can imagine this getting complicated if you have a series of commands root childA childB and we introduce the ability to chain them (instead of overwrite) then childB may become dependent on root.PersistentPreRun but then its behavior gets broken if someone else changes childA.

I have a contrary opinion that chaining would simplify things. Cobra provides an inheritance structure which flows very naturally.
On those Lines there could be functions which are common to all commands below it where chaining helps.

Eg. Lets say we had a google CLI
root - Login to Google Auth
gmail - Prerun specific to gmail inherited by all commands
youtube - Prerun specific to all youtube subcommands.

In such case not having pre-run at root level would force all sub-commands to call login and so on below.

Secondly you are right this could complicate structure in some cases but then user has option to not implement persistent pre-run at root level. But now this option doesn't exist at all for a Cobra User.
Also if login changes at root it would anyways break logic for commands below even if cobra doesn't allow persistent pre-run below as this would be called in some way or other by some pre-run for command.

[nanmu42]

I personally think this feature is a little surprising. I thought PersistentPreRun were chained and run in the order of root command -> subcommand -> subsubcommand.

Perhaps we should give #1253 a shot?

[bdwyertech]

I agree that chaining is desired, at least toggle-able ala #1253. To be quite honest, I expected this was how it behaved, and only just realized it is not. Just to give an idea, this is my root PersistentPreRun:

PersistentPreRun: func(cmd *cobra.Command, args []string) {
	telemetry.SentryInit(cmd)
	viper.BindPFlags(cmd.Flags())
}

I have a few nested subcommands and only realized this top-level PersistentPreRun wasn't firing when I realized that cmd arguments weren't passing through to Viper.

[polothy]

Fixed in #2044