feat: Prompt user to rebuild workspace on coder sh #223
Conversation
internal/cmd/shell.go
Outdated
| confirm := promptui.Prompt{ | ||
| Label: rebuildReason + " Do you wish to rebuild it now? (this will take a moment)", | ||
| IsConfirm: true, | ||
| } | ||
| if _, err := confirm.Run(); err != nil { |
There was a problem hiding this comment.
we should only prompt the user if they are in an interactive terminal
There was a problem hiding this comment.
Is this what you had in mind to check for that?
https://github.com/cdr/coder-cli/blob/423279755e6607dc1236066d47fd5333d25268b5/internal/cmd/shell.go#L107-L109
So these will not prompt:
$GOPATH/bin/coder sh tiny | echo
`/home/coder/go/bin/coder sh tiny`
Is there a specific way of executing you had in mind? I took this code from the runCommand, an alternative is I could use https://rosettacode.org/wiki/Check_output_device_is_a_terminal#Go. I think they do the same thing, so I kept it consistent with the other way we did it.
There was a problem hiding this comment.
Yeah that's what I had in mind. That said, point taken... this is certainly leaky behavior. Since this is more of a nicety, and we can assume 90% of cases will be coder sh [env] without pipes or args... I think we should do the cautious thing and only show the prompt when we know for certain the output is interactive.
Regarding coder sh tiny | echo, erroring out as we do now with the helpful error message seems sufficient.
There was a problem hiding this comment.
I left a comment that --force or --no-prompt would be a clear way for someone to determine behavior, but adding flags to this command is non-trivial because the flag parsing is disabled
There was a problem hiding this comment.
I am assuming running this via cron and other ways won't trigger the prompt too. So I can see how this is helpful, but I don't think it's obvious if I was just writing some bash scripts. Bash scripts will still trigger the prompt if you just run them ./script.sh.
There was a problem hiding this comment.
Yes, agreed. Doesn't appear to be an easy win here.
|
@cmoog addressed all comments, the last sticky point is if the current interactive shell behavior is sufficient |
internal/cmd/shell.go
Outdated
| confirm := promptui.Prompt{ | ||
| Label: rebuildReason + " Do you wish to rebuild it now? (this will take a moment)", | ||
| IsConfirm: true, | ||
| } | ||
| if _, err := confirm.Run(); err != nil { |
There was a problem hiding this comment.
Yes, agreed. Doesn't appear to be an easy win here.
If the workspace is OFF or a rebuild is required, prompt the user to rebuild right away. This prompt only occurs in an interactive shell
6752124 to
7a4f2db
Compare
Quality of Life Feature
When a user does
coder sh <env>, the cli first checks if that environment isOFFor if it requires a rebuild before using. It will then prompt the user if they want to rebuild the workspace.To test
Off
Rebuild Message
If the environment has a required rebuild message, the output looks like this:
Questions:
Currently
--forcedoes not work for this feature, and there is no--no-rebuildflag. The current command has flag parsing disabled. Are these kinds of flags required? I imagine a user prompt would make scriptingcoder shrather annoying.