feat: add support for exiting ssh session #3331
Conversation
|
That's still two different things in one PR, keeping them separate makes for easier review |
cmd/limactl/shell.go
Outdated
| @@ -49,6 +51,7 @@ func newShellCommand() *cobra.Command { | |||
|
|
|||
| shellCmd.Flags().String("shell", "", "shell interpreter, e.g. /bin/bash") | |||
| shellCmd.Flags().String("workdir", "", "working directory") | |||
| shellCmd.Flags().BoolP("exit-session", "e", false, "exit a ssh session for the instance") | |||
There was a problem hiding this comment.
I'd prefer to reserve e for now. (Potentially for something similar to ssh -e)
Can be revisited later.
cmd/limactl/shell.go
Outdated
| @@ -49,6 +51,7 @@ func newShellCommand() *cobra.Command { | |||
|
|
|||
| shellCmd.Flags().String("shell", "", "shell interpreter, e.g. /bin/bash") | |||
| shellCmd.Flags().String("workdir", "", "working directory") | |||
| shellCmd.Flags().BoolP("exit-session", "e", false, "exit a ssh session for the instance") | |||
There was a problem hiding this comment.
| shellCmd.Flags().BoolP("exit-session", "e", false, "exit a ssh session for the instance") | |
| shellCmd.Flags().Bool("exit-session", false, "exit a ssh session before running the command, for <REASON>") |
There was a problem hiding this comment.
Bikeshedding: something like --reestablish-session might be less confusing?
There was a problem hiding this comment.
@AkihiroSuda I want to know what we are trying to achieve here. Is it only a shell termination or a termination + re-establish?
Bikeshedding: something like --reestablish-session might be less confusing?
I guess this will be a little long for a user to type, --restart if you will?
There was a problem hiding this comment.
Maybe --restart-ssh ?
--restart sounds like restarting the VM. --exit sounds like exiting SSH without executing a command.
There was a problem hiding this comment.
--restart-ssh sounds great!
cmd/limactl/shell.go
Outdated
| @@ -49,6 +50,7 @@ func newShellCommand() *cobra.Command { | |||
|
|
|||
| shellCmd.Flags().String("shell", "", "shell interpreter, e.g. /bin/bash") | |||
| shellCmd.Flags().String("workdir", "", "working directory") | |||
| shellCmd.Flags().Bool("restart-ssh", false, "restart the SSH connection") | |||
There was a problem hiding this comment.
This is definitely bike-shedding now, but the --restart-ssh name somehow doesn't vibe with me. It feels more like an implementation detail than expressing what the user wants.
I think both --restart-session or even --reconnect would work better.
Another reason is that I think eventually we will use a different transport depending on the driver. E.g. with WSL2 it makes sense to execute commands via wsl -d $DISTRO instead of ssh. So using ssh in the option name would feel wrong in that situation.
unsuman
force-pushed
the
feat/exit-ssh-session
branch
from
08c5994 to
c9097fe
Compare
Signed-off-by: Ansuman Sahoo <[email protected]>
unsuman
force-pushed
the
feat/exit-ssh-session
branch
from
c9097fe to
fc974a5
Compare
There was a problem hiding this comment.
Thanks, LGTM
Thanks for changing the settins name to --reconnect, which does work for me. However, I wanted to hear from other @lima-vm/maintainers about their opinion.
So I'm approving this PR, but leave it to the others to either merge, or continue to bike-shed it.
This PR fixes #3125 by adding a boolean flag,
--exit-session, to thelimactl shellcommand, enabling users to exit the current SSH session. Additionally, it introduces an aliassshfor theshellcommand.