feat: show banner when workspace is outdated #4926
Conversation
There was a problem hiding this comment.
Nice simple solution 👍🏻.
I think printing to stderr is a good first step, but in the future I think it would be a good idea to start using BannerCallback instead.
At least on systems with /etc/motd and OpenSSH, messages don't appear in non-interactive modes like ssh host ls. I believe this should (automatically) be the case when using BannerCallback too.
But to do that, we'd also need to fix all other occurrences of printing to stdout in the coder ssh [--stdio] command.
cli/ssh.go
Outdated
| @@ -72,6 +73,11 @@ func ssh() *cobra.Command { | |||
| return err | |||
| } | |||
|
|
|||
| updateWorkspaceBanner, outdated := verifyWorkspaceOutdated(client, workspace) | |||
| if outdated { | |||
| _, _ = fmt.Fprintln(cmd.OutOrStdout(), updateWorkspaceBanner) | |||
There was a problem hiding this comment.
We should change this to print on stderr so that non-interactive commands like ssh coder.workspace ls | grep hi aren't dirtied.
I wonder if we should consider only outputting when the recipient is a tty as well. 🤔
There was a problem hiding this comment.
Good catch!
Switched to stderr and added a TTY check.
Thanks for bringing up the idea of BannerCallback. I'm wondering if it isn't something we could park for further improvement. I don't mind working on this, but not sure if adjusting the entire |
cli/ssh.go
Outdated
| @@ -72,6 +73,11 @@ func ssh() *cobra.Command { | |||
| return err | |||
| } | |||
|
|
|||
| updateWorkspaceBanner, outdated := verifyWorkspaceOutdated(client, workspace) | |||
| if outdated && isTTYOut(cmd) { | |||
There was a problem hiding this comment.
Ah, this tty check is only for stdout which doesn't help us (we want to confirm stderr.)
Made the following quick test:
❯ go run main.go >/dev/null
stdout isatty: false
stderr isatty: true
❯ go run main.go 2>/dev/null
stdout isatty: true
stderr isatty: false
Using:
package main
import (
"fmt"
"io"
"os"
"github.com/mattn/go-isatty"
)
func main() {
both := io.MultiWriter(os.Stdout, os.Stderr)
fmt.Fprintf(both, "stdout isatty: %v\n", isatty.IsTerminal(os.Stdout.Fd()))
fmt.Fprintf(both, "stderr isatty: %v\n", isatty.IsTerminal(os.Stderr.Fd()))
}Arguably, this is a bug we potentially have across our whole CLI, but for now I think we can just add a new function: isTTYErr.
There was a problem hiding this comment.
Thanks for the snippet. I refactored the original isTTYOut function, but please let me know what you think.
Definitely. I'll create a separate issue for it. 👍🏻 Edit: #4927 |
|
|
||
| client := codersdk.Client{URL: serverURL} | ||
|
|
||
| t.Run("Up-to-date", func(t *testing.T) { |
There was a problem hiding this comment.
Sorry, I didn't look too closely at these tests on my first pass. I should point out that the preferred way to test in the coder/coder repo is write external tests (package cli_test).
With these types of tests, we'd do a bit more setup for the test so that we can then test the output of running coder ssh ....
(Not a blocker for this PR, but just a heads up for future PRs.)
There was a problem hiding this comment.
Ok, I see now. I can use existing coderdtest APIs. I need to create a workspace to simulate a template update (CreateTemplateVersion). Something like setupWorkspaceForAgent, but customized.
I can adjust the implementation, but not sure if in this case the internal_test isn't just simpler (and shorter). Do you have any preference?
|
I primarily connect to my workspace with VS Code Remote SSH. Will VS Code users notice this, or is it hidden in the SSH logs somewhere? |
|
Hi @bpmct! I'm afraid that VS code operates on a different level as it uses ssh-configs directly. I'm not quite sure if there is a channel to present a notification, warning, orange light, etc. to signal to the VS user that the workspace is outdated. Regarding the traditional ssh, it's like this:
EDIT: after ...
debug1: identity file /Users/mtojek/.ssh/id_dsa type -1
debug1: identity file /Users/mtojek/.ssh/id_dsa-cert type -1
debug1: Local version string SSH-2.0-OpenSSH_9.0
debug1: kex_exchange_identification: banner line 0: �\237\221\213 Your workspace is outdated! Update it here: https://dev.coder.com/@mtojek/mtojek-1!
debug1: kex_exchange_identification: banner line 1:
debug1: Remote protocol version 2.0, remote software version Go
...EDIT2: With
|
|
I'm going to merge this PR as we can always improve the experience in the next iterations. |
Fixes: #4489
This PR introduces a CLI banner presented when the workspace, to which the user is trying to SSH, is outdated.