diff --git a/internal/codespaces/api/api.go b/internal/codespaces/api/api.go index d8807ec74..e8c76e6be 100644 --- a/internal/codespaces/api/api.go +++ b/internal/codespaces/api/api.go @@ -167,18 +167,22 @@ 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"` - 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 { @@ -781,6 +785,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) } @@ -797,6 +815,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) + } +} diff --git a/pkg/cmd/codespace/code.go b/pkg/cmd/codespace/code.go index f80e32527..0942d15d3 100644 --- a/pkg/cmd/codespace/code.go +++ b/pkg/cmd/codespace/code.go @@ -31,18 +31,12 @@ 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 err } - url := vscodeProtocolURL(codespaceName, useInsiders) + 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 cbc59864a..30f06bd39 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 := testingCodeApp() + + 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 + }, + } +} 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/list.go b/pkg/cmd/codespace/list.go index 17c7b6414..41f44d299 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, nameColor) + } else { + tp.AddField(c.State, nil, stateColor) + } if tp.IsTTY() { ct, err := time.Parse(time.RFC3339, c.CreatedAt) 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 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) +} diff --git a/pkg/cmd/codespace/ports.go b/pkg/cmd/codespace/ports.go index 094833e30..274be8d7b 100644 --- a/pkg/cmd/codespace/ports.go +++ b/pkg/cmd/codespace/ports.go @@ -48,11 +48,7 @@ func newPortsCmd(app *App) *cobra.Command { func (a *App) ListPorts(ctx context.Context, codespaceName string, exporter cmdutil.Exporter) (err error) { codespace, err := getOrChooseCodespace(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) @@ -237,10 +233,7 @@ func (a *App) UpdatePortVisibility(ctx context.Context, codespaceName string, ar codespace, err := getOrChooseCodespace(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) @@ -313,10 +306,7 @@ func (a *App) ForwardPorts(ctx context.Context, codespaceName string, ports []st codespace, err := getOrChooseCodespace(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) 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) +} 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 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) +}