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

Add OnFinalize method #1788

Merged
merged 1 commit into from Oct 3, 2022
Merged

Add OnFinalize method #1788

merged 1 commit into from Oct 3, 2022

Conversation

yann-soubeyrand
Copy link
Contributor

This method is the OnInitialize counterpart. Like OnInitialize which allows loading the configuration before each command is executed, OnFinalize allows saving the configuration after each command has been executed.

This method is the OnInitialize counterpart. Like OnInitialize which allows
loading the configuration before each command is executed, OnFinalize allows
saving the configuration after each command has been executed.
@github-actions github-actions bot added the size/S Denotes a PR that chanes 10-24 lines label Aug 28, 2022
@marckhouzam marckhouzam added kind/feature A feature request for cobra; new or enhanced behavior area/cobra-command Core `cobra.Command` implementations labels Aug 28, 2022
Copy link

@stmcginnis stmcginnis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like OnInitialize which allows loading the configuration before each command is executed, OnFinalize allows saving the configuration after each command has been executed.

Seems like a useful use case to support.

@marckhouzam
Copy link
Collaborator

This seems reasonable.
However, I never really understood the use case for initializers.
What is the difference between using an initializer/finalizer compared to a PersistentPreRun/PersistentPostRun?

Copy link
Collaborator

@jpmcb jpmcb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree - this seems reasonable, but I think we're looking for a valid use case on this to keep the Cobra API thin: @yann-soubeyrand can you provide a small Cobra CLI program that would require this use case? Thanks much!

@jpmcb jpmcb added the triage/needs-info Needs more investigation from maintainers or more info from the issue provider label Sep 29, 2022
@yann-soubeyrand
Copy link
Contributor Author

This seems reasonable. However, I never really understood the use case for initializers. What is the difference between using an initializer/finalizer compared to a PersistentPreRun/PersistentPostRun?

PersistentPreRun and PersistentPostRun of the root command aren’t run if there are PersistentPreRun and PersistentPostRun in “intermediary commands”. For example, the program here https://github.com/yann-soubeyrand/cobra-test gives the following result:

❯ ./cobra-test parent child
initialize
parent persistent pre run
child called
parent persistent post run
finalize

Also, if using RunE, a PostRun may not be run in case of error:

cobra/command.go

Lines 872 to 875 in 7039e1f

if c.RunE != nil {
if err := c.RunE(c, argWoFlags); err != nil {
return err
}

It may be interresting in some cases to be able to do some clean-up using a finalizer function.

@yann-soubeyrand
Copy link
Contributor Author

I agree - this seems reasonable, but I think we're looking for a valid use case on this to keep the Cobra API thin: @yann-soubeyrand can you provide a small Cobra CLI program that would require this use case? Thanks much!

The program here https://github.com/yann-soubeyrand/cobra-test-2 has been initialized using cobra-cli init --viper which creates the initConfig function to load the configuration using viper. I’d like to have the ability to save the configuration at the end of the execution, hence my saveConfig function. It can be tested like this:

❯ touch ~/.cobra-test-2
❯ ./cobra-test-2 config get
Using config file: /home/yann/.cobra-test-2
<nil>
❯ ./cobra-test-2 config set
Using config file: /home/yann/.cobra-test-2
❯ ./cobra-test-2 config get
Using config file: /home/yann/.cobra-test-2
value

In this simple case, I arguably could have done the write of the config at the end or in the PostRun of my set command. However, in more complex programs where the configuration can be modified in several places, it’s useful to have a function which will be executed for sure at the end (unless there’s a panic or an explicit exit of course) which can be used to save the config.

jpmcb
jpmcb approved these changes Oct 1, 2022
Copy link
Collaborator

@jpmcb jpmcb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! Been playing around with this and I do think it'll be useful. Thanks much for this!!

@jpmcb jpmcb added lgtm Denotes "looks good to me" from maintainers and signals other collaboratores that a PR is ready and removed triage/needs-info Needs more investigation from maintainers or more info from the issue provider labels Oct 1, 2022
@jpmcb jpmcb merged commit 93d1913 into spf13:main Oct 3, 2022
15 checks passed
@yann-soubeyrand yann-soubeyrand deleted the add-onfinalize branch October 3, 2022 16:47
@marckhouzam marckhouzam added this to the 1.6.0 milestone Oct 3, 2022
@umarcor umarcor mentioned this pull request Oct 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/cobra-command Core `cobra.Command` implementations kind/feature A feature request for cobra; new or enhanced behavior lgtm Denotes "looks good to me" from maintainers and signals other collaboratores that a PR is ready size/S Denotes a PR that chanes 10-24 lines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants