Merge pull request #5319 from cwndrws/cwndrws/codespaces/handle-codespaces-with-pending-operations

[Codespaces] Disallow some operations on codespaces that have a pending operation

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"`
 }

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)
+	}
+}

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)
 	}

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
+		},
+	}
+}

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
 }
 

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)

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)

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)
+}

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)

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)
+}

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()

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)
+}