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

cli: handle signals properly #1129

Merged
merged 1 commit into from
Oct 2, 2020
Merged

cli: handle signals properly #1129

merged 1 commit into from
Oct 2, 2020

Conversation

bonifaido
Copy link
Member

@bonifaido bonifaido commented Oct 2, 2020

Signed-off-by: Nandor Kracser bonifaido@gmail.com

Q A
Bug fix? yes
New feature? no
API breaks? yes
Deprecations? no
Related tickets fixes #1128
License Apache 2.0

What's in this PR?

Handling signals, and do a 0 status code exit.

Why?

If signals are not captured the CLI exits with status code 2.

Additional context

Checklist

@bonifaido bonifaido self-assigned this Oct 2, 2020
@@ -99,6 +101,16 @@ var rootCmd = &cobra.Command{
// Execute adds all child commands to the root command sets flags appropriately.
// This is called by main.main(). It only needs to happen once to the rootCmd.
func execute() {
// Handle signals to prevent bad exit codes on `docker stop`.
// TODO: robably a more sophisticated exit procedure should be implemented in the future.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// TODO: robably a more sophisticated exit procedure should be implemented in the future.
// TODO: probably a more sophisticated exit procedure should be implemented in the future.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that's not a bad idea. :D

sigs := make(chan os.Signal, 1)
signal.Notify(sigs, syscall.SIGTERM, syscall.SIGINT, syscall.SIGABRT)
go func() {
for range sigs {
Copy link
Member

Choose a reason for hiding this comment

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

No need for for. <-sigs should work.

Signed-off-by: Nandor Kracser <bonifaido@gmail.com>
@bonifaido bonifaido merged commit 36d1b6b into master Oct 2, 2020
@bonifaido bonifaido deleted the signals branch October 2, 2020 14:41
rrondeau pushed a commit to rrondeau/bank-vaults that referenced this pull request Nov 3, 2020
Signed-off-by: Nandor Kracser <bonifaido@gmail.com>
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.

Configurer does not respect signals
3 participants