X Tutup
Skip to content

Commit bb4fc52

Browse files
cwndrwscwndrws
authored
Merge pull request cli#5319 from cwndrws/cwndrws/codespaces/handle-codespaces-with-pending-operations
[Codespaces] Disallow some operations on codespaces that have a pending operation
2 parents f3f1455 + 4eedfc0 commit bb4fc52

File tree

12 files changed

+318
-39
lines changed

12 files changed

+318
-39
lines changed

‎internal/codespaces/api/api.go‎

Lines changed: 37 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -167,18 +167,22 @@ func (a *API) GetRepository(ctx context.Context, nwo string) (*Repository, error
167167
}
168168

169169
// Codespace represents a codespace.
170+
// You can see more about the fields in this type in the codespaces api docs:
171+
// https://docs.github.com/en/rest/reference/codespaces
170172
type Codespace struct {
171-
Name string `json:"name"`
172-
CreatedAt string `json:"created_at"`
173-
DisplayName string `json:"display_name"`
174-
LastUsedAt string `json:"last_used_at"`
175-
Owner User `json:"owner"`
176-
Repository Repository `json:"repository"`
177-
State string `json:"state"`
178-
GitStatus CodespaceGitStatus `json:"git_status"`
179-
Connection CodespaceConnection `json:"connection"`
180-
Machine CodespaceMachine `json:"machine"`
181-
VSCSTarget string `json:"vscs_target"`
173+
Name string `json:"name"`
174+
CreatedAt string `json:"created_at"`
175+
DisplayName string `json:"display_name"`
176+
LastUsedAt string `json:"last_used_at"`
177+
Owner User `json:"owner"`
178+
Repository Repository `json:"repository"`
179+
State string `json:"state"`
180+
GitStatus CodespaceGitStatus `json:"git_status"`
181+
Connection CodespaceConnection `json:"connection"`
182+
Machine CodespaceMachine `json:"machine"`
183+
VSCSTarget string `json:"vscs_target"`
184+
PendingOperation bool `json:"pending_operation"`
185+
PendingOperationDisabledReason string `json:"pending_operation_disabled_reason"`
182186
}
183187

184188
type CodespaceGitStatus struct {
@@ -781,6 +785,20 @@ func (a *API) EditCodespace(ctx context.Context, codespaceName string, params *E
781785
defer resp.Body.Close()
782786

783787
if resp.StatusCode != http.StatusOK {
788+
// 422 (unprocessable entity) is likely caused by the codespace having a
789+
// pending op, so we'll fetch the codespace to see if that's the case
790+
// and return a more understandable error message.
791+
if resp.StatusCode == http.StatusUnprocessableEntity {
792+
pendingOp, reason, err := a.checkForPendingOperation(ctx, codespaceName)
793+
// If there's an error or there's not a pending op, we want to let
794+
// this fall through to the normal api.HandleHTTPError flow
795+
if err == nil && pendingOp {
796+
return nil, fmt.Errorf(
797+
"codespace is disabled while it has a pending operation: %s",
798+
reason,
799+
)
800+
}
801+
}
784802
return nil, api.HandleHTTPError(resp)
785803
}
786804

@@ -797,6 +815,14 @@ func (a *API) EditCodespace(ctx context.Context, codespaceName string, params *E
797815
return &response, nil
798816
}
799817

818+
func (a *API) checkForPendingOperation(ctx context.Context, codespaceName string) (bool, string, error) {
819+
codespace, err := a.GetCodespace(ctx, codespaceName, false)
820+
if err != nil {
821+
return false, "", err
822+
}
823+
return codespace.PendingOperation, codespace.PendingOperationDisabledReason, nil
824+
}
825+
800826
type getCodespaceRepositoryContentsResponse struct {
801827
Content string `json:"content"`
802828
}

‎internal/codespaces/api/api_test.go‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -388,6 +388,7 @@ func createFakeEditServer(t *testing.T, codespaceName string) *httptest.Server {
388388
fmt.Fprint(w, string(responseData))
389389
}))
390390
}
391+
391392
func TestAPI_EditCodespace(t *testing.T) {
392393
type args struct {
393394
ctx context.Context
@@ -434,3 +435,41 @@ func TestAPI_EditCodespace(t *testing.T) {
434435
})
435436
}
436437
}
438+
439+
func createFakeEditPendingOpServer(t *testing.T) *httptest.Server {
440+
return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
441+
if r.Method == http.MethodPatch {
442+
w.WriteHeader(http.StatusUnprocessableEntity)
443+
return
444+
}
445+
446+
if r.Method == http.MethodGet {
447+
response := Codespace{
448+
PendingOperation: true,
449+
PendingOperationDisabledReason: "Some pending operation",
450+
}
451+
452+
responseData, _ := json.Marshal(response)
453+
fmt.Fprint(w, string(responseData))
454+
return
455+
}
456+
}))
457+
}
458+
459+
func TestAPI_EditCodespacePendingOperation(t *testing.T) {
460+
svr := createFakeEditPendingOpServer(t)
461+
defer svr.Close()
462+
463+
a := &API{
464+
client: &http.Client{},
465+
githubAPI: svr.URL,
466+
}
467+
468+
_, err := a.EditCodespace(context.Background(), "disabledCodespace", &EditCodespaceParams{DisplayName: "some silly name"})
469+
if err == nil {
470+
t.Error("Expected pending operation error, but got nothing")
471+
}
472+
if err.Error() != "codespace is disabled while it has a pending operation: Some pending operation" {
473+
t.Errorf("Expected pending operation error, but got %v", err)
474+
}
475+
}

