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

OnKillRun feature #1914

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

OnKillRun feature #1914

wants to merge 3 commits into from

Conversation

decanus
Copy link

@decanus decanus commented Feb 16, 2023

A helper to run a function if a command is killed.

@CLAassistant
Copy link

CLAassistant commented Feb 16, 2023

CLA assistant check
All committers have signed the CLA.

@marckhouzam
Copy link
Collaborator

Cool idea @decanus !
I haven't tried it out yet but it looks useful.
This will need some tests and documentation.
cc @jpmcb

@talbright
Copy link

talbright commented Mar 11, 2023

This is a good idea...we had to implement this outside of cobra (which is fine.) But I think this establishes a good practice that all CLI's should have OOB. Some lint issues to clean up, at a quick glance it should be safe to annotate out SA1017. Not sure about the others...

@marckhouzam marckhouzam added this to the 1.8.0 milestone Apr 5, 2023
@marckhouzam
Copy link
Collaborator

@decanus Sorry for the delay. Can you rebase this to make CI pass?

@decanus
Copy link
Author

decanus commented Oct 13, 2023

@marckhouzam done!

Copy link
Collaborator

@marckhouzam marckhouzam left a comment

Choose a reason for hiding this comment

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

This is really great @decanus ! I love it!
I would like to include it in the next release of Cobra.

Would you be able to add a small paragraph of documentation after this section https://github.com/spf13/cobra/blob/main/site/content/user_guide.md#prerun-and-postrun-hooks

And a unit test would be important to make sure this does not break in the future.

Finally, there are a couple of linter errors to fix.

Thanks a lot!

command.go Outdated Show resolved Hide resolved
command.go Outdated Show resolved Hide resolved
@marckhouzam
Copy link
Collaborator

I was thinking about how this new feature would be used. I like the fact that the OnKillRun can be defined for a specific command. But what if I wanted to create a global OnKillRun for all the commands? For example to always intercept ^C?

Would that be a likely scenario?

@marckhouzam marckhouzam modified the milestones: 1.8.0, 1.9.0 Nov 15, 2023
@marckhouzam
Copy link
Collaborator

@decanus Are you still interested in getting this merged?

syscall.SIGINT,
syscall.SIGKILL,
syscall.SIGTERM,
syscall.SIGQUIT,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we also need os.Interrupt for things to work on Windows

@rojer
Copy link

rojer commented Jun 19, 2024

found this through google - would be nice to get this finished and merged, but i guess for now i'll have to roll my own signal handler

@marckhouzam
Copy link
Collaborator

I'm still interested in merging this @decanus but CI is failing.
Also, a very small doc and test would be important.

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

Successfully merging this pull request may close these issues.

5 participants