From 3f65e5ae24c296423825a3d1ab557086ec84f4a2 Mon Sep 17 00:00:00 2001 From: Charlie Andrews Date: Tue, 15 Mar 2022 11:46:36 -0400 Subject: [PATCH 01/16] Add pending opertion and reason to codespace response struct --- internal/codespaces/api/api.go | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/internal/codespaces/api/api.go b/internal/codespaces/api/api.go index d8807ec74..e8cd0ac1f 100644 --- a/internal/codespaces/api/api.go +++ b/internal/codespaces/api/api.go @@ -168,17 +168,19 @@ func (a *API) GetRepository(ctx context.Context, nwo string) (*Repository, error // Codespace represents a codespace. type Codespace struct { - Name string `json:"name"` - CreatedAt string `json:"created_at"` - DisplayName string `json:"display_name"` - LastUsedAt string `json:"last_used_at"` - Owner User `json:"owner"` - Repository Repository `json:"repository"` - State string `json:"state"` - GitStatus CodespaceGitStatus `json:"git_status"` - Connection CodespaceConnection `json:"connection"` - Machine CodespaceMachine `json:"machine"` - VSCSTarget string `json:"vscs_target"` + Name string `json:"name"` + CreatedAt string `json:"created_at"` + DisplayName string `json:"display_name"` + LastUsedAt string `json:"last_used_at"` + Owner User `json:"owner"` + Repository Repository `json:"repository"` + State string `json:"state"` + GitStatus CodespaceGitStatus `json:"git_status"` + Connection CodespaceConnection `json:"connection"` + Machine CodespaceMachine `json:"machine"` + VSCSTarget string `json:"vscs_target"` + PendingOperation bool `json:"pending_operation"` + PendingOperationDisabledReason string `json:"pending_operation_disabled_reason"` } type CodespaceGitStatus struct { From 3d28c521044a39594683989f96bc4dd98f5fb910 Mon Sep 17 00:00:00 2001 From: Charlie Andrews Date: Tue, 15 Mar 2022 11:47:10 -0400 Subject: [PATCH 02/16] Mark codespace with pending op as disabled with reason instead of state --- pkg/cmd/codespace/list.go | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/pkg/cmd/codespace/list.go b/pkg/cmd/codespace/list.go index 17c7b6414..43eeb74e4 100644 --- a/pkg/cmd/codespace/list.go +++ b/pkg/cmd/codespace/list.go @@ -89,10 +89,22 @@ func (a *App) List(ctx context.Context, limit int, exporter cmdutil.Exporter) er formattedName := formatNameForVSCSTarget(c.Name, c.VSCSTarget) - tp.AddField(formattedName, nil, cs.Yellow) + var nameColor func(string) string + switch c.PendingOperation { + case false: + nameColor = cs.Yellow + case true: + nameColor = cs.Gray + } + + tp.AddField(formattedName, nil, nameColor) tp.AddField(c.Repository.FullName, nil, nil) tp.AddField(c.branchWithGitStatus(), nil, cs.Cyan) - tp.AddField(c.State, nil, stateColor) + if c.PendingOperation { + tp.AddField(c.PendingOperationDisabledReason, nil, cs.Gray) + } else { + tp.AddField(c.State, nil, stateColor) + } if tp.IsTTY() { ct, err := time.Parse(time.RFC3339, c.CreatedAt) From afa71c4b2fa7c99f0267a35046e8419c8a72db06 Mon Sep 17 00:00:00 2001 From: Charlie Andrews Date: Tue, 15 Mar 2022 12:21:46 -0400 Subject: [PATCH 03/16] Disallow ssh'ing to codespace with a pending operation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since the API already disallows this, this makes the error cleaner and more explicit when a user is trying to start/ssh into a codespace that has a pending operation. Example of the old error message: ``` $ gh cs ssh -c cwndrws-redacted Starting codespace ⣽error connecting to codespace: error starting codespace: HTTP 422: your codespace has an operation pending: updating to a sku with a different amount of storage; please wait until this operation is complete (https://api.github.com/user/codespaces/cwndrws-redacted/start) exit status 1 ``` Example of the new error message: ``` $ gh cs ssh -c cwndrws-redacted codespace is disabled while it has a pending operation: Changing machine types... exit status 1 ``` --- pkg/cmd/codespace/ssh.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/pkg/cmd/codespace/ssh.go b/pkg/cmd/codespace/ssh.go index 6ce783f2f..af6c86614 100644 --- a/pkg/cmd/codespace/ssh.go +++ b/pkg/cmd/codespace/ssh.go @@ -128,6 +128,13 @@ func (a *App) SSH(ctx context.Context, sshArgs []string, opts sshOptions) (err e return fmt.Errorf("get or choose codespace: %w", err) } + if codespace.PendingOperation { + return fmt.Errorf( + "codespace is disabled while it has a pending operation: %s", + codespace.PendingOperationDisabledReason, + ) + } + liveshareLogger := noopLogger() if opts.debug { debugLogger, err := newFileLogger(opts.debugFile) From 599c7c900f69642e0927011227ac8d683179cf20 Mon Sep 17 00:00:00 2001 From: Charlie Andrews Date: Tue, 15 Mar 2022 12:27:24 -0400 Subject: [PATCH 04/16] Disallow getting logs from codespaces with pending ops MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since the API already disallows this, this pretty much just cleans up the error reporting to the user. Example of old error: ``` $ gh cs logs -c cwndrws-redacted Starting codespace ⣽connecting to codespace: error starting codespace: HTTP 422: your codespace has an operation pending: updating to a sku with a different amount of storage; please wait until this operation is complete (https://api.github.com/user/codespaces/cwndrws-redacted/start) exit status 1 ``` Example of new error: ``` $ gh cs logs -c cwndrws-redacted codespace is disabled while it has a pending operation: Changing machine types... exit status 1 ``` --- pkg/cmd/codespace/logs.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/pkg/cmd/codespace/logs.go b/pkg/cmd/codespace/logs.go index d0a0c233b..221d95215 100644 --- a/pkg/cmd/codespace/logs.go +++ b/pkg/cmd/codespace/logs.go @@ -41,6 +41,13 @@ func (a *App) Logs(ctx context.Context, codespaceName string, follow bool) (err return fmt.Errorf("get or choose codespace: %w", err) } + if codespace.PendingOperation { + return fmt.Errorf( + "codespace is disabled while it has a pending operation: %s", + codespace.PendingOperationDisabledReason, + ) + } + authkeys := make(chan error, 1) go func() { authkeys <- checkAuthorizedKeys(ctx, a.apiClient) From da99a1b59bc6f13aa7246ccf8814de8528bb172b Mon Sep 17 00:00:00 2001 From: Charlie Andrews Date: Tue, 15 Mar 2022 14:11:33 -0400 Subject: [PATCH 05/16] Ensure codespace exists and doesn't have a pending op when opening Code The initial intention for this change was to disallow users to open a codespace in VS Code if the codespace has a pending operation. This also adds a side-benefit of presenting the user an error before waiting for VS Code to open if they provide an invalid codespace to open. --- pkg/cmd/codespace/code.go | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/pkg/cmd/codespace/code.go b/pkg/cmd/codespace/code.go index f80e32527..7b52db29f 100644 --- a/pkg/cmd/codespace/code.go +++ b/pkg/cmd/codespace/code.go @@ -31,18 +31,19 @@ func newCodeCmd(app *App) *cobra.Command { // VSCode opens a codespace in the local VS VSCode application. func (a *App) VSCode(ctx context.Context, codespaceName string, useInsiders bool) error { - if codespaceName == "" { - codespace, err := chooseCodespace(ctx, a.apiClient) - if err != nil { - if err == errNoCodespaces { - return err - } - return fmt.Errorf("error choosing codespace: %w", err) - } - codespaceName = codespace.Name + codespace, err := getOrChooseCodespace(ctx, a.apiClient, codespaceName) + if err != nil { + return fmt.Errorf("get or choose codespace: %w", err) } - url := vscodeProtocolURL(codespaceName, useInsiders) + if codespace.PendingOperation { + return fmt.Errorf( + "codespace is disabled while it has a pending operation: %s", + codespace.PendingOperationDisabledReason, + ) + } + + url := vscodeProtocolURL(codespace.Name, useInsiders) if err := a.browser.Browse(url); err != nil { return fmt.Errorf("error opening Visual Studio Code: %w", err) } From 5ffe838dce24b5456e804e76a349bdcc70f68eea Mon Sep 17 00:00:00 2001 From: Charlie Andrews Date: Tue, 15 Mar 2022 15:24:35 -0400 Subject: [PATCH 06/16] Disallow any port operations when codespace has pending operation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since all of the port operations require the codespace to be running, we need to disallow these operations when there's a pending op since we can't start the codespace in this state. Since the API already disallows this, this is basically cleaning up the error messages that the user sees in this state Old error message: ``` $ gh cs ports forward 80:80 ? Choose codespace: redacted Starting codespace ⣻error connecting to codespace: error starting codespace: HTTP 422: your codespace has an operation pending: updating to a sku with a different amount of storage; please wait until this operation is complete (https://api.github.com/user/codespaces/cwndrws-redacted/start) ``` New error message: ``` $ gh cs ports forward 80:80 ? Choose codespace: redacted codespace is disabled while it has a pending operation: Changing machine types... exit status 1 ``` --- pkg/cmd/codespace/ports.go | 42 +++++++++++++++++++++++--------------- 1 file changed, 26 insertions(+), 16 deletions(-) diff --git a/pkg/cmd/codespace/ports.go b/pkg/cmd/codespace/ports.go index 094833e30..cc4365742 100644 --- a/pkg/cmd/codespace/ports.go +++ b/pkg/cmd/codespace/ports.go @@ -46,13 +46,9 @@ func newPortsCmd(app *App) *cobra.Command { // ListPorts lists known ports in a codespace. func (a *App) ListPorts(ctx context.Context, codespaceName string, exporter cmdutil.Exporter) (err error) { - codespace, err := getOrChooseCodespace(ctx, a.apiClient, codespaceName) + codespace, err := getCodespaceForPorts(ctx, a.apiClient, codespaceName) if err != nil { - // TODO(josebalius): remove special handling of this error here and it other places - if err == errNoCodespaces { - return err - } - return fmt.Errorf("error choosing codespace: %w", err) + return err } devContainerCh := getDevContainer(ctx, a.apiClient, codespace) @@ -235,12 +231,9 @@ func (a *App) UpdatePortVisibility(ctx context.Context, codespaceName string, ar return fmt.Errorf("error parsing port arguments: %w", err) } - codespace, err := getOrChooseCodespace(ctx, a.apiClient, codespaceName) + codespace, err := getCodespaceForPorts(ctx, a.apiClient, codespaceName) if err != nil { - if err == errNoCodespaces { - return err - } - return fmt.Errorf("error getting codespace: %w", err) + return err } session, err := codespaces.ConnectToLiveshare(ctx, a, noopLogger(), a.apiClient, codespace) @@ -311,12 +304,9 @@ func (a *App) ForwardPorts(ctx context.Context, codespaceName string, ports []st return fmt.Errorf("get port pairs: %w", err) } - codespace, err := getOrChooseCodespace(ctx, a.apiClient, codespaceName) + codespace, err := getCodespaceForPorts(ctx, a.apiClient, codespaceName) if err != nil { - if err == errNoCodespaces { - return err - } - return fmt.Errorf("error getting codespace: %w", err) + return err } session, err := codespaces.ConnectToLiveshare(ctx, a, noopLogger(), a.apiClient, codespace) @@ -380,3 +370,23 @@ func normalizeJSON(j []byte) []byte { // remove trailing commas return bytes.ReplaceAll(j, []byte("},}"), []byte("}}")) } + +func getCodespaceForPorts(ctx context.Context, apiClient apiClient, codespaceName string) (*api.Codespace, error) { + codespace, err := getOrChooseCodespace(ctx, apiClient, codespaceName) + if err != nil { + // TODO(josebalius): remove special handling of this error here and it other places + if err == errNoCodespaces { + return nil, err + } + return nil, fmt.Errorf("error choosing codespace: %w", err) + } + + if codespace.PendingOperation { + return nil, fmt.Errorf( + "codespace is disabled while it has a pending operation: %s", + codespace.PendingOperationDisabledReason, + ) + } + + return codespace, nil +} From 27a5512b41aa4d000b1e62377332c40939d931a5 Mon Sep 17 00:00:00 2001 From: Charlie Andrews Date: Tue, 15 Mar 2022 16:58:50 -0400 Subject: [PATCH 07/16] Add test for disallowing ssh when codespace has a pending op --- pkg/cmd/codespace/ssh_test.go | 47 +++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+) create mode 100644 pkg/cmd/codespace/ssh_test.go diff --git a/pkg/cmd/codespace/ssh_test.go b/pkg/cmd/codespace/ssh_test.go new file mode 100644 index 000000000..3b242ae4b --- /dev/null +++ b/pkg/cmd/codespace/ssh_test.go @@ -0,0 +1,47 @@ +package codespace + +import ( + "context" + "testing" + + "github.com/cli/cli/v2/internal/codespaces/api" + "github.com/cli/cli/v2/pkg/iostreams" +) + +func TestPendingOperationDisallowsSSH(t *testing.T) { + app := testingSSHApp() + + if err := app.SSH(context.Background(), []string{}, sshOptions{codespace: "disabledCodespace"}); err != nil { + if err.Error() != "codespace is disabled while it has a pending operation: Some pending operation" { + t.Errorf("expected pending operation error, but got: %v", err) + } + } else { + t.Error("expected pending operation error, but got nothing") + } +} + +func testingSSHApp() *App { + user := &api.User{Login: "monalisa"} + disabledCodespace := &api.Codespace{ + Name: "disabledCodespace", + PendingOperation: true, + PendingOperationDisabledReason: "Some pending operation", + } + apiMock := &apiClientMock{ + GetCodespaceFunc: func(_ context.Context, name string, _ bool) (*api.Codespace, error) { + if name == "disabledCodespace" { + return disabledCodespace, nil + } + return nil, nil + }, + GetUserFunc: func(_ context.Context) (*api.User, error) { + return user, nil + }, + AuthorizedKeysFunc: func(_ context.Context, _ string) ([]byte, error) { + return []byte{}, nil + }, + } + + io, _, _, _ := iostreams.Test() + return NewApp(io, nil, apiMock, nil) +} From 6346779f3521090e053bf58c7decfb47c9ff243e Mon Sep 17 00:00:00 2001 From: Charlie Andrews Date: Tue, 15 Mar 2022 17:03:54 -0400 Subject: [PATCH 08/16] Add test for disallowing logs when codespace has a pending op --- pkg/cmd/codespace/logs_test.go | 47 ++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+) create mode 100644 pkg/cmd/codespace/logs_test.go diff --git a/pkg/cmd/codespace/logs_test.go b/pkg/cmd/codespace/logs_test.go new file mode 100644 index 000000000..1eaf2e9d9 --- /dev/null +++ b/pkg/cmd/codespace/logs_test.go @@ -0,0 +1,47 @@ +package codespace + +import ( + "context" + "testing" + + "github.com/cli/cli/v2/internal/codespaces/api" + "github.com/cli/cli/v2/pkg/iostreams" +) + +func TestPendingOperationDisallowsLogs(t *testing.T) { + app := testingLogsApp() + + if err := app.Logs(context.Background(), "disabledCodespace", false); err != nil { + if err.Error() != "codespace is disabled while it has a pending operation: Some pending operation" { + t.Errorf("expected pending operation error, but got: %v", err) + } + } else { + t.Error("expected pending operation error, but got nothing") + } +} + +func testingLogsApp() *App { + user := &api.User{Login: "monalisa"} + disabledCodespace := &api.Codespace{ + Name: "disabledCodespace", + PendingOperation: true, + PendingOperationDisabledReason: "Some pending operation", + } + apiMock := &apiClientMock{ + GetCodespaceFunc: func(_ context.Context, name string, _ bool) (*api.Codespace, error) { + if name == "disabledCodespace" { + return disabledCodespace, nil + } + return nil, nil + }, + GetUserFunc: func(_ context.Context) (*api.User, error) { + return user, nil + }, + AuthorizedKeysFunc: func(_ context.Context, _ string) ([]byte, error) { + return []byte{}, nil + }, + } + + io, _, _, _ := iostreams.Test() + return NewApp(io, nil, apiMock, nil) +} From f94a1a2bd49b7e6c7b747032b43214f45a4c7d45 Mon Sep 17 00:00:00 2001 From: Charlie Andrews Date: Tue, 15 Mar 2022 17:12:16 -0400 Subject: [PATCH 09/16] Add test for disallowing opening vs code for codespace with pending op I also refactored the existing vs code test a bit to work with the new use of `getOrChooseCodespace`. --- pkg/cmd/codespace/code_test.go | 48 +++++++++++++++++++++++++++++++++- 1 file changed, 47 insertions(+), 1 deletion(-) diff --git a/pkg/cmd/codespace/code_test.go b/pkg/cmd/codespace/code_test.go index cbc59864a..e35060c73 100644 --- a/pkg/cmd/codespace/code_test.go +++ b/pkg/cmd/codespace/code_test.go @@ -4,7 +4,9 @@ import ( "context" "testing" + "github.com/cli/cli/v2/internal/codespaces/api" "github.com/cli/cli/v2/pkg/cmdutil" + "github.com/cli/cli/v2/pkg/iostreams" ) func TestApp_VSCode(t *testing.T) { @@ -41,7 +43,8 @@ func TestApp_VSCode(t *testing.T) { t.Run(tt.name, func(t *testing.T) { b := &cmdutil.TestBrowser{} a := &App{ - browser: b, + browser: b, + apiClient: testCodeApiMock(), } if err := a.VSCode(context.Background(), tt.args.codespaceName, tt.args.useInsiders); (err != nil) != tt.wantErr { t.Errorf("App.VSCode() error = %v, wantErr %v", err, tt.wantErr) @@ -50,3 +53,46 @@ func TestApp_VSCode(t *testing.T) { }) } } + +func TestPendingOperationDisallowsCode(t *testing.T) { + app := testingLogsApp() + + if err := app.VSCode(context.Background(), "disabledCodespace", false); err != nil { + if err.Error() != "codespace is disabled while it has a pending operation: Some pending operation" { + t.Errorf("expected pending operation error, but got: %v", err) + } + } else { + t.Error("expected pending operation error, but got nothing") + } +} + +func testingCodeApp() *App { + io, _, _, _ := iostreams.Test() + return NewApp(io, nil, testCodeApiMock(), nil) +} + +func testCodeApiMock() *apiClientMock { + user := &api.User{Login: "monalisa"} + testingCodespace := &api.Codespace{ + Name: "monalisa-cli-cli-abcdef", + } + disabledCodespace := &api.Codespace{ + Name: "disabledCodespace", + PendingOperation: true, + PendingOperationDisabledReason: "Some pending operation", + } + return &apiClientMock{ + GetCodespaceFunc: func(_ context.Context, name string, _ bool) (*api.Codespace, error) { + if name == "disabledCodespace" { + return disabledCodespace, nil + } + return testingCodespace, nil + }, + GetUserFunc: func(_ context.Context) (*api.User, error) { + return user, nil + }, + AuthorizedKeysFunc: func(_ context.Context, _ string) ([]byte, error) { + return []byte{}, nil + }, + } +} From 3ed2e49bd95f324abbd01d09a72b7ab7d8274d97 Mon Sep 17 00:00:00 2001 From: Charlie Andrews Date: Tue, 15 Mar 2022 17:17:57 -0400 Subject: [PATCH 10/16] Add tests for disallowing all port commands for codespace w/ pending op --- pkg/cmd/codespace/ports_test.go | 71 +++++++++++++++++++++++++++++++++ 1 file changed, 71 insertions(+) create mode 100644 pkg/cmd/codespace/ports_test.go diff --git a/pkg/cmd/codespace/ports_test.go b/pkg/cmd/codespace/ports_test.go new file mode 100644 index 000000000..f1487ed2e --- /dev/null +++ b/pkg/cmd/codespace/ports_test.go @@ -0,0 +1,71 @@ +package codespace + +import ( + "context" + "testing" + + "github.com/cli/cli/v2/internal/codespaces/api" + "github.com/cli/cli/v2/pkg/iostreams" +) + +func TestPendingOperationDisallowsListPorts(t *testing.T) { + app := testingPortsApp() + + if err := app.ListPorts(context.Background(), "disabledCodespace", nil); err != nil { + if err.Error() != "codespace is disabled while it has a pending operation: Some pending operation" { + t.Errorf("expected pending operation error, but got: %v", err) + } + } else { + t.Error("expected pending operation error, but got nothing") + } +} + +func TestPendingOperationDisallowsUpdatePortVisability(t *testing.T) { + app := testingPortsApp() + + if err := app.UpdatePortVisibility(context.Background(), "disabledCodespace", nil); err != nil { + if err.Error() != "codespace is disabled while it has a pending operation: Some pending operation" { + t.Errorf("expected pending operation error, but got: %v", err) + } + } else { + t.Error("expected pending operation error, but got nothing") + } +} + +func TestPendingOperationDisallowsForwardPorts(t *testing.T) { + app := testingPortsApp() + + if err := app.ForwardPorts(context.Background(), "disabledCodespace", nil); err != nil { + if err.Error() != "codespace is disabled while it has a pending operation: Some pending operation" { + t.Errorf("expected pending operation error, but got: %v", err) + } + } else { + t.Error("expected pending operation error, but got nothing") + } +} + +func testingPortsApp() *App { + user := &api.User{Login: "monalisa"} + disabledCodespace := &api.Codespace{ + Name: "disabledCodespace", + PendingOperation: true, + PendingOperationDisabledReason: "Some pending operation", + } + apiMock := &apiClientMock{ + GetCodespaceFunc: func(_ context.Context, name string, _ bool) (*api.Codespace, error) { + if name == "disabledCodespace" { + return disabledCodespace, nil + } + return nil, nil + }, + GetUserFunc: func(_ context.Context) (*api.User, error) { + return user, nil + }, + AuthorizedKeysFunc: func(_ context.Context, _ string) ([]byte, error) { + return []byte{}, nil + }, + } + + io, _, _, _ := iostreams.Test() + return NewApp(io, nil, apiMock, nil) +} From 10e43b51364de4cad6d6d79bed3dd9d493bc86f2 Mon Sep 17 00:00:00 2001 From: Charlie Andrews Date: Wed, 16 Mar 2022 08:43:34 -0400 Subject: [PATCH 11/16] Use color variable instead of literal for disabled reason --- pkg/cmd/codespace/list.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cmd/codespace/list.go b/pkg/cmd/codespace/list.go index 43eeb74e4..41f44d299 100644 --- a/pkg/cmd/codespace/list.go +++ b/pkg/cmd/codespace/list.go @@ -101,7 +101,7 @@ func (a *App) List(ctx context.Context, limit int, exporter cmdutil.Exporter) er tp.AddField(c.Repository.FullName, nil, nil) tp.AddField(c.branchWithGitStatus(), nil, cs.Cyan) if c.PendingOperation { - tp.AddField(c.PendingOperationDisabledReason, nil, cs.Gray) + tp.AddField(c.PendingOperationDisabledReason, nil, nameColor) } else { tp.AddField(c.State, nil, stateColor) } From 8bf0cb8f13bafff98b6be64db208542ac4d65eb2 Mon Sep 17 00:00:00 2001 From: Charlie Andrews Date: Wed, 16 Mar 2022 08:51:33 -0400 Subject: [PATCH 12/16] Refactor the getOrChooseCodespace to always check for pending ops --- pkg/cmd/codespace/code.go | 7 ------- pkg/cmd/codespace/code_test.go | 2 +- pkg/cmd/codespace/common.go | 7 +++++++ pkg/cmd/codespace/logs.go | 7 ------- pkg/cmd/codespace/logs_test.go | 2 +- pkg/cmd/codespace/ports.go | 26 +++----------------------- pkg/cmd/codespace/ssh.go | 7 ------- pkg/cmd/codespace/ssh_test.go | 2 +- 8 files changed, 13 insertions(+), 47 deletions(-) diff --git a/pkg/cmd/codespace/code.go b/pkg/cmd/codespace/code.go index 7b52db29f..ba2455385 100644 --- a/pkg/cmd/codespace/code.go +++ b/pkg/cmd/codespace/code.go @@ -36,13 +36,6 @@ func (a *App) VSCode(ctx context.Context, codespaceName string, useInsiders bool return fmt.Errorf("get or choose codespace: %w", err) } - if codespace.PendingOperation { - return fmt.Errorf( - "codespace is disabled while it has a pending operation: %s", - codespace.PendingOperationDisabledReason, - ) - } - url := vscodeProtocolURL(codespace.Name, useInsiders) if err := a.browser.Browse(url); err != nil { return fmt.Errorf("error opening Visual Studio Code: %w", err) diff --git a/pkg/cmd/codespace/code_test.go b/pkg/cmd/codespace/code_test.go index e35060c73..0eb3b7ade 100644 --- a/pkg/cmd/codespace/code_test.go +++ b/pkg/cmd/codespace/code_test.go @@ -58,7 +58,7 @@ func TestPendingOperationDisallowsCode(t *testing.T) { app := testingLogsApp() if err := app.VSCode(context.Background(), "disabledCodespace", false); err != nil { - if err.Error() != "codespace is disabled while it has a pending operation: Some pending operation" { + if err.Error() != "get or choose codespace: codespace is disabled while it has a pending operation: Some pending operation" { t.Errorf("expected pending operation error, but got: %v", err) } } else { diff --git a/pkg/cmd/codespace/common.go b/pkg/cmd/codespace/common.go index 6061eac78..aec386f6c 100644 --- a/pkg/cmd/codespace/common.go +++ b/pkg/cmd/codespace/common.go @@ -183,6 +183,13 @@ func getOrChooseCodespace(ctx context.Context, apiClient apiClient, codespaceNam } } + if codespace.PendingOperation { + return nil, fmt.Errorf( + "codespace is disabled while it has a pending operation: %s", + codespace.PendingOperationDisabledReason, + ) + } + return codespace, nil } diff --git a/pkg/cmd/codespace/logs.go b/pkg/cmd/codespace/logs.go index 221d95215..d0a0c233b 100644 --- a/pkg/cmd/codespace/logs.go +++ b/pkg/cmd/codespace/logs.go @@ -41,13 +41,6 @@ func (a *App) Logs(ctx context.Context, codespaceName string, follow bool) (err return fmt.Errorf("get or choose codespace: %w", err) } - if codespace.PendingOperation { - return fmt.Errorf( - "codespace is disabled while it has a pending operation: %s", - codespace.PendingOperationDisabledReason, - ) - } - authkeys := make(chan error, 1) go func() { authkeys <- checkAuthorizedKeys(ctx, a.apiClient) diff --git a/pkg/cmd/codespace/logs_test.go b/pkg/cmd/codespace/logs_test.go index 1eaf2e9d9..225e10680 100644 --- a/pkg/cmd/codespace/logs_test.go +++ b/pkg/cmd/codespace/logs_test.go @@ -12,7 +12,7 @@ func TestPendingOperationDisallowsLogs(t *testing.T) { app := testingLogsApp() if err := app.Logs(context.Background(), "disabledCodespace", false); err != nil { - if err.Error() != "codespace is disabled while it has a pending operation: Some pending operation" { + if err.Error() != "get or choose codespace: codespace is disabled while it has a pending operation: Some pending operation" { t.Errorf("expected pending operation error, but got: %v", err) } } else { diff --git a/pkg/cmd/codespace/ports.go b/pkg/cmd/codespace/ports.go index cc4365742..274be8d7b 100644 --- a/pkg/cmd/codespace/ports.go +++ b/pkg/cmd/codespace/ports.go @@ -46,7 +46,7 @@ func newPortsCmd(app *App) *cobra.Command { // ListPorts lists known ports in a codespace. func (a *App) ListPorts(ctx context.Context, codespaceName string, exporter cmdutil.Exporter) (err error) { - codespace, err := getCodespaceForPorts(ctx, a.apiClient, codespaceName) + codespace, err := getOrChooseCodespace(ctx, a.apiClient, codespaceName) if err != nil { return err } @@ -231,7 +231,7 @@ func (a *App) UpdatePortVisibility(ctx context.Context, codespaceName string, ar return fmt.Errorf("error parsing port arguments: %w", err) } - codespace, err := getCodespaceForPorts(ctx, a.apiClient, codespaceName) + codespace, err := getOrChooseCodespace(ctx, a.apiClient, codespaceName) if err != nil { return err } @@ -304,7 +304,7 @@ func (a *App) ForwardPorts(ctx context.Context, codespaceName string, ports []st return fmt.Errorf("get port pairs: %w", err) } - codespace, err := getCodespaceForPorts(ctx, a.apiClient, codespaceName) + codespace, err := getOrChooseCodespace(ctx, a.apiClient, codespaceName) if err != nil { return err } @@ -370,23 +370,3 @@ func normalizeJSON(j []byte) []byte { // remove trailing commas return bytes.ReplaceAll(j, []byte("},}"), []byte("}}")) } - -func getCodespaceForPorts(ctx context.Context, apiClient apiClient, codespaceName string) (*api.Codespace, error) { - codespace, err := getOrChooseCodespace(ctx, apiClient, codespaceName) - if err != nil { - // TODO(josebalius): remove special handling of this error here and it other places - if err == errNoCodespaces { - return nil, err - } - return nil, fmt.Errorf("error choosing codespace: %w", err) - } - - if codespace.PendingOperation { - return nil, fmt.Errorf( - "codespace is disabled while it has a pending operation: %s", - codespace.PendingOperationDisabledReason, - ) - } - - return codespace, nil -} diff --git a/pkg/cmd/codespace/ssh.go b/pkg/cmd/codespace/ssh.go index af6c86614..6ce783f2f 100644 --- a/pkg/cmd/codespace/ssh.go +++ b/pkg/cmd/codespace/ssh.go @@ -128,13 +128,6 @@ func (a *App) SSH(ctx context.Context, sshArgs []string, opts sshOptions) (err e return fmt.Errorf("get or choose codespace: %w", err) } - if codespace.PendingOperation { - return fmt.Errorf( - "codespace is disabled while it has a pending operation: %s", - codespace.PendingOperationDisabledReason, - ) - } - liveshareLogger := noopLogger() if opts.debug { debugLogger, err := newFileLogger(opts.debugFile) diff --git a/pkg/cmd/codespace/ssh_test.go b/pkg/cmd/codespace/ssh_test.go index 3b242ae4b..fac5147bb 100644 --- a/pkg/cmd/codespace/ssh_test.go +++ b/pkg/cmd/codespace/ssh_test.go @@ -12,7 +12,7 @@ func TestPendingOperationDisallowsSSH(t *testing.T) { app := testingSSHApp() if err := app.SSH(context.Background(), []string{}, sshOptions{codespace: "disabledCodespace"}); err != nil { - if err.Error() != "codespace is disabled while it has a pending operation: Some pending operation" { + if err.Error() != "get or choose codespace: codespace is disabled while it has a pending operation: Some pending operation" { t.Errorf("expected pending operation error, but got: %v", err) } } else { From a2f76fdfe0e92e403ff301d668a96939c997b80b Mon Sep 17 00:00:00 2001 From: Charlie Andrews Date: Wed, 16 Mar 2022 09:10:54 -0400 Subject: [PATCH 13/16] Fix copy pasta error to appease the linter --- pkg/cmd/codespace/code_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cmd/codespace/code_test.go b/pkg/cmd/codespace/code_test.go index 0eb3b7ade..eda616a20 100644 --- a/pkg/cmd/codespace/code_test.go +++ b/pkg/cmd/codespace/code_test.go @@ -55,7 +55,7 @@ func TestApp_VSCode(t *testing.T) { } func TestPendingOperationDisallowsCode(t *testing.T) { - app := testingLogsApp() + app := testingCodeApp() if err := app.VSCode(context.Background(), "disabledCodespace", false); err != nil { if err.Error() != "get or choose codespace: codespace is disabled while it has a pending operation: Some pending operation" { From 64eecef17636627abbc20ff07ac09fcd1244637b Mon Sep 17 00:00:00 2001 From: Charlie Andrews Date: Wed, 16 Mar 2022 09:36:14 -0400 Subject: [PATCH 14/16] Remove unhelpful error wrapper --- pkg/cmd/codespace/code.go | 2 +- pkg/cmd/codespace/code_test.go | 2 +- pkg/cmd/codespace/logs.go | 2 +- pkg/cmd/codespace/logs_test.go | 2 +- pkg/cmd/codespace/ssh.go | 2 +- pkg/cmd/codespace/ssh_test.go | 2 +- 6 files changed, 6 insertions(+), 6 deletions(-) diff --git a/pkg/cmd/codespace/code.go b/pkg/cmd/codespace/code.go index ba2455385..0942d15d3 100644 --- a/pkg/cmd/codespace/code.go +++ b/pkg/cmd/codespace/code.go @@ -33,7 +33,7 @@ func newCodeCmd(app *App) *cobra.Command { func (a *App) VSCode(ctx context.Context, codespaceName string, useInsiders bool) error { codespace, err := getOrChooseCodespace(ctx, a.apiClient, codespaceName) if err != nil { - return fmt.Errorf("get or choose codespace: %w", err) + return err } url := vscodeProtocolURL(codespace.Name, useInsiders) diff --git a/pkg/cmd/codespace/code_test.go b/pkg/cmd/codespace/code_test.go index eda616a20..30f06bd39 100644 --- a/pkg/cmd/codespace/code_test.go +++ b/pkg/cmd/codespace/code_test.go @@ -58,7 +58,7 @@ func TestPendingOperationDisallowsCode(t *testing.T) { app := testingCodeApp() if err := app.VSCode(context.Background(), "disabledCodespace", false); err != nil { - if err.Error() != "get or choose codespace: codespace is disabled while it has a pending operation: Some pending operation" { + if err.Error() != "codespace is disabled while it has a pending operation: Some pending operation" { t.Errorf("expected pending operation error, but got: %v", err) } } else { diff --git a/pkg/cmd/codespace/logs.go b/pkg/cmd/codespace/logs.go index d0a0c233b..6feab3080 100644 --- a/pkg/cmd/codespace/logs.go +++ b/pkg/cmd/codespace/logs.go @@ -38,7 +38,7 @@ func (a *App) Logs(ctx context.Context, codespaceName string, follow bool) (err codespace, err := getOrChooseCodespace(ctx, a.apiClient, codespaceName) if err != nil { - return fmt.Errorf("get or choose codespace: %w", err) + return err } authkeys := make(chan error, 1) diff --git a/pkg/cmd/codespace/logs_test.go b/pkg/cmd/codespace/logs_test.go index 225e10680..1eaf2e9d9 100644 --- a/pkg/cmd/codespace/logs_test.go +++ b/pkg/cmd/codespace/logs_test.go @@ -12,7 +12,7 @@ func TestPendingOperationDisallowsLogs(t *testing.T) { app := testingLogsApp() if err := app.Logs(context.Background(), "disabledCodespace", false); err != nil { - if err.Error() != "get or choose codespace: codespace is disabled while it has a pending operation: Some pending operation" { + if err.Error() != "codespace is disabled while it has a pending operation: Some pending operation" { t.Errorf("expected pending operation error, but got: %v", err) } } else { diff --git a/pkg/cmd/codespace/ssh.go b/pkg/cmd/codespace/ssh.go index 6ce783f2f..dfbc75694 100644 --- a/pkg/cmd/codespace/ssh.go +++ b/pkg/cmd/codespace/ssh.go @@ -125,7 +125,7 @@ func (a *App) SSH(ctx context.Context, sshArgs []string, opts sshOptions) (err e codespace, err := getOrChooseCodespace(ctx, a.apiClient, opts.codespace) if err != nil { - return fmt.Errorf("get or choose codespace: %w", err) + return err } liveshareLogger := noopLogger() diff --git a/pkg/cmd/codespace/ssh_test.go b/pkg/cmd/codespace/ssh_test.go index fac5147bb..3b242ae4b 100644 --- a/pkg/cmd/codespace/ssh_test.go +++ b/pkg/cmd/codespace/ssh_test.go @@ -12,7 +12,7 @@ func TestPendingOperationDisallowsSSH(t *testing.T) { app := testingSSHApp() if err := app.SSH(context.Background(), []string{}, sshOptions{codespace: "disabledCodespace"}); err != nil { - if err.Error() != "get or choose codespace: codespace is disabled while it has a pending operation: Some pending operation" { + if err.Error() != "codespace is disabled while it has a pending operation: Some pending operation" { t.Errorf("expected pending operation error, but got: %v", err) } } else { From 7ca31e02b4feb25c903b7811fec0745815742320 Mon Sep 17 00:00:00 2001 From: Charlie Andrews Date: Wed, 16 Mar 2022 10:13:46 -0400 Subject: [PATCH 15/16] Disallow editing a codespace that has a pending operation This is already prevented by the API, but this will make the error message cleaner and easier to understand for the user. Example of old error message: ``` $ gh cs edit -c cwndrws-redacted -d "some silly name" error editing codespace: HTTP 422: your codespace has an operation pending: updating to a sku with a different amount of storage; please wait until this operation is complete (https://api.github.com/user/codespaces/cwndrws-redacted) exit status 1 ``` Example of new error message: ``` $ gh cs edit -c cwndrws-redacted -d "some silly name" error editing codespace: codespace is disabled while it has a pending operation: Changing machine types... exit status 1 ``` --- internal/codespaces/api/api.go | 22 ++++++++++++++++ internal/codespaces/api/api_test.go | 39 +++++++++++++++++++++++++++++ 2 files changed, 61 insertions(+) diff --git a/internal/codespaces/api/api.go b/internal/codespaces/api/api.go index e8cd0ac1f..e9ef4c339 100644 --- a/internal/codespaces/api/api.go +++ b/internal/codespaces/api/api.go @@ -783,6 +783,20 @@ func (a *API) EditCodespace(ctx context.Context, codespaceName string, params *E defer resp.Body.Close() if resp.StatusCode != http.StatusOK { + // 422 (unprocessable entity) is likely caused by the codespace having a + // pending op, so we'll fetch the codespace to see if that's the case + // and return a more understandable error message. + if resp.StatusCode == http.StatusUnprocessableEntity { + pendingOp, reason, err := a.checkForPendingOperation(ctx, codespaceName) + // If there's an error or there's not a pending op, we want to let + // this fall through to the normal api.HandleHTTPError flow + if err == nil && pendingOp { + return nil, fmt.Errorf( + "codespace is disabled while it has a pending operation: %s", + reason, + ) + } + } return nil, api.HandleHTTPError(resp) } @@ -799,6 +813,14 @@ func (a *API) EditCodespace(ctx context.Context, codespaceName string, params *E return &response, nil } +func (a *API) checkForPendingOperation(ctx context.Context, codespaceName string) (bool, string, error) { + codespace, err := a.GetCodespace(ctx, codespaceName, false) + if err != nil { + return false, "", err + } + return codespace.PendingOperation, codespace.PendingOperationDisabledReason, nil +} + type getCodespaceRepositoryContentsResponse struct { Content string `json:"content"` } diff --git a/internal/codespaces/api/api_test.go b/internal/codespaces/api/api_test.go index ebbbe5209..81eb563c3 100644 --- a/internal/codespaces/api/api_test.go +++ b/internal/codespaces/api/api_test.go @@ -388,6 +388,7 @@ func createFakeEditServer(t *testing.T, codespaceName string) *httptest.Server { fmt.Fprint(w, string(responseData)) })) } + func TestAPI_EditCodespace(t *testing.T) { type args struct { ctx context.Context @@ -434,3 +435,41 @@ func TestAPI_EditCodespace(t *testing.T) { }) } } + +func createFakeEditPendingOpServer(t *testing.T) *httptest.Server { + return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method == http.MethodPatch { + w.WriteHeader(http.StatusUnprocessableEntity) + return + } + + if r.Method == http.MethodGet { + response := Codespace{ + PendingOperation: true, + PendingOperationDisabledReason: "Some pending operation", + } + + responseData, _ := json.Marshal(response) + fmt.Fprint(w, string(responseData)) + return + } + })) +} + +func TestAPI_EditCodespacePendingOperation(t *testing.T) { + svr := createFakeEditPendingOpServer(t) + defer svr.Close() + + a := &API{ + client: &http.Client{}, + githubAPI: svr.URL, + } + + _, err := a.EditCodespace(context.Background(), "disabledCodespace", &EditCodespaceParams{DisplayName: "some silly name"}) + if err == nil { + t.Error("Expected pending operation error, but got nothing") + } + if err.Error() != "codespace is disabled while it has a pending operation: Some pending operation" { + t.Errorf("Expected pending operation error, but got %v", err) + } +} From 4eedfc05c1f672aeb0f75f29eb5bb0c762abb8be Mon Sep 17 00:00:00 2001 From: Charlie Andrews Date: Wed, 16 Mar 2022 10:20:13 -0400 Subject: [PATCH 16/16] Add docs link comment to Codespaces struct definition --- internal/codespaces/api/api.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/internal/codespaces/api/api.go b/internal/codespaces/api/api.go index e9ef4c339..e8c76e6be 100644 --- a/internal/codespaces/api/api.go +++ b/internal/codespaces/api/api.go @@ -167,6 +167,8 @@ func (a *API) GetRepository(ctx context.Context, nwo string) (*Repository, error } // Codespace represents a codespace. +// You can see more about the fields in this type in the codespaces api docs: +// https://docs.github.com/en/rest/reference/codespaces type Codespace struct { Name string `json:"name"` CreatedAt string `json:"created_at"`