Issue #219: added PreRunChain and PostRunChain hooks #220

algrebe · 2016-01-08T07:14:34Z

PreRunChain functions run in the order of root to child.
PostRunChain functions run in the order of child to root.

The overall exection order for a command is
PreRunChain
PersistentPreRun
PreRun
Run
PostRun
PersistentPostRun
PostRunChain

The following gist describes the usage and functionality
https://gist.github.com/algrebe/23cc8bf4739a129a6d0d

PreRunChain functions run in the order of root to child. PostRunChain functions run in the order of child to root. The overall exection order for a command is PreRunChain PersistentPreRun PreRun Run PostRun PersistentPostRun PostRunChain The following gist describes the usage and functionality https://gist.github.com/algrebe/23cc8bf4739a129a6d0d

mitake · 2016-01-12T07:47:16Z

Hi @algrebe , @spf13 , nice to meet you.

I'm using cobra and I really want this feature! @spf13 do you have a plan to merge this PR?

ToDo: use this spf13/cobra#220

algrebe · 2016-01-12T08:19:15Z

@mitake Hi there. Yes, I'm waiting for it to be pulled too after a review - I think they might want a better name for it.
@eparis was actively following up with me regarding #216
so I guess its just a matter of time.

mitake · 2016-01-12T08:50:57Z

@algrebe thanks for your reply, I'm looking forward to seeing next news!

spf13 · 2016-01-12T17:14:54Z

I'm fine with the functionality in general. I'm a bit worried this is a bit of an overcomplicated solution, but I must admit I don't really understand the use cases that would require all of this. This totals up to 7 different Run actions per command. It seems like overkill.

Not a criticism, but more of a request to help me understand why we need all of these. It seems like a simpler approach could exist that would give us all the functionality we need.

algrebe · 2016-01-12T19:02:08Z

@spf13 thanks for your reply
@spf13 good point. Before taking any new functionality, you should decide whether this is required, and whether we can make do with what is there.

Run function is needed to run the command.
Pre and Post are needed as hooks just before/after the command.
Persistent functions (existing logic) that are inherited and can be overridden.

For me, I just need something that always runs - irrespective of whether the child has its own hooks or not. I had always assumed Peristent* did this, but turns out they can be overridden. Hence, I would need
4. Functions that cannot be overridden and run whenever any of its children / self is executed.

As for the usecase - this was the issue I was facing #219 ( setting log levels and profiler at the root, and stopping it at the root - where I have access to the object that starts it )
Also, I feel a parent should have the option of running something irrespective of the child.

I have a suggestion, which may / may not be acceptable but just throwing it out here -
Can the *Chain functions take the place of Persistent* functions ? Since Persistent actually means always run.

spf13 · 2016-01-12T19:41:29Z

Chain without persistent makes sense to me.

Should the default behavior of pre and post just be behavior described as
chain here? I feel like we can probably cover all these use cases with just
that.

On Tue, Jan 12, 2016 at 2:02 PM Anthony notifications@github.com wrote:

@spf13 https://github.com/spf13 thanks for your reply
@spf13 https://github.com/spf13 good point. Before taking any new
functionality, you should decide whether this is required, and whether we
can make do with what is there.

Run function is needed to run the command.

Pre and Post are needed as hooks just before the command.

Persistent functions (existing logic ) that are inherited and can
be overridden.

For me, I just need something that always runs - irrespective of whether
the child has its own hooks or not. I had always assumed Peristent* did
this, but turns out they can be overridden. Hence, I would need
4. Functions that cannot be overridden and run whenever any of its
children / self is executed.

As for the usecase - this was the issue I was facing #219
#219 .
Also, I feel a parent should have the option of running something
irrespective of the child.

I have a suggestion, which may / may not be acceptable but just throwing
it out here -
Can the Chain functions take the place of Persistent functions ? Since
Persistent actually means always run.

—
Reply to this email directly or view it on GitHub
#220 (comment).

algrebe · 2016-01-12T19:59:32Z

Chain without persistent makes sense to me.
I'm not sure what you meant by this - chain should exist, persistent shouldnt ?

Should the default behavior of pre and post just be behavior described as
chain here
@spf13 Yes - makes sense to me
Shall I modify this pull request to remove the chains and put that behavior in Pre and Post ?

This is a change in the existing functionality though, so previous users will have to be aware that Pre and Post have changed right ?

spf13 · 2016-01-12T20:06:11Z

Let's have @eparis weigh in first.
On Tue, Jan 12, 2016 at 2:59 PM Anthony notifications@github.com wrote:

@spf13 https://github.com/spf13 Yes - makes sense to me.
Shall I modify this pull request to remove the chains and put that
behavior in Pre and Post ?

This is a change in the existing functionality though, so previous users
will have to be aware that Pre and Post have changed right ?

—
Reply to this email directly or view it on GitHub
#220 (comment).

eparis · 2016-01-12T20:29:29Z

I'll start with that fact that I think chaining makes more sense than what we have now. What we have now is copying the Flags()/PersistentFlags() where 'chaining' just doesn't make sense.

