From e7f888ad1d5a959fc9b84808d14e1ee7a54075aa Mon Sep 17 00:00:00 2001 From: Jeff Hubbard Date: Fri, 21 Jan 2022 10:20:15 -0800 Subject: [PATCH 01/10] Add devcontainer_path API param as an option --- internal/codespaces/api/api.go | 3 +++ pkg/cmd/codespace/create.go | 13 ++++++++----- pkg/cmd/codespace/create_test.go | 14 +++++++++----- 3 files changed, 20 insertions(+), 10 deletions(-) diff --git a/internal/codespaces/api/api.go b/internal/codespaces/api/api.go index 8de16bbb8..ed86a2c6c 100644 --- a/internal/codespaces/api/api.go +++ b/internal/codespaces/api/api.go @@ -481,6 +481,7 @@ type CreateCodespaceParams struct { Branch string Machine string Location string + DevContainerPath string } // CreateCodespace creates a codespace with the given parameters and returns a non-nil error if it @@ -526,6 +527,7 @@ type startCreateRequest struct { Ref string `json:"ref"` Location string `json:"location"` Machine string `json:"machine"` + DevContainerPath string `json:"devcontainer_path,omitempty"` } var errProvisioningInProgress = errors.New("provisioning in progress") @@ -545,6 +547,7 @@ func (a *API) startCreate(ctx context.Context, params *CreateCodespaceParams) (* Ref: params.Branch, Location: params.Location, Machine: params.Machine, + DevContainerPath: params.DevContainerPath, }) if err != nil { return nil, fmt.Errorf("error marshaling request: %w", err) diff --git a/pkg/cmd/codespace/create.go b/pkg/cmd/codespace/create.go index ee9d81fe7..8f884f8da 100644 --- a/pkg/cmd/codespace/create.go +++ b/pkg/cmd/codespace/create.go @@ -13,11 +13,12 @@ import ( ) type createOptions struct { - repo string - branch string - machine string - showStatus bool - idleTimeout time.Duration + repo string + branch string + machine string + showStatus bool + idleTimeout time.Duration + devContainerPath string } func newCreateCmd(app *App) *cobra.Command { @@ -37,6 +38,7 @@ func newCreateCmd(app *App) *cobra.Command { createCmd.Flags().StringVarP(&opts.machine, "machine", "m", "", "hardware specifications for the VM") createCmd.Flags().BoolVarP(&opts.showStatus, "status", "s", false, "show status of post-create command and dotfiles") createCmd.Flags().DurationVar(&opts.idleTimeout, "idle-timeout", 0, "allowed inactivity before codespace is stopped, e.g. \"10m\", \"1h\"") + createCmd.Flags().StringVar(&opts.devContainerPath, "devcontainer-path", "", "path to the devcontainer.json file to use when creating codespace") return createCmd } @@ -109,6 +111,7 @@ func (a *App) Create(ctx context.Context, opts createOptions) error { Machine: machine, Location: locationResult.Location, IdleTimeoutMinutes: int(opts.idleTimeout.Minutes()), + DevContainerPath: opts.devContainerPath, }) a.StopProgressIndicator() if err != nil { diff --git a/pkg/cmd/codespace/create_test.go b/pkg/cmd/codespace/create_test.go index 4b266ffaa..c5c1ea075 100644 --- a/pkg/cmd/codespace/create_test.go +++ b/pkg/cmd/codespace/create_test.go @@ -51,6 +51,9 @@ func TestApp_Create(t *testing.T) { if params.IdleTimeoutMinutes != 30 { return nil, fmt.Errorf("idle timeout minutes was %v", params.IdleTimeoutMinutes) } + if params.DevContainerPath != ".devcontainer/foobar/devcontainer.json" { + return nil, fmt.Errorf("got dev container path %q, want %q", params.DevContainerPath, ".devcontainer/foobar/devcontainer.json") + } return &api.Codespace{ Name: "monalisa-dotfiles-abcd1234", }, nil @@ -58,11 +61,12 @@ func TestApp_Create(t *testing.T) { }, }, opts: createOptions{ - repo: "monalisa/dotfiles", - branch: "", - machine: "GIGA", - showStatus: false, - idleTimeout: 30 * time.Minute, + repo: "monalisa/dotfiles", + branch: "", + machine: "GIGA", + showStatus: false, + idleTimeout: 30 * time.Minute, + devContainerPath: ".devcontainer/foobar/devcontainer.json", }, wantStdout: "monalisa-dotfiles-abcd1234\n", }, From 5cee9e16f3023eaf17f70663055ddae39ddbf363 Mon Sep 17 00:00:00 2001 From: Jeff Hubbard Date: Fri, 21 Jan 2022 14:11:52 -0800 Subject: [PATCH 02/10] Add interactive prompt to choose from list of available devcontainer files --- internal/codespaces/api/api.go | 58 +++++++++++++++++++++ pkg/cmd/codespace/common.go | 1 + pkg/cmd/codespace/create.go | 33 +++++++++++- pkg/cmd/codespace/create_test.go | 89 +++++++++++++++++++++++++++++++- pkg/cmd/codespace/mock_api.go | 61 ++++++++++++++++++++++ 5 files changed, 240 insertions(+), 2 deletions(-) diff --git a/internal/codespaces/api/api.go b/internal/codespaces/api/api.go index ed86a2c6c..6b31f0f69 100644 --- a/internal/codespaces/api/api.go +++ b/internal/codespaces/api/api.go @@ -605,6 +605,64 @@ func (a *API) DeleteCodespace(ctx context.Context, codespaceName string) error { return nil } +// ListDevContainers returns a list of valid devcontainer.json files for the repo. Pass a negative limit to request all pages from +// the API until all devcontainer.json files have been fetched. +func (a *API) ListDevContainers(ctx context.Context, repoID int, branch string, limit int) (devcontainers []string, err error) { + perPage := 100 + if limit > 0 && limit < 100 { + perPage = limit + } + + listURL := fmt.Sprintf("%s/repositories/%d/codespaces/devcontainers?per_page=%d", a.githubAPI, repoID, perPage) + if branch != "" { + listURL += "&ref=" + branch + } + for { + req, err := http.NewRequest(http.MethodGet, listURL, nil) + if err != nil { + return nil, fmt.Errorf("error creating request: %w", err) + } + a.setHeaders(req) + + resp, err := a.do(ctx, req, fmt.Sprintf("/repositories/%d/codespaces/devcontainers", repoID)) + if err != nil { + return nil, fmt.Errorf("error making request: %w", err) + } + defer resp.Body.Close() + + if resp.StatusCode != http.StatusOK { + return nil, api.HandleHTTPError(resp) + } + + var response struct { + Devcontainers []string `json:"devcontainers"` + } + dec := json.NewDecoder(resp.Body) + if err := dec.Decode(&response); err != nil { + return nil, fmt.Errorf("error unmarshaling response: %w", err) + } + + nextURL := findNextPage(resp.Header.Get("Link")) + devcontainers = append(devcontainers, response.Devcontainers...) + + if nextURL == "" || (limit > 0 && len(devcontainers) >= limit) { + break + } + + if newPerPage := limit - len(devcontainers); limit > 0 && newPerPage < 100 { + u, _ := url.Parse(nextURL) + q := u.Query() + q.Set("per_page", strconv.Itoa(newPerPage)) + u.RawQuery = q.Encode() + listURL = u.String() + } else { + listURL = nextURL + } + } + + return devcontainers, nil +} + type getCodespaceRepositoryContentsResponse struct { Content string `json:"content"` } diff --git a/pkg/cmd/codespace/common.go b/pkg/cmd/codespace/common.go index 1107ae6a5..20d9cfe20 100644 --- a/pkg/cmd/codespace/common.go +++ b/pkg/cmd/codespace/common.go @@ -67,6 +67,7 @@ type apiClient interface { GetCodespaceRegionLocation(ctx context.Context) (string, error) GetCodespacesMachines(ctx context.Context, repoID int, branch, location string) ([]*api.Machine, error) GetCodespaceRepositoryContents(ctx context.Context, codespace *api.Codespace, path string) ([]byte, error) + ListDevContainers(ctx context.Context, repoID int, branch string, limit int) (devcontainers []string, err error) } var errNoCodespaces = errors.New("you have no codespaces") diff --git a/pkg/cmd/codespace/create.go b/pkg/cmd/codespace/create.go index 8f884f8da..7964fb1c0 100644 --- a/pkg/cmd/codespace/create.go +++ b/pkg/cmd/codespace/create.go @@ -91,6 +91,37 @@ func (a *App) Create(ctx context.Context, opts createOptions) error { branch = repository.DefaultBranch } + devContainerPath := opts.devContainerPath + + // now that we have repo+branch, we can list available devcontainer.json files (if any) + if len(opts.devContainerPath) < 1 { + a.StartProgressIndicatorWithLabel("Fetching devcontainer.json files") + devContainerPaths, err := a.apiClient.ListDevContainers(ctx, repository.ID, branch, 100) + if err != nil { + return fmt.Errorf("error getting devcontainer.json paths: %w", err) + } + a.StopProgressIndicator() + + if len(devContainerPaths) > 0 { + devContainerPathQuestion := &survey.Question{ + Name: "devContainerPath", + Prompt: &survey.Select{ + Message: "Devcontainer.json file:", + Options: append([]string{"none"}, devContainerPaths...), + }, + } + + if err := ask([]*survey.Question{devContainerPathQuestion}, &devContainerPath); err != nil { + return fmt.Errorf("failed to prompt: %w", err) + } + } + + if devContainerPath == "none" { + // special arg allows users to opt out of devcontainer.json selection + devContainerPath = "" + } + } + locationResult := <-locationCh if locationResult.Err != nil { return fmt.Errorf("error getting codespace region location: %w", locationResult.Err) @@ -111,7 +142,7 @@ func (a *App) Create(ctx context.Context, opts createOptions) error { Machine: machine, Location: locationResult.Location, IdleTimeoutMinutes: int(opts.idleTimeout.Minutes()), - DevContainerPath: opts.devContainerPath, + DevContainerPath: devContainerPath, }) a.StopProgressIndicator() if err != nil { diff --git a/pkg/cmd/codespace/create_test.go b/pkg/cmd/codespace/create_test.go index c5c1ea075..0a7db6294 100644 --- a/pkg/cmd/codespace/create_test.go +++ b/pkg/cmd/codespace/create_test.go @@ -19,6 +19,7 @@ func TestApp_Create(t *testing.T) { fields fields opts createOptions wantErr bool + wantErrMsg string wantStdout string wantStderr string }{ @@ -70,17 +71,103 @@ func TestApp_Create(t *testing.T) { }, wantStdout: "monalisa-dotfiles-abcd1234\n", }, + { + name: "create codespace with default branch with default devcontainer if no path provided", + fields: fields{ + apiClient: &apiClientMock{ + GetCodespaceRegionLocationFunc: func(ctx context.Context) (string, error) { + return "EUROPE", nil + }, + GetRepositoryFunc: func(ctx context.Context, nwo string) (*api.Repository, error) { + return &api.Repository{ + ID: 1234, + FullName: nwo, + DefaultBranch: "main", + }, nil + }, + ListDevContainersFunc: func(ctx context.Context, repoID int, branch string, limit int) ([]string, error) { + return []string{}, nil + }, + GetCodespacesMachinesFunc: func(ctx context.Context, repoID int, branch, location string) ([]*api.Machine, error) { + return []*api.Machine{ + { + Name: "GIGA", + DisplayName: "Gigabits of a machine", + }, + }, nil + }, + CreateCodespaceFunc: func(ctx context.Context, params *api.CreateCodespaceParams) (*api.Codespace, error) { + if params.Branch != "main" { + return nil, fmt.Errorf("got branch %q, want %q", params.Branch, "main") + } + if params.IdleTimeoutMinutes != 30 { + return nil, fmt.Errorf("idle timeout minutes was %v", params.IdleTimeoutMinutes) + } + if params.DevContainerPath != "" { + return nil, fmt.Errorf("got dev container path %q, want %q", params.DevContainerPath, ".devcontainer/foobar/devcontainer.json") + } + return &api.Codespace{ + Name: "monalisa-dotfiles-abcd1234", + }, nil + }, + }, + }, + opts: createOptions{ + repo: "monalisa/dotfiles", + branch: "", + machine: "GIGA", + showStatus: false, + idleTimeout: 30 * time.Minute, + }, + wantStdout: "monalisa-dotfiles-abcd1234\n", + }, + { + name: "returns error when getting devcontainer paths fails", + fields: fields{ + apiClient: &apiClientMock{ + GetCodespaceRegionLocationFunc: func(ctx context.Context) (string, error) { + return "EUROPE", nil + }, + GetRepositoryFunc: func(ctx context.Context, nwo string) (*api.Repository, error) { + return &api.Repository{ + ID: 1234, + FullName: nwo, + DefaultBranch: "main", + }, nil + }, + ListDevContainersFunc: func(ctx context.Context, repoID int, branch string, limit int) ([]string, error) { + return nil, fmt.Errorf("some error") + }, + }, + }, + opts: createOptions{ + repo: "monalisa/dotfiles", + branch: "", + machine: "GIGA", + showStatus: false, + idleTimeout: 30 * time.Minute, + }, + wantErr: true, + wantErrMsg: "error getting devcontainer.json paths: some error", + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { io, _, stdout, stderr := iostreams.Test() + io.SetStdinTTY(true) + io.SetStdoutTTY(true) + a := &App{ io: io, apiClient: tt.fields.apiClient, } - if err := a.Create(context.Background(), tt.opts); (err != nil) != tt.wantErr { + err := a.Create(context.Background(), tt.opts) + if (err != nil) != tt.wantErr { t.Errorf("App.Create() error = %v, wantErr %v", err, tt.wantErr) } + if tt.wantErrMsg != "" && err.Error() != tt.wantErrMsg { + t.Errorf("err message = %v, wantErrMsg %v", err.Error(), tt.wantErrMsg) + } if got := stdout.String(); got != tt.wantStdout { t.Errorf("stdout = %v, want %v", got, tt.wantStdout) } diff --git a/pkg/cmd/codespace/mock_api.go b/pkg/cmd/codespace/mock_api.go index 8d40934da..c27d5b48d 100644 --- a/pkg/cmd/codespace/mock_api.go +++ b/pkg/cmd/codespace/mock_api.go @@ -46,6 +46,9 @@ import ( // ListCodespacesFunc: func(ctx context.Context, limit int) ([]*api.Codespace, error) { // panic("mock out the ListCodespaces method") // }, +// ListDevContainersFunc: func(ctx context.Context, repoID int, branch string, limit int) ([]string, error) { +// panic("mock out the ListDevContainers method") +// }, // StartCodespaceFunc: func(ctx context.Context, name string) error { // panic("mock out the StartCodespace method") // }, @@ -89,6 +92,9 @@ type apiClientMock struct { // ListCodespacesFunc mocks the ListCodespaces method. ListCodespacesFunc func(ctx context.Context, limit int) ([]*api.Codespace, error) + // ListDevContainersFunc mocks the ListDevContainers method. + ListDevContainersFunc func(ctx context.Context, repoID int, branch string, limit int) ([]string, error) + // StartCodespaceFunc mocks the StartCodespace method. StartCodespaceFunc func(ctx context.Context, name string) error @@ -171,6 +177,17 @@ type apiClientMock struct { // Limit is the limit argument value. Limit int } + // ListDevContainers holds details about calls to the ListDevContainers method. + ListDevContainers []struct { + // Ctx is the ctx argument value. + Ctx context.Context + // RepoID is the repoID argument value. + RepoID int + // Branch is the branch argument value. + Branch string + // Limit is the limit argument value. + Limit int + } // StartCodespace holds details about calls to the StartCodespace method. StartCodespace []struct { // Ctx is the ctx argument value. @@ -196,6 +213,7 @@ type apiClientMock struct { lockGetRepository sync.RWMutex lockGetUser sync.RWMutex lockListCodespaces sync.RWMutex + lockListDevContainers sync.RWMutex lockStartCodespace sync.RWMutex lockStopCodespace sync.RWMutex } @@ -558,6 +576,49 @@ func (mock *apiClientMock) ListCodespacesCalls() []struct { return calls } +// ListDevContainers calls ListDevContainersFunc. +func (mock *apiClientMock) ListDevContainers(ctx context.Context, repoID int, branch string, limit int) ([]string, error) { + if mock.ListDevContainersFunc == nil { + panic("apiClientMock.ListDevContainersFunc: method is nil but apiClient.ListDevContainers was just called") + } + callInfo := struct { + Ctx context.Context + RepoID int + Branch string + Limit int + }{ + Ctx: ctx, + RepoID: repoID, + Branch: branch, + Limit: limit, + } + mock.lockListDevContainers.Lock() + mock.calls.ListDevContainers = append(mock.calls.ListDevContainers, callInfo) + mock.lockListDevContainers.Unlock() + return mock.ListDevContainersFunc(ctx, repoID, branch, limit) +} + +// ListDevContainersCalls gets all the calls that were made to ListDevContainers. +// Check the length with: +// len(mockedapiClient.ListDevContainersCalls()) +func (mock *apiClientMock) ListDevContainersCalls() []struct { + Ctx context.Context + RepoID int + Branch string + Limit int +} { + var calls []struct { + Ctx context.Context + RepoID int + Branch string + Limit int + } + mock.lockListDevContainers.RLock() + calls = mock.calls.ListDevContainers + mock.lockListDevContainers.RUnlock() + return calls +} + // StartCodespace calls StartCodespaceFunc. func (mock *apiClientMock) StartCodespace(ctx context.Context, name string) error { if mock.StartCodespaceFunc == nil { From 71d6e61a5b2b18151f4d4f6efb84722ccb211801 Mon Sep 17 00:00:00 2001 From: Jeff Hubbard Date: Fri, 21 Jan 2022 14:28:17 -0800 Subject: [PATCH 03/10] Rename test --- pkg/cmd/codespace/create_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cmd/codespace/create_test.go b/pkg/cmd/codespace/create_test.go index 0a7db6294..7afb5e833 100644 --- a/pkg/cmd/codespace/create_test.go +++ b/pkg/cmd/codespace/create_test.go @@ -72,7 +72,7 @@ func TestApp_Create(t *testing.T) { wantStdout: "monalisa-dotfiles-abcd1234\n", }, { - name: "create codespace with default branch with default devcontainer if no path provided", + name: "create codespace with default branch with default devcontainer if no path provided and no devcontainer files exist in the repo", fields: fields{ apiClient: &apiClientMock{ GetCodespaceRegionLocationFunc: func(ctx context.Context) (string, error) { From 8abf50be1de4e08b7496d4bf524e3e9c10d7e41d Mon Sep 17 00:00:00 2001 From: Jeff Hubbard Date: Fri, 18 Mar 2022 10:10:57 -0700 Subject: [PATCH 04/10] Change some copy per product's suggestions --- pkg/cmd/codespace/create.go | 8 ++++---- pkg/cmd/codespace/create_test.go | 2 +- testdevcontainer | 6 ++++++ 3 files changed, 11 insertions(+), 5 deletions(-) create mode 100644 testdevcontainer diff --git a/pkg/cmd/codespace/create.go b/pkg/cmd/codespace/create.go index 8e7a577e2..d95bfba1e 100644 --- a/pkg/cmd/codespace/create.go +++ b/pkg/cmd/codespace/create.go @@ -21,7 +21,7 @@ type createOptions struct { machine string showStatus bool permissionsOptOut bool - devContainerPath string + devContainerPath string idleTimeout time.Duration } @@ -123,8 +123,8 @@ func (a *App) Create(ctx context.Context, opts createOptions) error { devContainerPathQuestion := &survey.Question{ Name: "devContainerPath", Prompt: &survey.Select{ - Message: "Devcontainer.json file:", - Options: append([]string{"none"}, devContainerPaths...), + Message: "Devcontainer definition file:", + Options: append([]string{"default"}, devContainerPaths...), }, } @@ -133,7 +133,7 @@ func (a *App) Create(ctx context.Context, opts createOptions) error { } } - if devContainerPath == "none" { + if devContainerPath == "default" { // special arg allows users to opt out of devcontainer.json selection devContainerPath = "" } diff --git a/pkg/cmd/codespace/create_test.go b/pkg/cmd/codespace/create_test.go index 0c87ba9e0..9d97e3700 100644 --- a/pkg/cmd/codespace/create_test.go +++ b/pkg/cmd/codespace/create_test.go @@ -154,7 +154,7 @@ func TestApp_Create(t *testing.T) { wantErr: true, wantErrMsg: "error getting devcontainer.json paths: some error", }, - { + { name: "create codespace that requires accepting additional permissions", fields: fields{ apiClient: &apiClientMock{ diff --git a/testdevcontainer b/testdevcontainer new file mode 100644 index 000000000..1168d0af0 --- /dev/null +++ b/testdevcontainer @@ -0,0 +1,6 @@ +{ + "repository_id": 392831291, + "location": "WestUs2", + "devcontainer_path": ".devcontainer/foo ' \" bar/devcontainer.json", + "vscs_target": "development" +} From 125a7b00a2b90b10d1dd53da2cc63070941d4d27 Mon Sep 17 00:00:00 2001 From: Jeff Hubbard Date: Fri, 18 Mar 2022 14:44:15 -0700 Subject: [PATCH 05/10] Fix tests --- pkg/cmd/codespace/create_test.go | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/pkg/cmd/codespace/create_test.go b/pkg/cmd/codespace/create_test.go index 9d97e3700..eff850dda 100644 --- a/pkg/cmd/codespace/create_test.go +++ b/pkg/cmd/codespace/create_test.go @@ -9,6 +9,7 @@ import ( "github.com/cli/cli/v2/internal/codespaces/api" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/iostreams" + "github.com/stretchr/testify/assert" ) func TestApp_Create(t *testing.T) { @@ -19,8 +20,7 @@ func TestApp_Create(t *testing.T) { name string fields fields opts createOptions - wantErr bool - wantErrMsg string + wantErr error wantStdout string wantStderr string }{ @@ -151,8 +151,7 @@ func TestApp_Create(t *testing.T) { showStatus: false, idleTimeout: 30 * time.Minute, }, - wantErr: true, - wantErrMsg: "error getting devcontainer.json paths: some error", + wantErr: fmt.Errorf("error getting devcontainer.json paths: some error"), }, { name: "create codespace that requires accepting additional permissions", @@ -168,6 +167,9 @@ func TestApp_Create(t *testing.T) { DefaultBranch: "main", }, nil }, + ListDevContainersFunc: func(ctx context.Context, repoID int, branch string, limit int) ([]string, error) { + return []string{}, nil + }, GetCodespacesMachinesFunc: func(ctx context.Context, repoID int, branch, location string) ([]*api.Machine, error) { return []*api.Machine{ { @@ -209,19 +211,13 @@ Alternatively, you can run "create" with the "--default-permissions" option to c for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { io, _, stdout, stderr := iostreams.Test() - io.SetStdinTTY(true) - io.SetStdoutTTY(true) - a := &App{ io: io, apiClient: tt.fields.apiClient, } - if err := a.Create(context.Background(), tt.opts); err != tt.wantErr { - t.Errorf("App.Create() error = %v, wantErr %v", err, tt.wantErr) - } - if tt.wantErrMsg != "" && err.Error() != tt.wantErrMsg { - t.Errorf("err message = %v, wantErrMsg %v", err.Error(), tt.wantErrMsg) + if err := a.Create(context.Background(), tt.opts); err != nil && tt.wantErr != nil { + assert.EqualError(t, err, tt.wantErr.Error()) } if got := stdout.String(); got != tt.wantStdout { t.Errorf("stdout = %v, want %v", got, tt.wantStdout) From 9554e522af8e06faeac42c793a96bb96d7bc9eb2 Mon Sep 17 00:00:00 2001 From: Jeff Hubbard Date: Mon, 28 Mar 2022 14:24:57 -0700 Subject: [PATCH 06/10] Change the way we parse list-devcontainers response --- internal/codespaces/api/api.go | 10 +++++++-- pkg/cmd/codespace/common.go | 2 +- pkg/cmd/codespace/create.go | 36 ++++++++++++++++++++++---------- pkg/cmd/codespace/create_test.go | 10 ++++----- pkg/cmd/codespace/mock_api.go | 6 +++--- testdevcontainer | 6 ------ utils/utils.go | 9 ++++++++ 7 files changed, 51 insertions(+), 28 deletions(-) delete mode 100644 testdevcontainer diff --git a/internal/codespaces/api/api.go b/internal/codespaces/api/api.go index d0e61a0b1..0765525ec 100644 --- a/internal/codespaces/api/api.go +++ b/internal/codespaces/api/api.go @@ -762,9 +762,14 @@ func (a *API) DeleteCodespace(ctx context.Context, codespaceName string) error { return nil } +type DevContainerEntry struct { + Path string `json:"path"` + Name string `json:"name,omitempty"` +} + // ListDevContainers returns a list of valid devcontainer.json files for the repo. Pass a negative limit to request all pages from // the API until all devcontainer.json files have been fetched. -func (a *API) ListDevContainers(ctx context.Context, repoID int, branch string, limit int) (devcontainers []string, err error) { +func (a *API) ListDevContainers(ctx context.Context, repoID int, branch string, limit int) (devcontainers []DevContainerEntry, err error) { perPage := 100 if limit > 0 && limit < 100 { perPage = limit @@ -792,8 +797,9 @@ func (a *API) ListDevContainers(ctx context.Context, repoID int, branch string, } var response struct { - Devcontainers []string `json:"devcontainers"` + Devcontainers []DevContainerEntry `json:"devcontainers"` } + dec := json.NewDecoder(resp.Body) if err := dec.Decode(&response); err != nil { return nil, fmt.Errorf("error unmarshaling response: %w", err) diff --git a/pkg/cmd/codespace/common.go b/pkg/cmd/codespace/common.go index d9c941cf7..1bd04ca56 100644 --- a/pkg/cmd/codespace/common.go +++ b/pkg/cmd/codespace/common.go @@ -74,7 +74,7 @@ type apiClient interface { GetCodespaceRegionLocation(ctx context.Context) (string, error) GetCodespacesMachines(ctx context.Context, repoID int, branch, location string) ([]*api.Machine, error) GetCodespaceRepositoryContents(ctx context.Context, codespace *api.Codespace, path string) ([]byte, error) - ListDevContainers(ctx context.Context, repoID int, branch string, limit int) (devcontainers []string, err error) + ListDevContainers(ctx context.Context, repoID int, branch string, limit int) (devcontainers []api.DevContainerEntry, err error) GetCodespaceRepoSuggestions(ctx context.Context, partialSearch string, params api.RepoSearchParameters) ([]string, error) } diff --git a/pkg/cmd/codespace/create.go b/pkg/cmd/codespace/create.go index d95bfba1e..1a5560b8d 100644 --- a/pkg/cmd/codespace/create.go +++ b/pkg/cmd/codespace/create.go @@ -58,6 +58,8 @@ func (a *App) Create(ctx context.Context, opts createOptions) error { locationCh := getLocation(ctx, vscsLocation, a.apiClient) + DEFAULT_DEVCONTAINER_DEFINITIONS := []string{".devcontainer.json", ".devcontainer/devcontainer.json"} + userInputs := struct { Repository string Branch string @@ -113,23 +115,35 @@ func (a *App) Create(ctx context.Context, opts createOptions) error { // now that we have repo+branch, we can list available devcontainer.json files (if any) if len(opts.devContainerPath) < 1 { a.StartProgressIndicatorWithLabel("Fetching devcontainer.json files") - devContainerPaths, err := a.apiClient.ListDevContainers(ctx, repository.ID, branch, 100) + devcontainers, err := a.apiClient.ListDevContainers(ctx, repository.ID, branch, 100) if err != nil { return fmt.Errorf("error getting devcontainer.json paths: %w", err) } a.StopProgressIndicator() - if len(devContainerPaths) > 0 { - devContainerPathQuestion := &survey.Question{ - Name: "devContainerPath", - Prompt: &survey.Select{ - Message: "Devcontainer definition file:", - Options: append([]string{"default"}, devContainerPaths...), - }, - } + if len(devcontainers) > 0 { - if err := ask([]*survey.Question{devContainerPathQuestion}, &devContainerPath); err != nil { - return fmt.Errorf("failed to prompt: %w", err) + // if there is only one devcontainer.json file and it is one of the default paths we can auto-select it + if len(devcontainers) == 1 && utils.StringInSlice(devcontainers[0].Path, DEFAULT_DEVCONTAINER_DEFINITIONS) { + devContainerPath = devcontainers[0].Path + } else { + promptOptions := []string{"default"} + + for _, devcontainer := range devcontainers { + promptOptions = append(promptOptions, devcontainer.Path) + } + + devContainerPathQuestion := &survey.Question{ + Name: "devContainerPath", + Prompt: &survey.Select{ + Message: "Devcontainer definition file:", + Options: promptOptions, + }, + } + + if err := ask([]*survey.Question{devContainerPathQuestion}, &devContainerPath); err != nil { + return fmt.Errorf("failed to prompt: %w", err) + } } } diff --git a/pkg/cmd/codespace/create_test.go b/pkg/cmd/codespace/create_test.go index eff850dda..c1b2174f5 100644 --- a/pkg/cmd/codespace/create_test.go +++ b/pkg/cmd/codespace/create_test.go @@ -86,8 +86,8 @@ func TestApp_Create(t *testing.T) { DefaultBranch: "main", }, nil }, - ListDevContainersFunc: func(ctx context.Context, repoID int, branch string, limit int) ([]string, error) { - return []string{}, nil + ListDevContainersFunc: func(ctx context.Context, repoID int, branch string, limit int) ([]api.DevContainerEntry, error) { + return []api.DevContainerEntry{}, nil }, GetCodespacesMachinesFunc: func(ctx context.Context, repoID int, branch, location string) ([]*api.Machine, error) { return []*api.Machine{ @@ -139,7 +139,7 @@ func TestApp_Create(t *testing.T) { DefaultBranch: "main", }, nil }, - ListDevContainersFunc: func(ctx context.Context, repoID int, branch string, limit int) ([]string, error) { + ListDevContainersFunc: func(ctx context.Context, repoID int, branch string, limit int) ([]api.DevContainerEntry, error) { return nil, fmt.Errorf("some error") }, }, @@ -167,8 +167,8 @@ func TestApp_Create(t *testing.T) { DefaultBranch: "main", }, nil }, - ListDevContainersFunc: func(ctx context.Context, repoID int, branch string, limit int) ([]string, error) { - return []string{}, nil + ListDevContainersFunc: func(ctx context.Context, repoID int, branch string, limit int) ([]api.DevContainerEntry, error) { + return []api.DevContainerEntry{}, nil }, GetCodespacesMachinesFunc: func(ctx context.Context, repoID int, branch, location string) ([]*api.Machine, error) { return []*api.Machine{ diff --git a/pkg/cmd/codespace/mock_api.go b/pkg/cmd/codespace/mock_api.go index bccb90566..897b39f1d 100644 --- a/pkg/cmd/codespace/mock_api.go +++ b/pkg/cmd/codespace/mock_api.go @@ -52,7 +52,7 @@ import ( // ListCodespacesFunc: func(ctx context.Context, limit int) ([]*api.Codespace, error) { // panic("mock out the ListCodespaces method") // }, -// ListDevContainersFunc: func(ctx context.Context, repoID int, branch string, limit int) ([]string, error) { +// ListDevContainersFunc: func(ctx context.Context, repoID int, branch string, limit int) ([]api.DevContainerEntry, error) { // panic("mock out the ListDevContainers method") // }, // StartCodespaceFunc: func(ctx context.Context, name string) error { @@ -105,7 +105,7 @@ type apiClientMock struct { ListCodespacesFunc func(ctx context.Context, limit int) ([]*api.Codespace, error) // ListDevContainersFunc mocks the ListDevContainers method. - ListDevContainersFunc func(ctx context.Context, repoID int, branch string, limit int) ([]string, error) + ListDevContainersFunc func(ctx context.Context, repoID int, branch string, limit int) ([]api.DevContainerEntry, error) // StartCodespaceFunc mocks the StartCodespace method. StartCodespaceFunc func(ctx context.Context, name string) error @@ -687,7 +687,7 @@ func (mock *apiClientMock) ListCodespacesCalls() []struct { } // ListDevContainers calls ListDevContainersFunc. -func (mock *apiClientMock) ListDevContainers(ctx context.Context, repoID int, branch string, limit int) ([]string, error) { +func (mock *apiClientMock) ListDevContainers(ctx context.Context, repoID int, branch string, limit int) ([]api.DevContainerEntry, error) { if mock.ListDevContainersFunc == nil { panic("apiClientMock.ListDevContainersFunc: method is nil but apiClient.ListDevContainers was just called") } diff --git a/testdevcontainer b/testdevcontainer deleted file mode 100644 index 1168d0af0..000000000 --- a/testdevcontainer +++ /dev/null @@ -1,6 +0,0 @@ -{ - "repository_id": 392831291, - "location": "WestUs2", - "devcontainer_path": ".devcontainer/foo ' \" bar/devcontainer.json", - "vscs_target": "development" -} diff --git a/utils/utils.go b/utils/utils.go index cd423bf65..18ae33143 100644 --- a/utils/utils.go +++ b/utils/utils.go @@ -83,3 +83,12 @@ func DisplayURL(urlStr string) string { func ValidURL(urlStr string) bool { return len(urlStr) < 8192 } + +func StringInSlice(a string, slice []string) bool { + for _, b := range slice { + if b == a { + return true + } + } + return false +} From b8ecbe748fac1d3fc475a8d26eb80d60145c5433 Mon Sep 17 00:00:00 2001 From: Jeff Hubbard Date: Tue, 29 Mar 2022 13:50:31 -0700 Subject: [PATCH 07/10] go fmt --- utils/utils.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/utils/utils.go b/utils/utils.go index b73501f42..503fdf0af 100644 --- a/utils/utils.go +++ b/utils/utils.go @@ -93,7 +93,7 @@ func StringInSlice(a string, slice []string) bool { } return false } - + func IsDebugEnabled() (bool, string) { debugValue, isDebugSet := os.LookupEnv("GH_DEBUG") legacyDebugValue := os.Getenv("DEBUG") From a84e43cadf69b6631339e5c08ca059912a17ef29 Mon Sep 17 00:00:00 2001 From: Jeff Hubbard Date: Mon, 4 Apr 2022 16:32:38 -0700 Subject: [PATCH 08/10] Final copy and behavior update for prompt --- pkg/cmd/codespace/create.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/pkg/cmd/codespace/create.go b/pkg/cmd/codespace/create.go index 1a5560b8d..5fe111d0c 100644 --- a/pkg/cmd/codespace/create.go +++ b/pkg/cmd/codespace/create.go @@ -127,7 +127,11 @@ func (a *App) Create(ctx context.Context, opts createOptions) error { if len(devcontainers) == 1 && utils.StringInSlice(devcontainers[0].Path, DEFAULT_DEVCONTAINER_DEFINITIONS) { devContainerPath = devcontainers[0].Path } else { - promptOptions := []string{"default"} + promptOptions := []string{} + + if !utils.StringInSlice(devcontainers[0].Path, DEFAULT_DEVCONTAINER_DEFINITIONS) { + promptOptions = []string{"Default Codespaces configuration"} + } for _, devcontainer := range devcontainers { promptOptions = append(promptOptions, devcontainer.Path) @@ -147,7 +151,7 @@ func (a *App) Create(ctx context.Context, opts createOptions) error { } } - if devContainerPath == "default" { + if devContainerPath == "Default Codespaces configuration" { // special arg allows users to opt out of devcontainer.json selection devContainerPath = "" } From 9fb6cf54237a045e057842b3e4b317c9ab5fd783 Mon Sep 17 00:00:00 2001 From: Jeff Hubbard Date: Tue, 12 Apr 2022 10:26:08 -0700 Subject: [PATCH 09/10] Fix tests --- pkg/cmd/codespace/create.go | 2 +- pkg/cmd/codespace/create_test.go | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/pkg/cmd/codespace/create.go b/pkg/cmd/codespace/create.go index 521b37437..5bf2c3806 100644 --- a/pkg/cmd/codespace/create.go +++ b/pkg/cmd/codespace/create.go @@ -58,7 +58,7 @@ func (a *App) Create(ctx context.Context, opts createOptions) error { vscsTargetUrl := os.Getenv("VSCS_TARGET_URL") DEFAULT_DEVCONTAINER_DEFINITIONS := []string{".devcontainer.json", ".devcontainer/devcontainer.json"} - + userInputs := struct { Repository string Branch string diff --git a/pkg/cmd/codespace/create_test.go b/pkg/cmd/codespace/create_test.go index 73475097a..3f78a94b2 100644 --- a/pkg/cmd/codespace/create_test.go +++ b/pkg/cmd/codespace/create_test.go @@ -36,6 +36,9 @@ func TestApp_Create(t *testing.T) { DefaultBranch: "main", }, nil }, + ListDevContainersFunc: func(ctx context.Context, repoID int, branch string, limit int) ([]api.DevContainerEntry, error) { + return []api.DevContainerEntry{{Path: ".devcontainer/devcontainer.json"}}, nil + }, GetCodespacesMachinesFunc: func(ctx context.Context, repoID int, branch, location string) ([]*api.Machine, error) { return []*api.Machine{ { @@ -265,6 +268,9 @@ func TestApp_Create(t *testing.T) { DefaultBranch: "main", }, nil }, + ListDevContainersFunc: func(ctx context.Context, repoID int, branch string, limit int) ([]api.DevContainerEntry, error) { + return []api.DevContainerEntry{{Path: ".devcontainer/devcontainer.json"}}, nil + }, GetCodespacesMachinesFunc: func(ctx context.Context, repoID int, branch, location string) ([]*api.Machine, error) { return []*api.Machine{ { From 2b57084bd0351847ae9fd1cc6b5fd54cf071e189 Mon Sep 17 00:00:00 2001 From: Jeff Hubbard Date: Wed, 20 Apr 2022 11:52:32 -0700 Subject: [PATCH 10/10] PR feedback --- internal/codespaces/api/api.go | 7 +++++-- pkg/cmd/codespace/create.go | 19 ++++++++++++------- 2 files changed, 17 insertions(+), 9 deletions(-) diff --git a/internal/codespaces/api/api.go b/internal/codespaces/api/api.go index b52f05e22..d43208e82 100644 --- a/internal/codespaces/api/api.go +++ b/internal/codespaces/api/api.go @@ -742,10 +742,13 @@ func (a *API) ListDevContainers(ctx context.Context, repoID int, branch string, perPage = limit } - listURL := fmt.Sprintf("%s/repositories/%d/codespaces/devcontainers?per_page=%d", a.githubAPI, repoID, perPage) + v := url.Values{} + v.Set("per_page", strconv.Itoa(perPage)) if branch != "" { - listURL += "&ref=" + branch + v.Set("ref", branch) } + listURL := fmt.Sprintf("%s/repositories/%d/codespaces/devcontainers?%s", a.githubAPI, repoID, v.Encode()) + for { req, err := http.NewRequest(http.MethodGet, listURL, nil) if err != nil { diff --git a/pkg/cmd/codespace/create.go b/pkg/cmd/codespace/create.go index 5bf2c3806..91d293658 100644 --- a/pkg/cmd/codespace/create.go +++ b/pkg/cmd/codespace/create.go @@ -15,6 +15,14 @@ import ( "github.com/spf13/cobra" ) +const ( + DEVCONTAINER_PROMPT_DEFAULT = "Default Codespaces configuration" +) + +var ( + DEFAULT_DEVCONTAINER_DEFINITIONS = []string{".devcontainer.json", ".devcontainer/devcontainer.json"} +) + type createOptions struct { repo string branch string @@ -57,8 +65,6 @@ func (a *App) Create(ctx context.Context, opts createOptions) error { vscsTarget := os.Getenv("VSCS_TARGET") vscsTargetUrl := os.Getenv("VSCS_TARGET_URL") - DEFAULT_DEVCONTAINER_DEFINITIONS := []string{".devcontainer.json", ".devcontainer/devcontainer.json"} - userInputs := struct { Repository string Branch string @@ -118,13 +124,13 @@ func (a *App) Create(ctx context.Context, opts createOptions) error { devContainerPath := opts.devContainerPath // now that we have repo+branch, we can list available devcontainer.json files (if any) - if len(opts.devContainerPath) < 1 { + if opts.devContainerPath == "" { a.StartProgressIndicatorWithLabel("Fetching devcontainer.json files") devcontainers, err := a.apiClient.ListDevContainers(ctx, repository.ID, branch, 100) + a.StopProgressIndicator() if err != nil { return fmt.Errorf("error getting devcontainer.json paths: %w", err) } - a.StopProgressIndicator() if len(devcontainers) > 0 { @@ -135,7 +141,7 @@ func (a *App) Create(ctx context.Context, opts createOptions) error { promptOptions := []string{} if !utils.StringInSlice(devcontainers[0].Path, DEFAULT_DEVCONTAINER_DEFINITIONS) { - promptOptions = []string{"Default Codespaces configuration"} + promptOptions = []string{DEVCONTAINER_PROMPT_DEFAULT} } for _, devcontainer := range devcontainers { @@ -156,7 +162,7 @@ func (a *App) Create(ctx context.Context, opts createOptions) error { } } - if devContainerPath == "Default Codespaces configuration" { + if devContainerPath == DEVCONTAINER_PROMPT_DEFAULT { // special arg allows users to opt out of devcontainer.json selection devContainerPath = "" } @@ -184,7 +190,6 @@ func (a *App) Create(ctx context.Context, opts createOptions) error { a.StartProgressIndicatorWithLabel("Creating codespace") codespace, err := a.apiClient.CreateCodespace(ctx, createParams) - a.StopProgressIndicator() if err != nil {