Skip to content

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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Nov 7, 2022
Merged

feat: show banner when workspace is outdated #4926

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
aba2bd5
feat: show banner when workspace is outdated
mtojek Nov 7, 2022
b50d2fe
Address PR comments
mtojek Nov 7, 2022
4e82d1e
Fix: writer
mtojek Nov 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions cli/ssh.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"errors"
"fmt"
"io"
"net/url"
"os"
"path/filepath"
"strings"
Expand Down Expand Up @@ -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 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. 🤔

Copy link
Member Author

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.

}

// OpenSSH passes stderr directly to the calling TTY.
// This is required in "stdio" mode so a connecting indicator can be displayed.
err = cliui.Agent(ctx, cmd.ErrOrStderr(), cliui.AgentOptions{
Expand Down Expand Up @@ -343,3 +349,18 @@ func notifyCondition(ctx context.Context, client *codersdk.Client, workspaceID u
return deadline.Truncate(time.Minute), callback
}
}

// Verify if the user workspace is outdated and prepare an actionable message for user.
func verifyWorkspaceOutdated(client *codersdk.Client, workspace codersdk.Workspace) (string, bool) {
if !workspace.Outdated {
return "", false // workspace is up-to-date
}

workspaceLink := buildWorkspaceLink(client.URL, workspace)
return fmt.Sprintf("👋 Your workspace is outdated! Update it here: %s\n", workspaceLink), true
}

// Build the user workspace link which navigates to the Coder web UI.
func buildWorkspaceLink(serverURL *url.URL, workspace codersdk.Workspace) *url.URL {
return serverURL.ResolveReference(&url.URL{Path: fmt.Sprintf("@%s/%s", workspace.OwnerName, workspace.Name)})
}
58 changes: 58 additions & 0 deletions cli/ssh_internal_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
package cli

import (
"net/url"
"testing"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

"github.com/coder/coder/codersdk"
)

const (
fakeOwnerName = "fake-owner-name"
fakeServerURL = "https://fake-foo-url"
fakeWorkspaceName = "fake-workspace-name"
)

func TestVerifyWorkspaceOutdated(t *testing.T) {
t.Parallel()

serverURL, err := url.Parse(fakeServerURL)
require.NoError(t, err)

client := codersdk.Client{URL: serverURL}

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

@mafredri mafredri Nov 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 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.)

Copy link
Member Author

@mtojek mtojek Nov 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 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?

t.Parallel()

workspace := codersdk.Workspace{Name: fakeWorkspaceName, OwnerName: fakeOwnerName}

_, outdated := verifyWorkspaceOutdated(&client, workspace)

assert.False(t, outdated, "workspace should be up-to-date")
})
t.Run("Outdated", func(t *testing.T) {
t.Parallel()

workspace := codersdk.Workspace{Name: fakeWorkspaceName, OwnerName: fakeOwnerName, Outdated: true}

updateWorkspaceBanner, outdated := verifyWorkspaceOutdated(&client, workspace)

assert.True(t, outdated, "workspace should be outdated")
assert.NotEmpty(t, updateWorkspaceBanner, "workspace banner should be present")
})
}

func TestBuildWorkspaceLink(t *testing.T) {
t.Parallel()

serverURL, err := url.Parse(fakeServerURL)
require.NoError(t, err)

workspace := codersdk.Workspace{Name: fakeWorkspaceName, OwnerName: fakeOwnerName}
workspaceLink := buildWorkspaceLink(serverURL, workspace)

assert.Equal(t, workspaceLink.String(), fakeServerURL+"/@"+fakeOwnerName+"/"+fakeWorkspaceName)
}