We have what we have so I'm all over the place on what to do. Imagine I have a subcommand pre/post that is calling the parent command pre/post. With a change to make 'persistent' 'chained' my parent command is going to get called twice.

Imagine the same case where the parent pre/post isn't called today and shouldn't be, now it will be and the user of cobra won't have a reasonable way to know of this change in behavior.

2.0 API should ONLY have chained....

spf13 · 2016-01-12T20:43:44Z

I agree. There's a good solution here thought that doesn't break compatibility and doesn't introduce a more complicated mechanism.

We add a flag to Cobra that defaults to the current behavior, but can be flipped to the chain behavior. Cobra 2.0 we keep the flag, but change the default. Cobra 3.0 we remove the flag.

That way new implementations can immediately take advantage of this cleaner approach while existing implementations can have time to migrate.

Does that make sense?

I recognize it's a big change, but I'm super weary of adding complexity to the interface and would much prefer to refine and simplify what we have now instead. If we can do that without introducing breaking changes that I think it's a win/win.

eparis · 2016-01-12T20:44:38Z

@spf13 +1

bep · 2016-01-12T20:50:02Z

+1

spf13 · 2016-01-12T22:40:29Z

@algrebe can you update the PR accordingly? I'd gladly merge.

algrebe · 2016-01-13T06:34:10Z

@spf13 yes, this sounds good.

Just to confirm

Add a flag to cobra ( --use-chain-hooks ) that is false by default
PreRun and PostRun examine the --use-chained-hooks to decide whether to chain or not.

I'm confused about the order:
Currently PersistentPreRun is called before PreRun, and PersistentPostRun after PostRun.
If there is chaining, where would you want it ? Before PersistentPreRun / After PostRun or like how it is right now? Or depending on the flag ?

This commit adds flags for profiling with runtime/pprof to storage put: - --cpuprof-path: specify a path of CPU profiling result, if it is not empty, profiling is activated - --memprof-path: specify a path of heap profiling result, if it is not empty, profiling is activated Of course, the flags should be added to RootCmd ideally. However, adding common flags that shared by children command requires the ongoing PR: spf13/cobra#220 . Therefore this commit adds the flags to storage put only.

spf13 · 2016-01-13T12:11:34Z

We don't need persistent anymore. Just chaining.

With chaining it should cascade down from the root to the run of the
executed command (pre) then back the other direction with the post.
On Wed, Jan 13, 2016 at 1:34 AM Anthony notifications@github.com wrote:

@spf13 https://github.com/spf13 yes, this sounds good.

Just to confirm

Add a flag to cobra ( --use-chain-hooks ) that is false by default

PreRun and PostRun examine the --use-chained-hooks to decide whether to
call it or not.

I'm confused about the order:
Currently PersistentPreRun is called before PreRun, and PersistentPostRun
after PostRun.
If there is chaining, where would you want it ? Before PersistentPreRun /
After PostRun or like how it is right now? Or depending on the flag ?

—
Reply to this email directly or view it on GitHub
#220 (comment).

eparis · 2016-01-13T13:53:44Z

Also I don't know about --use-chain-hooks. The end user shouldn't (need) to know about chaining, only the programmer. Which makes that seem like an external variable, UseChainHooks.

However I'm not sure what behavior we should get if the var is set on the root command but not on the subcommand being executed...

spf13 · 2016-01-13T14:57:17Z

It should only be able to be set (or checked) on the root. It's effectively
global. This can be accomplished with private fields and functions. See how
Exec() is crafted.

On Wed, Jan 13, 2016 at 8:53 AM Eric Paris notifications@github.com wrote:

Also I don't know about --use-chain-hooks. The end user shouldn't (need)
to know about chaining, only the programmer. Which makes that that an
external variable UseChainHooks.

However I'm not sure what behavior we should get if the var is set on the
root command but not on the subcommand being executed...

—
Reply to this email directly or view it on GitHub
#220 (comment).

This reverts commit 3dbb5be. As part of spf13#220 PreRunChain and PostRunChain aren't required - the PersistentRun functions need to be modified

Added the functionality for persistent functions to chain from root to child in the case of PersistentPreRun and from child to root in the case of PersistentPostRun. Added a global internal variable useChainHooks which decides whether to use chain feature, or use the previous logic of nearest defined ancestor. This variable useChainHooks is disabled by default in this commit. Developers importing the cobra package can enable it using the function EnableChainHooks() or disable it using DisableChainHooks(). However, these functions may be called only once externally, preferably in the init of the developers main program.

algrebe · 2016-01-13T17:45:31Z

@spf13 Hey, I think I've got a neat solution based on all the feedback - here it is.
You can also check out this gist https://gist.github.com/algrebe/cae234e77dae809bfe18
to test out its functionality.

This commit adds flags for profiling with runtime/pprof to storage put: - --cpuprof-path: specify a path of CPU profiling result, if it is not empty, profiling is activated - --memprof-path: specify a path of heap profiling result, if it is not empty, profiling is activated Of course, the flags should be added to RootCmd ideally. However, adding common flags that shared by children command requires the ongoing PR: spf13/cobra#220 . Therefore this commit adds the flags to storage put only.

