- Notifications
You must be signed in to change notification settings - Fork18
feat: Prompt user to rebuild workspace on coder sh#223
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
we should only prompt the user if they are in an interactive terminal
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.
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 willnot 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 therunCommand, an alternative is I could usehttps://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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 becoder 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.
Regardingcoder sh tiny | echo, erroring out as we do now with the helpful error message seems sufficient.
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I am assuming running this viacron 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yes, agreed. Doesn't appear to be an easy win here.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
@cmoog addressed all comments, the last sticky point is if the current interactive shell behavior is sufficient |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yes, agreed. Doesn't appear to be an easy win here.
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.
Looks good. Nice work.
I think the interactive behavior, as you have it, is the best we're going to get for now.
If the workspace is OFF or a rebuild is required, promptthe user to rebuild right away. This prompt only occursin an interactive shell
6752124 to7a4f2dbCompare
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.