‎pkg/cmd/codespace/code.go‎

Lines changed: 4 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -31,18 +31,12 @@ func newCodeCmd(app *App) *cobra.Command {
3131

3232
// VSCode opens a codespace in the local VS VSCode application.
3333
func (a *App) VSCode(ctx context.Context, codespaceName string, useInsiders bool) error {
34-
if codespaceName == "" {
35-
codespace, err := chooseCodespace(ctx, a.apiClient)
36-
if err != nil {
37-
if err == errNoCodespaces {
38-
return err
39-
}
40-
return fmt.Errorf("error choosing codespace: %w", err)
41-
}
42-
codespaceName = codespace.Name
34+
codespace, err := getOrChooseCodespace(ctx, a.apiClient, codespaceName)
35+
if err != nil {
36+
return err
4337
}
4438

45-
url := vscodeProtocolURL(codespaceName, useInsiders)
39+
url := vscodeProtocolURL(codespace.Name, useInsiders)
4640
if err := a.browser.Browse(url); err != nil {
4741
return fmt.Errorf("error opening Visual Studio Code: %w", err)
4842
}

‎pkg/cmd/codespace/code_test.go‎

Lines changed: 47 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,9 @@ import (
44
"context"
55
"testing"
66

7+
"github.com/cli/cli/v2/internal/codespaces/api"
78
"github.com/cli/cli/v2/pkg/cmdutil"
9+
"github.com/cli/cli/v2/pkg/iostreams"
810
)
911

1012
func TestApp_VSCode(t *testing.T) {
@@ -41,7 +43,8 @@ func TestApp_VSCode(t *testing.T) {
4143
t.Run(tt.name, func(t *testing.T) {
4244
b := &cmdutil.TestBrowser{}
4345
a := &App{
44-
browser: b,
46+
browser: b,
47+
apiClient: testCodeApiMock(),
4548
}
4649
if err := a.VSCode(context.Background(), tt.args.codespaceName, tt.args.useInsiders); (err != nil) != tt.wantErr {
4750
t.Errorf("App.VSCode() error = %v, wantErr %v", err, tt.wantErr)
@@ -50,3 +53,46 @@ func TestApp_VSCode(t *testing.T) {
5053
})
5154
}
5255
}
56+
57+
func TestPendingOperationDisallowsCode(t *testing.T) {
58+
app := testingCodeApp()
59+
60+
if err := app.VSCode(context.Background(), "disabledCodespace", false); err != nil {
61+
if err.Error() != "codespace is disabled while it has a pending operation: Some pending operation" {
62+
t.Errorf("expected pending operation error, but got: %v", err)
63+
}
64+
} else {
65+
t.Error("expected pending operation error, but got nothing")
66+
}
67+
}
68+
69+
func testingCodeApp() *App {
70+
io, _, _, _ := iostreams.Test()
71+
return NewApp(io, nil, testCodeApiMock(), nil)
72+
}
73+
74+
func testCodeApiMock() *apiClientMock {
75+
user := &api.User{Login: "monalisa"}
76+
testingCodespace := &api.Codespace{
77+
Name: "monalisa-cli-cli-abcdef",
78+
}
79+
disabledCodespace := &api.Codespace{
80+
Name: "disabledCodespace",
81+
PendingOperation: true,
82+
PendingOperationDisabledReason: "Some pending operation",
83+
}
84+
return &apiClientMock{
85+
GetCodespaceFunc: func(_ context.Context, name string, _ bool) (*api.Codespace, error) {
86+
if name == "disabledCodespace" {
87+
return disabledCodespace, nil
88+
}
89+
return testingCodespace, nil
90+
},
91+
GetUserFunc: func(_ context.Context) (*api.User, error) {
92+
return user, nil
93+
},
94+
AuthorizedKeysFunc: func(_ context.Context, _ string) ([]byte, error) {
95+
return []byte{}, nil
96+
},
97+
}
98+
}

‎pkg/cmd/codespace/common.go‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -183,6 +183,13 @@ func getOrChooseCodespace(ctx context.Context, apiClient apiClient, codespaceNam
183183
}
184184
}
185185

186+
if codespace.PendingOperation {
187+
return nil, fmt.Errorf(
188+
"codespace is disabled while it has a pending operation: %s",
189+
codespace.PendingOperationDisabledReason,
190+
)
191+
}
192+
186193
return codespace, nil
187194
}
188195

‎pkg/cmd/codespace/list.go‎

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -89,10 +89,22 @@ func (a *App) List(ctx context.Context, limit int, exporter cmdutil.Exporter) er
8989

9090
formattedName := formatNameForVSCSTarget(c.Name, c.VSCSTarget)
9191

92-
tp.AddField(formattedName, nil, cs.Yellow)
92+
var nameColor func(string) string
93+
switch c.PendingOperation {
94+
case false:
95+
nameColor = cs.Yellow
96+
case true:
97+
nameColor = cs.Gray
98+
}
99+
100+
tp.AddField(formattedName, nil, nameColor)
93101
tp.AddField(c.Repository.FullName, nil, nil)
94102
tp.AddField(c.branchWithGitStatus(), nil, cs.Cyan)
95-
tp.AddField(c.State, nil, stateColor)
103+
if c.PendingOperation {
104+
tp.AddField(c.PendingOperationDisabledReason, nil, nameColor)
105+
} else {
106+
tp.AddField(c.State, nil, stateColor)
107+
}
96108

97109
if tp.IsTTY() {
98110
ct, err := time.Parse(time.RFC3339, c.CreatedAt)

‎pkg/cmd/codespace/logs.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ func (a *App) Logs(ctx context.Context, codespaceName string, follow bool) (err
3838

3939
codespace, err := getOrChooseCodespace(ctx, a.apiClient, codespaceName)
4040
if err != nil {
41-
return fmt.Errorf("get or choose codespace: %w", err)
41+
return err
4242
}
4343

4444
authkeys := make(chan error, 1)

‎pkg/cmd/codespace/logs_test.go‎

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
package codespace
2+
3+
import (
4+
"context"
5+
"testing"
6+
7+
"github.com/cli/cli/v2/internal/codespaces/api"
8+
"github.com/cli/cli/v2/pkg/iostreams"
9+
)
10+
11+
func TestPendingOperationDisallowsLogs(t *testing.T) {
12+
app := testingLogsApp()
13+
14+
if err := app.Logs(context.Background(), "disabledCodespace", false); err != nil {
15+
if err.Error() != "codespace is disabled while it has a pending operation: Some pending operation" {
16+
t.Errorf("expected pending operation error, but got: %v", err)
17+
}
18+
} else {
19+
t.Error("expected pending operation error, but got nothing")
20+
}
21+
}
22+
23+
func testingLogsApp() *App {
24+
user := &api.User{Login: "monalisa"}
25+
disabledCodespace := &api.Codespace{
26+
Name: "disabledCodespace",
27+
PendingOperation: true,
28+
PendingOperationDisabledReason: "Some pending operation",
29+
}
30+
apiMock := &apiClientMock{
31+
GetCodespaceFunc: func(_ context.Context, name string, _ bool) (*api.Codespace, error) {
32+
if name == "disabledCodespace" {
33+
return disabledCodespace, nil
34+
}
35+
return nil, nil
36+
},
37+
GetUserFunc: func(_ context.Context) (*api.User, error) {
38+
return user, nil
39+
},
40+
AuthorizedKeysFunc: func(_ context.Context, _ string) ([]byte, error) {
41+
return []byte{}, nil
42+
},
43+
}
44+
45+
io, _, _, _ := iostreams.Test()
46+
return NewApp(io, nil, apiMock, nil)
47+
}

‎pkg/cmd/codespace/ports.go‎

Lines changed: 3 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -48,11 +48,7 @@ func newPortsCmd(app *App) *cobra.Command {
4848
func (a *App) ListPorts(ctx context.Context, codespaceName string, exporter cmdutil.Exporter) (err error) {
4949
codespace, err := getOrChooseCodespace(ctx, a.apiClient, codespaceName)
5050
if err != nil {
51-
// TODO(josebalius): remove special handling of this error here and it other places
52-
if err == errNoCodespaces {
53-
return err
54-
}
55-
return fmt.Errorf("error choosing codespace: %w", err)
51+
return err
5652
}
5753

5854
devContainerCh := getDevContainer(ctx, a.apiClient, codespace)
@@ -237,10 +233,7 @@ func (a *App) UpdatePortVisibility(ctx context.Context, codespaceName string, ar
237233

238234
codespace, err := getOrChooseCodespace(ctx, a.apiClient, codespaceName)
239235
if err != nil {
240-
if err == errNoCodespaces {
241-
return err
242-
}
243-
return fmt.Errorf("error getting codespace: %w", err)
236+
return err
244237
}
245238

246239
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
313306

314307
codespace, err := getOrChooseCodespace(ctx, a.apiClient, codespaceName)
315308
if err != nil {
316-
if err == errNoCodespaces {
317-
return err
318-
}
319-
return fmt.Errorf("error getting codespace: %w", err)
309+
return err
320310
}
321311

322312
session, err := codespaces.ConnectToLiveshare(ctx, a, noopLogger(), a.apiClient, codespace)

0 commit comments

Comments
 (0)
X Tutup