Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Persistent*Run of rootCmd will be skipped if its sub-command has the same Persistent*Run #1198

Closed
hyorigo opened this issue Aug 21, 2020 · 6 comments

Comments

@hyorigo
Copy link

hyorigo commented Aug 21, 2020

I spent some time with https://github.com/carolynvs/stingoftheviper , and I found the strange behavior ---

  • If only the PersistentPreRunE of rootCmd is defined, it will always be triggered.
  • If both the PersistentPreRunE of rootCmd and its sub-command, only the sub-command one will be triggered.

If I need the Persistent*Run of rootCmd and its sub-command both being triggered, what should I do? Or maybe the current behavior is a bug?

@marckhouzam
Copy link
Collaborator

What you can do is have the PersistentPreRunE of the sub-cmd call the PersistentPreRunE of its parent.

@hyorigo
Copy link
Author

hyorigo commented Aug 24, 2020

@marckhouzam Oh, thanks! I found this logic, so why did you authors make this decision?

cobra/command.go

Lines 863 to 873 in 9ed1d71

for p := c; p != nil; p = p.Parent() {
if p.PersistentPostRunE != nil {
if err := p.PersistentPostRunE(c, argWoFlags); err != nil {
return err
}
break
} else if p.PersistentPostRun != nil {
p.PersistentPostRun(c, argWoFlags)
break
}
}

Btw, as per your answer, I tried this code snippet in the sub-cmd, and got stack overflow:

	PersistentPreRunE: func(cmd *cobra.Command, args []string) error {
		if parentPreE := cmd.Parent().PersistentPreRunE; parentPreE != nil {
			if err := parentPreE(cmd, args); err != nil {
				return err
			}
		}

		// blab ... blab ... blah
		return nil
	},

It works after I changed cmd.Parent() into rootCmd, but I just couldn't understand why this would cause infinite loop, even I debugged step by step.

@marckhouzam
Copy link
Collaborator

@marckhouzam Oh, thanks! I found this logic, so why did you authors make this decision?

I can't say for sure what the original intent was, but based on our current discussion, this approach seems to give the most flexibility. It allows to either override an ancestor's PersistentPreRun function by defining one in a subcmd, and it allows to extend ancestor's PersistentPreRun by calling them as you are now doing.

Btw, as per your answer, I tried this code snippet in the sub-cmd, and got stack overflow:

	PersistentPreRunE: func(cmd *cobra.Command, args []string) error {
		if parentPreE := cmd.Parent().PersistentPreRunE; parentPreE != nil {
			if err := parentPreE(cmd, args); err != nil {

I'm guessing the problem is that you pass cmd to parentPreE when you should be passing cmd.Parent().

@hyorigo
Copy link
Author

hyorigo commented Aug 25, 2020

@marckhouzam Thanks for your help! I think we can close this issue now.

@marckhouzam
Copy link
Collaborator

@hyorigo would you mind closing the issue yourself? I don't have permissions.

@hyorigo hyorigo closed this as completed Aug 27, 2020
@hyorigo
Copy link
Author

hyorigo commented Aug 27, 2020

OK. Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants