-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Suspend k9s using CTRL-Z #2484
base: master
Are you sure you want to change the base?
Suspend k9s using CTRL-Z #2484
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@arajski Thanks for this update Artur!
Biggest concern with this behavior is to make sure we're cool all supported platforms.
@@ -13,6 +13,7 @@ | |||
"maxConnRetry": { "type": "integer" }, | |||
"readOnly": { "type": "boolean" }, | |||
"noExitOnCtrlC": { "type": "boolean" }, | |||
"noSuspendOnCtrlZ": { "type": "boolean" }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not keen on the negative naming here. Think folks may want to opt in just in case.
Perhaps allowSuspend
? Ctrl-z is likely already understood from folks aware of this command.
To boot, it will default to false if left unset hence keeping current behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree on that one. I was looking at noExitOnCtrlC
as a sort of naming convention here. Anyway - renamed to allowSuspend
👍
internal/ui/app.go
Outdated
@@ -143,6 +144,7 @@ func (a *App) bindKeys() { | |||
KeyColon: NewKeyAction("Cmd", a.activateCmd, false), | |||
tcell.KeyCtrlR: NewKeyAction("Redraw", a.redrawCmd, false), | |||
tcell.KeyCtrlC: NewKeyAction("Quit", a.quitCmd, false), | |||
tcell.KeyCtrlZ: NewKeyAction("Suspend", a.suspendCmd, false), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should only be available if the option is set and likely we may not want to show it in the header all the time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved it out of the initial set
internal/ui/app.go
Outdated
return evt | ||
} | ||
|
||
if !a.Config.K9s.NoSuspendOnCtrlZ { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We would not be needed the check here if we don't enable the action in the first place?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed ;)
internal/ui/app.go
Outdated
} | ||
|
||
if !a.Config.K9s.NoSuspendOnCtrlZ { | ||
a.Suspend(func() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will require additional testing on various platforms to make sure we're cool on other non nix aka windows?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've got Windows 10 as dual boot, so can give it a test. Is there are set of other recommended platforms/versions?
Some VMs/containers might be required for this.
internal/view/reference.go
Outdated
@@ -40,7 +40,7 @@ func (r *Reference) Init(ctx context.Context) error { | |||
|
|||
func (r *Reference) bindKeys(aa ui.KeyActions) { | |||
aa.Delete(ui.KeyShiftA, tcell.KeyCtrlS, tcell.KeyCtrlSpace, ui.KeySpace) | |||
aa.Delete(tcell.KeyCtrlW, tcell.KeyCtrlL, tcell.KeyCtrlZ) | |||
aa.Delete(tcell.KeyCtrlW, tcell.KeyCtrlL) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should now be KeyCtrlT since we renamed it ;(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added KeyCtrlT back
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@arajski Thank you for the updates Artur! LGTM
I think we need to test this out on linux, windows flavors, macOs.
@derailed seems like go doesn't support os signals on Windows without some hacky workarounds. |
@arajski Thank you for the research Artur! I think for non nix users likely Ctrtl-z does not make much sense. I don't use windows much these days but isn't Ctrl-z equivalent to |
9497cc0
to
33e727e
Compare
Done 👍 Tested on macOS Sonoma and Debian 12 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@arajski Thanks for the checks and updates Artur.
internal/ui/app.go
Outdated
@@ -146,6 +148,12 @@ func (a *App) bindKeys() { | |||
tcell.KeyCtrlU: NewSharedKeyAction("Clear Filter", a.clearCmd, false), | |||
tcell.KeyCtrlQ: NewSharedKeyAction("Clear Filter", a.clearCmd, false), | |||
} | |||
|
|||
if a.Config.K9s.AllowSuspend && runtime.GOOS != "windows" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it might be best to make a call on the config and keep this functionality encapsulated vs exposing on the call site? Perhaps something a.Config.K9s.IsSuspendable()?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep makes sense - done :)
Description
Implements unix-like suspend functionality mentioned in #2013
At the moment, k9s supports Ctrl-C, which exits the application, however it's not possible to suspend the process and send it to the background. This PR implements that functionality, which is common among other tools, like vim.
User can now send the process to the background by pressing Ctrl-Z.
Once suspended, it can be resumed by either using
fg
or selecting a specific id from thejobs
list (for example%1
) in case there are multiple ones.This behaviour can be enabled by setting
allowSuspend
totrue
.Changes
SIGTSTP
signal to the current k9s processallowSuspend
config to opt in to this behaviourTODO:
Notes
This is my first contribution to this project, so happy to update things if there's anything missing/required. Thanks!