This commit adds flags for profiling with runtime/pprof to storage put: - --cpuprofile: specify a path of CPU profiling result, if it is not empty, profiling is activated - --memprofile: specify a path of heap profiling result, if it is not empty, profiling is activated Of course, the flags should be added to RootCmd ideally. However, adding common flags that shared by children command requires the ongoing PR: spf13/cobra#220 . Therefore this commit adds the flags to storage put only.

spf13 · 2016-01-18T18:44:42Z

@algrebe I think we can do this without the persistent funcs at all... or at least I can't think of why we would need them. Just Pre & Post with a toggle option to make them universally chain-able or not.

Am I oversimplifying it?

algrebe · 2016-01-18T19:32:15Z

@spf13 I cant find a reason where someone would want a hybrid of both chaining and running a hook just for that command in the same setup, so simplifying it makes sense to me.
Although, just in case we've missed a valid scenario - this means loss of a feature right ? Which can easily be introduced later if necessary I guess. Shall we wait for others to pitch on or should I go ahead with the changes mentioned ?

spf13 · 2016-01-18T19:36:50Z

We should talk to @eparis. I'm not sure how much this existing feature is
in use. I don't use it, but I have far from the most complex usage of Cobra
out there. I'm ok with a breaking change if it doesn't affect many people.
If it does affect people then we should hold off on taking it out until we
make a breaking release.

algrebe · 2016-05-21T11:15:53Z

@spf13 @eparis Hi there ! Long time. I was looking at the issues on the page and #252 seems related to this as well. Please let me know what else I can do to take this feature to completion.

lukasmalkmus · 2016-07-06T15:55:52Z

@spf13 @eparis Hi. Don't want to bother you but I see a lot of open PRs and issues. Some are months old. Is this project still maintained?

algrebe force-pushed the persistent-chaining branch from 42f7c84 to 3dbb5be Compare January 8, 2016 07:24

mitake added a commit to mitake/etcd that referenced this pull request Jan 12, 2016

wip

e287aef

ToDo: use this spf13/cobra#220

mitake mentioned this pull request Jan 13, 2016

tools/benchmark: add flags for pprof to storage put etcd-io/etcd#4201

Merged

algrebe added 2 commits January 13, 2016 21:52

Revert "Issue spf13#219: added PreRunChain and PostRunChain hooks"

348f909

This reverts commit 3dbb5be. As part of spf13#220 PreRunChain and PostRunChain aren't required - the PersistentRun functions need to be modified

spf13 closed this Jun 7, 2019

steven200796 mentioned this pull request Nov 16, 2019

Run Persistent*Run from root to sub #714

Closed

bartdeboer mentioned this pull request May 26, 2020

Include EnablePersistentHookOverride option to control Persistent*Run override behavior #1123

Closed

bartdeboer mentioned this pull request Jun 22, 2020

Extend Persistent*Run behavior to allow multiple hooks throughout the execution chain #1142

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #219: added PreRunChain and PostRunChain hooks #220

Issue #219: added PreRunChain and PostRunChain hooks #220

algrebe commented Jan 8, 2016

mitake commented Jan 12, 2016

algrebe commented Jan 12, 2016

mitake commented Jan 12, 2016

spf13 commented Jan 12, 2016

algrebe commented Jan 12, 2016

spf13 commented Jan 12, 2016

algrebe commented Jan 12, 2016

spf13 commented Jan 12, 2016

eparis commented Jan 12, 2016

spf13 commented Jan 12, 2016

eparis commented Jan 12, 2016

bep commented Jan 12, 2016

spf13 commented Jan 12, 2016

algrebe commented Jan 13, 2016

spf13 commented Jan 13, 2016

eparis commented Jan 13, 2016

spf13 commented Jan 13, 2016

algrebe commented Jan 13, 2016

spf13 commented Jan 18, 2016

algrebe commented Jan 18, 2016

spf13 commented Jan 18, 2016

algrebe commented May 21, 2016

lukasmalkmus commented Jul 6, 2016

Issue #219: added PreRunChain and PostRunChain hooks #220

Issue #219: added PreRunChain and PostRunChain hooks #220

Conversation

algrebe commented Jan 8, 2016

mitake commented Jan 12, 2016

algrebe commented Jan 12, 2016

mitake commented Jan 12, 2016

spf13 commented Jan 12, 2016

algrebe commented Jan 12, 2016

spf13 commented Jan 12, 2016

algrebe commented Jan 12, 2016

spf13 commented Jan 12, 2016

eparis commented Jan 12, 2016

spf13 commented Jan 12, 2016

eparis commented Jan 12, 2016

bep commented Jan 12, 2016

spf13 commented Jan 12, 2016

algrebe commented Jan 13, 2016

spf13 commented Jan 13, 2016

eparis commented Jan 13, 2016

spf13 commented Jan 13, 2016

algrebe commented Jan 13, 2016

spf13 commented Jan 18, 2016

algrebe commented Jan 18, 2016

spf13 commented Jan 18, 2016

algrebe commented May 21, 2016

lukasmalkmus commented Jul 6, 2016