NotificationsYou must be signed in to change notification settings
Fork928
Star10.1k

feat: show banner when workspace is outdated#4926

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

mtojek merged 3 commits intocoder:mainfrommtojek:4489-ssh-outdated-baner

Nov 7, 2022

Merged

feat: show banner when workspace is outdated#4926

mtojek merged 3 commits intocoder:mainfrommtojek:4489-ssh-outdated-baner

Nov 7, 2022

Conversation

Copy link

Member

mtojek commentedNov 7, 2022

Fixes:#4489

This PR introduces a CLI banner presented when the workspace, to which the user is trying to SSH, is outdated.

feat: show banner when workspace is outdated

aba2bd5

mtojek self-assigned this

Nov 7, 2022

mtojek marked this pull request as ready for review

November 7, 2022 11:09

mtojek requested a review froma team

November 7, 2022 11:09

mafredri reviewed

Nov 7, 2022

View reviewed changes

Copy link

Member

mafredri left a comment•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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 usingBannerCallback instead.

At least on systems with/etc/motd and OpenSSH, messages don't appear in non-interactive modes likessh host ls. I believe this should (automatically) be the case when usingBannerCallback too.

But to do that, we'd also need to fix all other occurrences of printing to stdout in thecoder 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)

Copy link

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

We should change this to print on stderr so that non-interactive commands likessh coder.workspace ls | grep hi aren't dirtied.

I wonder if we should consider only outputting when the recipient is a tty as well. 🤔

Copy link

MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Good catch!

Switched to stderr and added a TTY check.

Address PR comments

b50d2fe

Copy link

MemberAuthor

mtojek commentedNov 7, 2022

I think printing to stderr is a good first step, but in the future I think it would be a good idea to start usingBannerCallback instead.

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 entirecoder ssh isn't a rabbit hole. Let me know what you think.

mafredri reviewed

Nov 7, 2022

View reviewed changes

cli/ssh.go Outdated

		@@ -72,6 +73,11 @@ func ssh() *cobra.Command {
		return err
		}

		updateWorkspaceBanner, outdated := verifyWorkspaceOutdated(client, workspace)
		if outdated && isTTYOut(cmd) {

Copy link

Member

mafredriNov 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Ah, this tty check is only forstdout which doesn't help us (we want to confirm stderr.)

Made the following quick test:

❯ go run main.go >/dev/nullstdout isatty: falsestderr isatty: true❯ go run main.go 2>/dev/nullstdout isatty: truestderr isatty: false

Using:

package mainimport ("fmt""io""os""github.com/mattn/go-isatty")funcmain() {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.

Copy link

MemberAuthor

mtojekNov 7, 2022•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Thanks for the snippet. I refactored the originalisTTYOut function, but please let me know what you think.

Copy link

Member

mafredri commentedNov 7, 2022•
edited
Loading

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 entirecoder ssh isn't a rabbit hole. Let me know what you think.

Definitely. I'll create a separate issue for it. 👍🏻

Edit:#4927

mafredri mentioned this pull request

Nov 7, 2022

Use motd for all output incoder ssh [--stdio] commands#4927

Closed

Fix: writer

4e82d1e

mtojek requested a review frommafredri

November 7, 2022 14:27

mafredri approved these changes

Nov 7, 2022

View reviewed changes

cli/ssh_internal_test.go


		client := codersdk.Client{URL: serverURL}

		t.Run("Up-to-date", func(t *testing.T) {

Copy link

Member

mafredriNov 7, 2022•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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 thecoder/coder repo is write external tests (package cli_test).

With these types of tests, we'd do abit more setup for the test so that we can then test the output of runningcoder ssh ....

(Not a blocker for this PR, but just a heads up for future PRs.)

Copy link

MemberAuthor

mtojekNov 7, 2022•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Ok, I see now. I can use existing coderdtest APIs. I need to create a workspace to simulate a template update (CreateTemplateVersion). Something likesetupWorkspaceForAgent, 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?

Copy link

Member

bpmct commentedNov 7, 2022

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?

Copy link

MemberAuthor

mtojek commentedNov 7, 2022•
edited
Loading

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:

aftercoder config-ssh executed andLogLevel DEBUG,ssh coder.mtojek-1 gives:

...debug1: identity file /Users/mtojek/.ssh/id_dsatype -1debug1: identity file /Users/mtojek/.ssh/id_dsa-certtype -1debug1: Local version string SSH-2.0-OpenSSH_9.0debug1: 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"remote.SSH.logLevel": "trace" insettings.json, I'm able to find the message about the outdated env, so I guess that it answers your question. Unfortunately it's deep in logs:

Copy link

MemberAuthor

mtojek commentedNov 7, 2022

I'm going to merge this PR as we can always improve the experience in the next iterations.

mtojek merged commit641aacf intocoder:main

Nov 7, 2022

github-actionsbot locked and limited conversation to collaborators

Nov 7, 2022

Labels

None yet

3 participants

Movatterモバイル変換

feat: show banner when workspace is outdated#4926

feat: show banner when workspace is outdated#4926

Uh oh!

Conversation

mtojek commentedNov 7, 2022

Uh oh!

mafredri left a comment• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mafredriNov 7, 2022

Choose a reason for hiding this comment

Uh oh!

mtojekNov 7, 2022

Choose a reason for hiding this comment

Uh oh!

mtojek commentedNov 7, 2022

Uh oh!

mafredriNov 7, 2022

Choose a reason for hiding this comment

Uh oh!

mtojekNov 7, 2022• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mafredri commentedNov 7, 2022• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

mafredriNov 7, 2022• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtojekNov 7, 2022• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bpmct commentedNov 7, 2022

Uh oh!

mtojek commentedNov 7, 2022• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

mtojek commentedNov 7, 2022

Uh oh!

Uh oh!

mafredri left a comment•
edited
Loading

mtojekNov 7, 2022•
edited
Loading

mafredri commentedNov 7, 2022•
edited
Loading

mafredriNov 7, 2022•
edited
Loading

mtojekNov 7, 2022•
edited
Loading

mtojek commentedNov 7, 2022•
edited
Loading