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

Update pterm so it doesn't intercept signals #528

Merged
merged 2 commits into from
May 14, 2024
Merged

Conversation

adamwg
Copy link
Member

@adamwg adamwg commented May 13, 2024

Description of your changes

In older versions, the pterm library we use for pretty-printing intercepted SIGINT and SIGTERM. This signal handling was set up in the pacakge's init function, so any code that ran before we set up our own signal handler couldn't be killed with Ctrl-C.

The signal handling was removed from pterm in a more recent version (see [1]). Updating to the latest fixes the issue where we couldn't kill up login with Ctrl-C (#526).

[1] pterm/pterm#562

Fixes #526

Signed-off-by: Adam Wolfe Gordon adam.wolfegordon@upbound.io

I have:

  • Read and followed Upbound's contribution process.
  • Run make reviewable to ensure this PR is ready for review.
  • Added backport release-x.y labels to auto-backport this PR, as appropriate.

How has this code been tested

  • Manual testing to validate that up login can be killed with Ctrl-C at either the Username or the Password prompt, with or without entering input first.
  • make test to ensure the dependency update didn't break any unit tests.
  • Ran a few other commands (up ctx, up ctp list, up space list) to validate that output still looks right.

In older versions, the pterm library we use for pretty-printing intercepted
SIGINT and SIGTERM. This signal handling was set up in the pacakge's `init`
function, so any code that ran before we set up our own signal handler couldn't
be killed with Ctrl-C.

The signal handling was removed from pterm in a more recent version (see
[1]). Updating to the latest fixes the issue where we couldn't kill `up login`
with Ctrl-C (#526).

[1] pterm/pterm#562

Fixes #526

Signed-off-by: Adam Wolfe Gordon <adam.wolfegordon@upbound.io>
@adamwg
Copy link
Member Author

adamwg commented May 13, 2024

Question: with this change, do we still need the signal handler we set up in main.go? All it does is call os.Exit (via kongCtx.Exit), and looking at git history I suspect it was introduced to work around the fact that pterm was intercepting our signals.

@tnthornton
Copy link
Member

Question: with this change, do we still need the signal handler we set up in main.go? All it does is call os.Exit (via kongCtx.Exit), and looking at git history I suspect it was introduced to work around the fact that pterm was intercepting our signals.

My guess is no, but it should be easy enough to test your changes with the up space init flow to double check.

With pterm updated to not intercept signals, we don't need our own signal
handler, which was just implementing the default behavior of exiting on SIGINT.

Signed-off-by: Adam Wolfe Gordon <adam.wolfegordon@upbound.io>
@adamwg
Copy link
Member Author

adamwg commented May 14, 2024

Confirmed that with our signal handler removed up space init can still be stopped with Ctrl-C. Added a commit to remove our signal handler, since it's not necessary.

@adamwg adamwg merged commit 433c767 into main May 14, 2024
7 checks passed
@adamwg adamwg deleted the awg/fix-ctrlc-login branch May 14, 2024 19:04
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.

up login does not respond to Ctrl-C
2 participants