feat(cli/configssh): add support for wait yes/no/auto #7893
Conversation
6494ad9 to
a47b44c
Compare
a47b44c to
f4ec444
Compare
cli/configssh.go
Outdated
| Flag: "wait", | ||
| Env: "CODER_CONFIGSSH_WAIT", // Not to be mixed with CODER_SSH_WAIT. | ||
| Description: "Set the option to wait for the the startup script to finish executing. This is the default if the template has configured the agent startup script behavior as blocking. Can not be used together with --no-wait.", | ||
| Value: clibase.BoolOf(&sshConfigOpts.wait), | ||
| }, | ||
| { | ||
| Flag: "no-wait", | ||
| Env: "CODER_CONFIGSSH_NO_WAIT", // Not to be mixed with CODER_SSH_NO_WAIT. | ||
| Description: "Set the option to enter workspace immediately after the agent has connected. This is the default if the template has configured the agent startup script behavior as non-blocking. Can not be used together with --wait.", | ||
| Value: clibase.BoolOf(&sshConfigOpts.noWait), |
There was a problem hiding this comment.
Should we deprecate one of these options?
There was a problem hiding this comment.
It's a valid thing to point out, however, both are needed because depending on the agent configuration, you may want to use the opposite of what is configured.
If we only used --wait and --wait=false, for instance, we would still need to detect the presence of this flag. I.e. we can't simply check the boolean, we have to check if the flag/env was provided so that we know whether or not to override the template. We also can't say on the CLI what the default value for this flag is because it's workspace dependent.
I'm not sure clibase easily provides this info and I also think it's bad UX to require =false (or true). 🤔
For most clarity, this should perhaps be --startup-script-behavior=auto|blocking|non-blocking, but again, not very UX friendly.
There was a problem hiding this comment.
Just throwing ideas about, but how about --wait=(always|never|auto) (default=auto)? That equates to the same as --startup-script-behaviour and is arguably easier to manage than --wait vs --no-wait, at the cost of a little more typing.
There was a problem hiding this comment.
It's a great suggestion. What do you think @bpmct?
I would amend this to be --wait=yes|no|auto, this way it's less typing, and could even support shorthand in the future like -w y.
There was a problem hiding this comment.
Tried implementing it here: 5e8c487 (#7894), I like it.
| sshOptions []string | ||
| waitEnum string | ||
| userHostPrefix string | ||
| sshOptions []string |
| if sshConfigOpts.waitEnum != "auto" && skipProxyCommand { | ||
| return xerrors.Errorf("cannot specify both --skip-proxy-command and --wait") | ||
| } |
There was a problem hiding this comment.
Makes sense, but for anyone reading this code for the first time, maybe include a comment on this that says something like:
// The waitEnum is a flag applied to the ProxyCommand. If the user specifies skipProxyCommand
// then the waitEnum flag is not used.Just took me a second to remember what SkipProxyCommand did.
| @@ -505,9 +527,16 @@ func (r *RootCmd) configSSH() *clibase.Cmd { | |||
| }, | |||
| { | |||
| Flag: "ssh-host-prefix", | |||
| Env: "", | |||
| Env: "CODER_CONFIGSSH_SSH_HOST_PREFIX", | |||
This PR adds support for wait/no-wait and also for flag option serialization into the config file, like ssh options provided via
-o.Refs #7768
Depends on #7894