From edf83af07a5d33a8e0aa8488501606a87241a92f Mon Sep 17 00:00:00 2001 From: Jake Shorty Date: Wed, 15 Jun 2022 21:17:04 +0000 Subject: [PATCH 01/13] Add Codespaces pre-flight request to API client --- internal/codespaces/api/api.go | 44 ++++++++++++++++++++++++++++++++++ pkg/cmd/codespace/common.go | 1 + 2 files changed, 45 insertions(+) diff --git a/internal/codespaces/api/api.go b/internal/codespaces/api/api.go index c11e635f6..8cf762b5b 100644 --- a/internal/codespaces/api/api.go +++ b/internal/codespaces/api/api.go @@ -556,6 +556,50 @@ func (a *API) GetCodespaceRepoSuggestions(ctx context.Context, partialSearch str return repoNames, nil } +// GetCodespacePreFlight returns the billable owner and expected default values for +// codespaces created by the user for a given repository. +func (a *API) GetCodespacePreFlight(ctx context.Context, nwo string) (*User, error) { + req, err := http.NewRequest(http.MethodGet, a.githubAPI+"/repos/"+nwo+"/codespaces/new", nil) + if err != nil { + return nil, fmt.Errorf("error creating request: %w", err) + } + + a.setHeaders(req) + resp, err := a.do(ctx, req, "/repos/*/codespaces/new") + if err != nil { + return nil, fmt.Errorf("error making request: %w", err) + } + defer resp.Body.Close() + + if resp.StatusCode == http.StatusNotFound { + return nil, nil + } else if resp.StatusCode != http.StatusForbidden { + return nil, fmt.Errorf("codespaces cannot be created from that repository") + } else if resp.StatusCode != http.StatusOK { + return nil, api.HandleHTTPError(resp) + } + + b, err := io.ReadAll(resp.Body) + if err != nil { + return nil, fmt.Errorf("error reading response body: %w", err) + } + + var response struct { + BillableOwner User `json:"billableOwner"` + Defaults struct { + DevcontainerPath string `json:"devcontainerPath"` + Location string `json:"location"` + } + } + if err := json.Unmarshal(b, &response); err != nil { + return nil, fmt.Errorf("error unmarshaling response: %w", err) + } + + // While this response contains further helpful information ahead of codespace creation, + // we're only referencing the billable owner today. + return &response.BillableOwner, nil +} + // CreateCodespaceParams are the required parameters for provisioning a Codespace. type CreateCodespaceParams struct { RepositoryID int diff --git a/pkg/cmd/codespace/common.go b/pkg/cmd/codespace/common.go index 6720aa0bb..ef918cb11 100644 --- a/pkg/cmd/codespace/common.go +++ b/pkg/cmd/codespace/common.go @@ -120,6 +120,7 @@ type apiClient interface { GetCodespaceRepositoryContents(ctx context.Context, codespace *api.Codespace, path string) ([]byte, 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) + GetCodespacePreFlight(ctx context.Context, nwo string) (*api.User, error) } var errNoCodespaces = errors.New("you have no codespaces") From cad9a050becf5381813f7132240f9537e5bb6a7f Mon Sep 17 00:00:00 2001 From: Jake Shorty Date: Thu, 16 Jun 2022 15:42:29 +0000 Subject: [PATCH 02/13] Get basics working with codespaces pre-flight during creation --- internal/codespaces/api/api.go | 1 + pkg/cmd/codespace/create.go | 30 ++++++++++++++++++++++++------ 2 files changed, 25 insertions(+), 6 deletions(-) diff --git a/internal/codespaces/api/api.go b/internal/codespaces/api/api.go index 8cf762b5b..b13d5847e 100644 --- a/internal/codespaces/api/api.go +++ b/internal/codespaces/api/api.go @@ -94,6 +94,7 @@ func New(serverURL, apiURL, vscsURL string, httpClient httpClient) *API { // User represents a GitHub user. type User struct { Login string `json:"login"` + Type string `json:"type"` } // GetUser returns the user associated with the given token. diff --git a/pkg/cmd/codespace/create.go b/pkg/cmd/codespace/create.go index 49d950e9f..1d4752d79 100644 --- a/pkg/cmd/codespace/create.go +++ b/pkg/cmd/codespace/create.go @@ -112,11 +112,7 @@ func (a *App) Create(ctx context.Context, opts createOptions) error { } if userInputs.Repository == "" { - branchPrompt := "Branch (leave blank for default branch):" - if userInputs.Branch != "" { - branchPrompt = "Branch:" - } - questions := []*survey.Question{ + repoQuestions := []*survey.Question{ { Name: "repository", Prompt: &survey.Input{ @@ -128,6 +124,27 @@ func (a *App) Create(ctx context.Context, opts createOptions) error { }, Validate: survey.Required, }, + } + if err := ask(repoQuestions, &userInputs); err != nil { + return fmt.Errorf("failed to prompt: %w", err) + } + + a.StartProgressIndicatorWithLabel("Validating repository for codespaces") + billableOwner, err := a.apiClient.GetCodespacePreFlight(ctx, userInputs.Repository) + a.StopProgressIndicator() + + if billableOwner.Type == "Organization" { + cs := a.io.ColorScheme() + fmt.Fprintln(a.io.Out, cs.Blue("✓ Usage covered by "+billableOwner.Login)) + } else if err != nil { + return fmt.Errorf("error checking codespace ownership: %w", err) + } + + branchPrompt := "Branch (leave blank for default branch):" + if userInputs.Branch != "" { + branchPrompt = "Branch:" + } + branchQuestions := []*survey.Question{ { Name: "branch", Prompt: &survey.Input{ @@ -136,7 +153,8 @@ func (a *App) Create(ctx context.Context, opts createOptions) error { }, }, } - if err := ask(questions, &userInputs); err != nil { + + if err := ask(branchQuestions, &userInputs); err != nil { return fmt.Errorf("failed to prompt: %w", err) } } From 7e4ec074f563921d3050c73f8babacf353717f2f Mon Sep 17 00:00:00 2001 From: Jake Shorty Date: Fri, 17 Jun 2022 18:42:31 +0000 Subject: [PATCH 03/13] Use expanded copy for org billable owner --- pkg/cmd/codespace/create.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cmd/codespace/create.go b/pkg/cmd/codespace/create.go index 1d4752d79..abb9112ba 100644 --- a/pkg/cmd/codespace/create.go +++ b/pkg/cmd/codespace/create.go @@ -135,7 +135,7 @@ func (a *App) Create(ctx context.Context, opts createOptions) error { if billableOwner.Type == "Organization" { cs := a.io.ColorScheme() - fmt.Fprintln(a.io.Out, cs.Blue("✓ Usage covered by "+billableOwner.Login)) + fmt.Fprintln(a.io.Out, cs.Blue("✓ Codespaces usage for this repository is paid for by "+billableOwner.Login)) } else if err != nil { return fmt.Errorf("error checking codespace ownership: %w", err) } From 08446c8fcd0f1691b7db50f5ed823ea87c7c6b60 Mon Sep 17 00:00:00 2001 From: Jake Shorty Date: Fri, 17 Jun 2022 19:52:37 +0000 Subject: [PATCH 04/13] Add new API client function to tests --- pkg/cmd/codespace/create_test.go | 36 ++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/pkg/cmd/codespace/create_test.go b/pkg/cmd/codespace/create_test.go index 910c73495..631cda849 100644 --- a/pkg/cmd/codespace/create_test.go +++ b/pkg/cmd/codespace/create_test.go @@ -29,6 +29,12 @@ func TestApp_Create(t *testing.T) { name: "create codespace with default branch and 30m idle timeout", fields: fields{ apiClient: &apiClientMock{ + GetCodespacePreFlightFunc: func(ctx context.Context, nwo string) (*api.User, error) { + return &api.User{ + Login: "monalisa", + Type: "User", + }, nil + }, GetRepositoryFunc: func(ctx context.Context, nwo string) (*api.Repository, error) { return &api.Repository{ ID: 1234, @@ -83,6 +89,12 @@ func TestApp_Create(t *testing.T) { GetCodespaceRegionLocationFunc: func(ctx context.Context) (string, error) { return "EUROPE", nil }, + GetCodespacePreFlightFunc: func(ctx context.Context, nwo string) (*api.User, error) { + return &api.User{ + Login: "monalisa", + Type: "User", + }, nil + }, GetRepositoryFunc: func(ctx context.Context, nwo string) (*api.Repository, error) { return &api.Repository{ ID: 1234, @@ -134,6 +146,12 @@ func TestApp_Create(t *testing.T) { GetCodespaceRegionLocationFunc: func(ctx context.Context) (string, error) { return "EUROPE", nil }, + GetCodespacePreFlightFunc: func(ctx context.Context, nwo string) (*api.User, error) { + return &api.User{ + Login: "monalisa", + Type: "User", + }, nil + }, GetRepositoryFunc: func(ctx context.Context, nwo string) (*api.Repository, error) { return &api.Repository{ ID: 1234, @@ -190,6 +208,12 @@ func TestApp_Create(t *testing.T) { GetCodespaceRegionLocationFunc: func(ctx context.Context) (string, error) { return "EUROPE", nil }, + GetCodespacePreFlightFunc: func(ctx context.Context, nwo string) (*api.User, error) { + return &api.User{ + Login: "monalisa", + Type: "User", + }, nil + }, GetRepositoryFunc: func(ctx context.Context, nwo string) (*api.Repository, error) { return &api.Repository{ ID: 1234, @@ -218,6 +242,12 @@ func TestApp_Create(t *testing.T) { GetCodespaceRegionLocationFunc: func(ctx context.Context) (string, error) { return "EUROPE", nil }, + GetCodespacePreFlightFunc: func(ctx context.Context, nwo string) (*api.User, error) { + return &api.User{ + Login: "monalisa", + Type: "User", + }, nil + }, GetRepositoryFunc: func(ctx context.Context, nwo string) (*api.Repository, error) { return &api.Repository{ ID: 1234, @@ -268,6 +298,12 @@ func TestApp_Create(t *testing.T) { name: "create codespace that requires accepting additional permissions", fields: fields{ apiClient: &apiClientMock{ + GetCodespacePreFlightFunc: func(ctx context.Context, nwo string) (*api.User, error) { + return &api.User{ + Login: "monalisa", + Type: "User", + }, nil + }, GetRepositoryFunc: func(ctx context.Context, nwo string) (*api.Repository, error) { return &api.Repository{ ID: 1234, From 1c8b26c5d8504ee3bf9ed277a1460040baa529c2 Mon Sep 17 00:00:00 2001 From: Jake Shorty Date: Fri, 17 Jun 2022 19:59:29 +0000 Subject: [PATCH 05/13] Remove unused test function --- pkg/cmd/codespace/create_test.go | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/pkg/cmd/codespace/create_test.go b/pkg/cmd/codespace/create_test.go index 631cda849..238709faf 100644 --- a/pkg/cmd/codespace/create_test.go +++ b/pkg/cmd/codespace/create_test.go @@ -86,9 +86,6 @@ func TestApp_Create(t *testing.T) { name: "create codespace with default branch shows idle timeout notice if present", fields: fields{ apiClient: &apiClientMock{ - GetCodespaceRegionLocationFunc: func(ctx context.Context) (string, error) { - return "EUROPE", nil - }, GetCodespacePreFlightFunc: func(ctx context.Context, nwo string) (*api.User, error) { return &api.User{ Login: "monalisa", @@ -143,9 +140,6 @@ func TestApp_Create(t *testing.T) { 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) { - return "EUROPE", nil - }, GetCodespacePreFlightFunc: func(ctx context.Context, nwo string) (*api.User, error) { return &api.User{ Login: "monalisa", @@ -205,9 +199,6 @@ func TestApp_Create(t *testing.T) { name: "returns error when getting devcontainer paths fails", fields: fields{ apiClient: &apiClientMock{ - GetCodespaceRegionLocationFunc: func(ctx context.Context) (string, error) { - return "EUROPE", nil - }, GetCodespacePreFlightFunc: func(ctx context.Context, nwo string) (*api.User, error) { return &api.User{ Login: "monalisa", @@ -239,9 +230,6 @@ func TestApp_Create(t *testing.T) { name: "create codespace with default branch does not show idle timeout notice if not conntected to terminal", fields: fields{ apiClient: &apiClientMock{ - GetCodespaceRegionLocationFunc: func(ctx context.Context) (string, error) { - return "EUROPE", nil - }, GetCodespacePreFlightFunc: func(ctx context.Context, nwo string) (*api.User, error) { return &api.User{ Login: "monalisa", From 09f926c475a2046ab02e3d5cca34fe5bb8d62f14 Mon Sep 17 00:00:00 2001 From: Jake Shorty Date: Fri, 17 Jun 2022 20:15:53 +0000 Subject: [PATCH 06/13] Generate test mocks for new API func --- pkg/cmd/codespace/mock_api.go | 48 ++++++++++++++++++++--------------- 1 file changed, 27 insertions(+), 21 deletions(-) diff --git a/pkg/cmd/codespace/mock_api.go b/pkg/cmd/codespace/mock_api.go index 897b39f1d..d12644de7 100644 --- a/pkg/cmd/codespace/mock_api.go +++ b/pkg/cmd/codespace/mock_api.go @@ -31,8 +31,8 @@ import ( // GetCodespaceFunc: func(ctx context.Context, name string, includeConnection bool) (*api.Codespace, error) { // panic("mock out the GetCodespace method") // }, -// GetCodespaceRegionLocationFunc: func(ctx context.Context) (string, error) { -// panic("mock out the GetCodespaceRegionLocation method") +// GetCodespacePreFlightFunc: func(ctx context.Context, nwo string) (*api.User, error) { +// panic("mock out the GetCodespacePreFlight method") // }, // GetCodespaceRepoSuggestionsFunc: func(ctx context.Context, partialSearch string, params api.RepoSearchParameters) ([]string, error) { // panic("mock out the GetCodespaceRepoSuggestions method") @@ -83,8 +83,8 @@ type apiClientMock struct { // GetCodespaceFunc mocks the GetCodespace method. GetCodespaceFunc func(ctx context.Context, name string, includeConnection bool) (*api.Codespace, error) - // GetCodespaceRegionLocationFunc mocks the GetCodespaceRegionLocation method. - GetCodespaceRegionLocationFunc func(ctx context.Context) (string, error) + // GetCodespacePreFlightFunc mocks the GetCodespacePreFlight method. + GetCodespacePreFlightFunc func(ctx context.Context, nwo string) (*api.User, error) // GetCodespaceRepoSuggestionsFunc mocks the GetCodespaceRepoSuggestions method. GetCodespaceRepoSuggestionsFunc func(ctx context.Context, partialSearch string, params api.RepoSearchParameters) ([]string, error) @@ -154,10 +154,12 @@ type apiClientMock struct { // IncludeConnection is the includeConnection argument value. IncludeConnection bool } - // GetCodespaceRegionLocation holds details about calls to the GetCodespaceRegionLocation method. - GetCodespaceRegionLocation []struct { + // GetCodespacePreFlight holds details about calls to the GetCodespacePreFlight method. + GetCodespacePreFlight []struct { // Ctx is the ctx argument value. Ctx context.Context + // Nwo is the nwo argument value. + Nwo string } // GetCodespaceRepoSuggestions holds details about calls to the GetCodespaceRepoSuggestions method. GetCodespaceRepoSuggestions []struct { @@ -238,7 +240,7 @@ type apiClientMock struct { lockDeleteCodespace sync.RWMutex lockEditCodespace sync.RWMutex lockGetCodespace sync.RWMutex - lockGetCodespaceRegionLocation sync.RWMutex + lockGetCodespacePreFlight sync.RWMutex lockGetCodespaceRepoSuggestions sync.RWMutex lockGetCodespaceRepositoryContents sync.RWMutex lockGetCodespacesMachines sync.RWMutex @@ -433,34 +435,38 @@ func (mock *apiClientMock) GetCodespaceCalls() []struct { return calls } -// GetCodespaceRegionLocation calls GetCodespaceRegionLocationFunc. -func (mock *apiClientMock) GetCodespaceRegionLocation(ctx context.Context) (string, error) { - if mock.GetCodespaceRegionLocationFunc == nil { - panic("apiClientMock.GetCodespaceRegionLocationFunc: method is nil but apiClient.GetCodespaceRegionLocation was just called") +// GetCodespacePreFlight calls GetCodespacePreFlightFunc. +func (mock *apiClientMock) GetCodespacePreFlight(ctx context.Context, nwo string) (*api.User, error) { + if mock.GetCodespacePreFlightFunc == nil { + panic("apiClientMock.GetCodespacePreFlightFunc: method is nil but apiClient.GetCodespacePreFlight was just called") } callInfo := struct { Ctx context.Context + Nwo string }{ Ctx: ctx, + Nwo: nwo, } - mock.lockGetCodespaceRegionLocation.Lock() - mock.calls.GetCodespaceRegionLocation = append(mock.calls.GetCodespaceRegionLocation, callInfo) - mock.lockGetCodespaceRegionLocation.Unlock() - return mock.GetCodespaceRegionLocationFunc(ctx) + mock.lockGetCodespacePreFlight.Lock() + mock.calls.GetCodespacePreFlight = append(mock.calls.GetCodespacePreFlight, callInfo) + mock.lockGetCodespacePreFlight.Unlock() + return mock.GetCodespacePreFlightFunc(ctx, nwo) } -// GetCodespaceRegionLocationCalls gets all the calls that were made to GetCodespaceRegionLocation. +// GetCodespacePreFlightCalls gets all the calls that were made to GetCodespacePreFlight. // Check the length with: -// len(mockedapiClient.GetCodespaceRegionLocationCalls()) -func (mock *apiClientMock) GetCodespaceRegionLocationCalls() []struct { +// len(mockedapiClient.GetCodespacePreFlightCalls()) +func (mock *apiClientMock) GetCodespacePreFlightCalls() []struct { Ctx context.Context + Nwo string } { var calls []struct { Ctx context.Context + Nwo string } - mock.lockGetCodespaceRegionLocation.RLock() - calls = mock.calls.GetCodespaceRegionLocation - mock.lockGetCodespaceRegionLocation.RUnlock() + mock.lockGetCodespacePreFlight.RLock() + calls = mock.calls.GetCodespacePreFlight + mock.lockGetCodespacePreFlight.RUnlock() return calls } From b3d2cc6e376fa80fbd9c7bfcfc6cc72ec1c07f48 Mon Sep 17 00:00:00 2001 From: Jake Shorty Date: Fri, 17 Jun 2022 21:09:38 +0000 Subject: [PATCH 07/13] Handle 404s smoothly during create on codespaces pre-flight --- pkg/cmd/codespace/create.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cmd/codespace/create.go b/pkg/cmd/codespace/create.go index abb9112ba..9d00f205d 100644 --- a/pkg/cmd/codespace/create.go +++ b/pkg/cmd/codespace/create.go @@ -133,7 +133,7 @@ func (a *App) Create(ctx context.Context, opts createOptions) error { billableOwner, err := a.apiClient.GetCodespacePreFlight(ctx, userInputs.Repository) a.StopProgressIndicator() - if billableOwner.Type == "Organization" { + if billableOwner != nil && billableOwner.Type == "Organization" { cs := a.io.ColorScheme() fmt.Fprintln(a.io.Out, cs.Blue("✓ Codespaces usage for this repository is paid for by "+billableOwner.Login)) } else if err != nil { From e8bde879b2a1202992c9fcda9b69a48c9ed7b6ba Mon Sep 17 00:00:00 2001 From: Jake Shorty Date: Fri, 17 Jun 2022 22:44:43 +0000 Subject: [PATCH 08/13] Always show org billable owner in stdout --- pkg/cmd/codespace/create.go | 45 ++++++++++++++++++++----------------- 1 file changed, 24 insertions(+), 21 deletions(-) diff --git a/pkg/cmd/codespace/create.go b/pkg/cmd/codespace/create.go index 9d00f205d..6da70bfe9 100644 --- a/pkg/cmd/codespace/create.go +++ b/pkg/cmd/codespace/create.go @@ -111,7 +111,8 @@ func (a *App) Create(ctx context.Context, opts createOptions) error { Location: opts.location, } - if userInputs.Repository == "" { + promptForRepoAndBranch := userInputs.Repository == "" + if promptForRepoAndBranch { repoQuestions := []*survey.Question{ { Name: "repository", @@ -128,18 +129,31 @@ func (a *App) Create(ctx context.Context, opts createOptions) error { if err := ask(repoQuestions, &userInputs); err != nil { return fmt.Errorf("failed to prompt: %w", err) } + } - a.StartProgressIndicatorWithLabel("Validating repository for codespaces") - billableOwner, err := a.apiClient.GetCodespacePreFlight(ctx, userInputs.Repository) - a.StopProgressIndicator() + if userInputs.Location == "" && vscsLocation != "" { + userInputs.Location = vscsLocation + } - if billableOwner != nil && billableOwner.Type == "Organization" { - cs := a.io.ColorScheme() - fmt.Fprintln(a.io.Out, cs.Blue("✓ Codespaces usage for this repository is paid for by "+billableOwner.Login)) - } else if err != nil { - return fmt.Errorf("error checking codespace ownership: %w", err) - } + a.StartProgressIndicatorWithLabel("Fetching repository") + repository, err := a.apiClient.GetRepository(ctx, userInputs.Repository) + a.StopProgressIndicator() + if err != nil { + return fmt.Errorf("error getting repository: %w", err) + } + a.StartProgressIndicatorWithLabel("Validating repository for codespaces") + billableOwner, err := a.apiClient.GetCodespacePreFlight(ctx, userInputs.Repository) + a.StopProgressIndicator() + + if billableOwner != nil && billableOwner.Type == "Organization" { + cs := a.io.ColorScheme() + fmt.Fprintln(a.io.Out, cs.Blue("✓ Codespaces usage for this repository is paid for by "+billableOwner.Login)) + } else if err != nil { + return fmt.Errorf("error checking codespace ownership: %w", err) + } + + if promptForRepoAndBranch { branchPrompt := "Branch (leave blank for default branch):" if userInputs.Branch != "" { branchPrompt = "Branch:" @@ -159,17 +173,6 @@ func (a *App) Create(ctx context.Context, opts createOptions) error { } } - if userInputs.Location == "" && vscsLocation != "" { - userInputs.Location = vscsLocation - } - - a.StartProgressIndicatorWithLabel("Fetching repository") - repository, err := a.apiClient.GetRepository(ctx, userInputs.Repository) - a.StopProgressIndicator() - if err != nil { - return fmt.Errorf("error getting repository: %w", err) - } - branch := userInputs.Branch if branch == "" { branch = repository.DefaultBranch From 454b3489aa64a61b258da79a23bfebc7567ff884 Mon Sep 17 00:00:00 2001 From: Jake Shorty Date: Fri, 17 Jun 2022 22:45:04 +0000 Subject: [PATCH 09/13] Add test for billable owner in stdout --- pkg/cmd/codespace/create_test.go | 129 ++++++++++++++++++++++++------- 1 file changed, 99 insertions(+), 30 deletions(-) diff --git a/pkg/cmd/codespace/create_test.go b/pkg/cmd/codespace/create_test.go index 238709faf..42b0fd731 100644 --- a/pkg/cmd/codespace/create_test.go +++ b/pkg/cmd/codespace/create_test.go @@ -29,12 +29,6 @@ func TestApp_Create(t *testing.T) { name: "create codespace with default branch and 30m idle timeout", fields: fields{ apiClient: &apiClientMock{ - GetCodespacePreFlightFunc: func(ctx context.Context, nwo string) (*api.User, error) { - return &api.User{ - Login: "monalisa", - Type: "User", - }, nil - }, GetRepositoryFunc: func(ctx context.Context, nwo string) (*api.Repository, error) { return &api.Repository{ ID: 1234, @@ -42,6 +36,12 @@ func TestApp_Create(t *testing.T) { DefaultBranch: "main", }, nil }, + GetCodespacePreFlightFunc: func(ctx context.Context, nwo string) (*api.User, error) { + return &api.User{ + Login: "monalisa", + Type: "User", + }, nil + }, ListDevContainersFunc: func(ctx context.Context, repoID int, branch string, limit int) ([]api.DevContainerEntry, error) { return []api.DevContainerEntry{{Path: ".devcontainer/devcontainer.json"}}, nil }, @@ -86,12 +86,6 @@ func TestApp_Create(t *testing.T) { name: "create codespace with default branch shows idle timeout notice if present", fields: fields{ apiClient: &apiClientMock{ - GetCodespacePreFlightFunc: func(ctx context.Context, nwo string) (*api.User, error) { - return &api.User{ - Login: "monalisa", - Type: "User", - }, nil - }, GetRepositoryFunc: func(ctx context.Context, nwo string) (*api.Repository, error) { return &api.Repository{ ID: 1234, @@ -99,6 +93,12 @@ func TestApp_Create(t *testing.T) { DefaultBranch: "main", }, nil }, + GetCodespacePreFlightFunc: func(ctx context.Context, nwo string) (*api.User, error) { + return &api.User{ + Login: "monalisa", + Type: "User", + }, nil + }, GetCodespacesMachinesFunc: func(ctx context.Context, repoID int, branch, location string) ([]*api.Machine, error) { return []*api.Machine{ { @@ -140,12 +140,6 @@ func TestApp_Create(t *testing.T) { 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{ - GetCodespacePreFlightFunc: func(ctx context.Context, nwo string) (*api.User, error) { - return &api.User{ - Login: "monalisa", - Type: "User", - }, nil - }, GetRepositoryFunc: func(ctx context.Context, nwo string) (*api.Repository, error) { return &api.Repository{ ID: 1234, @@ -153,6 +147,12 @@ func TestApp_Create(t *testing.T) { DefaultBranch: "main", }, nil }, + GetCodespacePreFlightFunc: func(ctx context.Context, nwo string) (*api.User, error) { + return &api.User{ + Login: "monalisa", + Type: "User", + }, nil + }, ListDevContainersFunc: func(ctx context.Context, repoID int, branch string, limit int) ([]api.DevContainerEntry, error) { return []api.DevContainerEntry{}, nil }, @@ -199,12 +199,6 @@ func TestApp_Create(t *testing.T) { name: "returns error when getting devcontainer paths fails", fields: fields{ apiClient: &apiClientMock{ - GetCodespacePreFlightFunc: func(ctx context.Context, nwo string) (*api.User, error) { - return &api.User{ - Login: "monalisa", - Type: "User", - }, nil - }, GetRepositoryFunc: func(ctx context.Context, nwo string) (*api.Repository, error) { return &api.Repository{ ID: 1234, @@ -212,6 +206,12 @@ func TestApp_Create(t *testing.T) { DefaultBranch: "main", }, nil }, + GetCodespacePreFlightFunc: func(ctx context.Context, nwo string) (*api.User, error) { + return &api.User{ + Login: "monalisa", + Type: "User", + }, nil + }, ListDevContainersFunc: func(ctx context.Context, repoID int, branch string, limit int) ([]api.DevContainerEntry, error) { return nil, fmt.Errorf("some error") }, @@ -230,12 +230,6 @@ func TestApp_Create(t *testing.T) { name: "create codespace with default branch does not show idle timeout notice if not conntected to terminal", fields: fields{ apiClient: &apiClientMock{ - GetCodespacePreFlightFunc: func(ctx context.Context, nwo string) (*api.User, error) { - return &api.User{ - Login: "monalisa", - Type: "User", - }, nil - }, GetRepositoryFunc: func(ctx context.Context, nwo string) (*api.Repository, error) { return &api.Repository{ ID: 1234, @@ -243,6 +237,12 @@ func TestApp_Create(t *testing.T) { DefaultBranch: "main", }, nil }, + GetCodespacePreFlightFunc: func(ctx context.Context, nwo string) (*api.User, error) { + return &api.User{ + Login: "monalisa", + Type: "User", + }, nil + }, ListDevContainersFunc: func(ctx context.Context, repoID int, branch string, limit int) ([]api.DevContainerEntry, error) { return []api.DevContainerEntry{}, nil }, @@ -339,6 +339,75 @@ Open this URL in your browser to review and authorize additional permissions: ex Alternatively, you can run "create" with the "--default-permissions" option to continue without authorizing additional permissions. `, }, + { + name: "returns error when user can't create codepaces for a repository", + fields: fields{ + apiClient: &apiClientMock{ + GetRepositoryFunc: func(ctx context.Context, nwo string) (*api.Repository, error) { + return &api.Repository{ + ID: 1234, + FullName: nwo, + DefaultBranch: "main", + }, nil + }, + GetCodespacePreFlightFunc: func(ctx context.Context, nwo string) (*api.User, error) { + return nil, fmt.Errorf("some error") + }, + }, + }, + opts: createOptions{ + repo: "megacorp/private", + branch: "", + machine: "GIGA", + showStatus: false, + idleTimeout: 30 * time.Minute, + }, + wantErr: fmt.Errorf("error checking codespace ownership: some error"), + }, + { + name: "mentions billable owner when org covers codepaces for a repository", + fields: fields{ + apiClient: &apiClientMock{ + GetRepositoryFunc: func(ctx context.Context, nwo string) (*api.Repository, error) { + return &api.Repository{ + ID: 1234, + FullName: nwo, + DefaultBranch: "main", + }, nil + }, + GetCodespacePreFlightFunc: func(ctx context.Context, nwo string) (*api.User, error) { + return &api.User{ + Type: "Organization", + Login: "megacorp", + }, 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{ + { + Name: "GIGA", + DisplayName: "Gigabits of a machine", + }, + }, nil + }, + CreateCodespaceFunc: func(ctx context.Context, params *api.CreateCodespaceParams) (*api.Codespace, error) { + return &api.Codespace{ + Name: "megacorp-private-abcd1234", + }, nil + }, + }, + }, + opts: createOptions{ + repo: "megacorp/private", + branch: "", + machine: "GIGA", + showStatus: false, + idleTimeout: 30 * time.Minute, + }, + wantStdout: "✓ Codespaces usage for this repository is paid for by megacorp\nmegacorp-private-abcd1234\n", + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { From ddbf8590e8d79ebea73cff4653be4804a834cfed Mon Sep 17 00:00:00 2001 From: Jake Shorty Date: Sun, 19 Jun 2022 18:28:33 +0000 Subject: [PATCH 10/13] Fix error message and JSON keys --- internal/codespaces/api/api.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/internal/codespaces/api/api.go b/internal/codespaces/api/api.go index b13d5847e..162232e74 100644 --- a/internal/codespaces/api/api.go +++ b/internal/codespaces/api/api.go @@ -574,8 +574,8 @@ func (a *API) GetCodespacePreFlight(ctx context.Context, nwo string) (*User, err if resp.StatusCode == http.StatusNotFound { return nil, nil - } else if resp.StatusCode != http.StatusForbidden { - return nil, fmt.Errorf("codespaces cannot be created from that repository") + } else if resp.StatusCode == http.StatusForbidden { + return nil, fmt.Errorf("you cannot create codespaces with that repository") } else if resp.StatusCode != http.StatusOK { return nil, api.HandleHTTPError(resp) } @@ -586,9 +586,9 @@ func (a *API) GetCodespacePreFlight(ctx context.Context, nwo string) (*User, err } var response struct { - BillableOwner User `json:"billableOwner"` + BillableOwner User `json:"billable_owner"` Defaults struct { - DevcontainerPath string `json:"devcontainerPath"` + DevcontainerPath string `json:"devcontainer_path"` Location string `json:"location"` } } From 2b9f9bb92a28df627ca76e3c6e1eb2ad05ef4189 Mon Sep 17 00:00:00 2001 From: Jake Shorty Date: Sun, 19 Jun 2022 18:38:38 +0000 Subject: [PATCH 11/13] Add test for individual case --- pkg/cmd/codespace/create_test.go | 44 ++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/pkg/cmd/codespace/create_test.go b/pkg/cmd/codespace/create_test.go index 42b0fd731..4d1570ca1 100644 --- a/pkg/cmd/codespace/create_test.go +++ b/pkg/cmd/codespace/create_test.go @@ -408,6 +408,50 @@ Alternatively, you can run "create" with the "--default-permissions" option to c }, wantStdout: "✓ Codespaces usage for this repository is paid for by megacorp\nmegacorp-private-abcd1234\n", }, + { + name: "doesn't mention billable owner when it's the individual", + fields: fields{ + apiClient: &apiClientMock{ + GetRepositoryFunc: func(ctx context.Context, nwo string) (*api.Repository, error) { + return &api.Repository{ + ID: 1234, + FullName: nwo, + DefaultBranch: "main", + }, nil + }, + GetCodespacePreFlightFunc: func(ctx context.Context, nwo string) (*api.User, error) { + return &api.User{ + Type: "User", + Login: "monalisa", + }, 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{ + { + Name: "GIGA", + DisplayName: "Gigabits of a machine", + }, + }, nil + }, + CreateCodespaceFunc: func(ctx context.Context, params *api.CreateCodespaceParams) (*api.Codespace, error) { + return &api.Codespace{ + Name: "megacorp-private-abcd1234", + }, nil + }, + }, + }, + opts: createOptions{ + repo: "megacorp/private", + branch: "", + machine: "GIGA", + showStatus: false, + idleTimeout: 30 * time.Minute, + }, + wantStdout: "megacorp-private-abcd1234\n", + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { From 832f35e0005b7b98f2378de20f8f5848ef9fbb72 Mon Sep 17 00:00:00 2001 From: Jake Shorty Date: Mon, 20 Jun 2022 17:12:07 +0000 Subject: [PATCH 12/13] PreFlight => BillableOwner for less confusion --- internal/codespaces/api/api.go | 4 +-- pkg/cmd/codespace/common.go | 2 +- pkg/cmd/codespace/create.go | 2 +- pkg/cmd/codespace/create_test.go | 18 +++++++------- pkg/cmd/codespace/mock_api.go | 42 ++++++++++++++++---------------- 5 files changed, 34 insertions(+), 34 deletions(-) diff --git a/internal/codespaces/api/api.go b/internal/codespaces/api/api.go index 162232e74..8eec820a8 100644 --- a/internal/codespaces/api/api.go +++ b/internal/codespaces/api/api.go @@ -557,9 +557,9 @@ func (a *API) GetCodespaceRepoSuggestions(ctx context.Context, partialSearch str return repoNames, nil } -// GetCodespacePreFlight returns the billable owner and expected default values for +// GetCodespaceBillableOwner returns the billable owner and expected default values for // codespaces created by the user for a given repository. -func (a *API) GetCodespacePreFlight(ctx context.Context, nwo string) (*User, error) { +func (a *API) GetCodespaceBillableOwner(ctx context.Context, nwo string) (*User, error) { req, err := http.NewRequest(http.MethodGet, a.githubAPI+"/repos/"+nwo+"/codespaces/new", nil) if err != nil { return nil, fmt.Errorf("error creating request: %w", err) diff --git a/pkg/cmd/codespace/common.go b/pkg/cmd/codespace/common.go index ef918cb11..6d272bcbb 100644 --- a/pkg/cmd/codespace/common.go +++ b/pkg/cmd/codespace/common.go @@ -120,7 +120,7 @@ type apiClient interface { GetCodespaceRepositoryContents(ctx context.Context, codespace *api.Codespace, path string) ([]byte, 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) - GetCodespacePreFlight(ctx context.Context, nwo string) (*api.User, error) + GetCodespaceBillableOwner(ctx context.Context, nwo string) (*api.User, error) } var errNoCodespaces = errors.New("you have no codespaces") diff --git a/pkg/cmd/codespace/create.go b/pkg/cmd/codespace/create.go index 6da70bfe9..7092353c1 100644 --- a/pkg/cmd/codespace/create.go +++ b/pkg/cmd/codespace/create.go @@ -143,7 +143,7 @@ func (a *App) Create(ctx context.Context, opts createOptions) error { } a.StartProgressIndicatorWithLabel("Validating repository for codespaces") - billableOwner, err := a.apiClient.GetCodespacePreFlight(ctx, userInputs.Repository) + billableOwner, err := a.apiClient.GetCodespaceBillableOwner(ctx, userInputs.Repository) a.StopProgressIndicator() if billableOwner != nil && billableOwner.Type == "Organization" { diff --git a/pkg/cmd/codespace/create_test.go b/pkg/cmd/codespace/create_test.go index 4d1570ca1..7a14a6056 100644 --- a/pkg/cmd/codespace/create_test.go +++ b/pkg/cmd/codespace/create_test.go @@ -36,7 +36,7 @@ func TestApp_Create(t *testing.T) { DefaultBranch: "main", }, nil }, - GetCodespacePreFlightFunc: func(ctx context.Context, nwo string) (*api.User, error) { + GetCodespaceBillableOwnerFunc: func(ctx context.Context, nwo string) (*api.User, error) { return &api.User{ Login: "monalisa", Type: "User", @@ -93,7 +93,7 @@ func TestApp_Create(t *testing.T) { DefaultBranch: "main", }, nil }, - GetCodespacePreFlightFunc: func(ctx context.Context, nwo string) (*api.User, error) { + GetCodespaceBillableOwnerFunc: func(ctx context.Context, nwo string) (*api.User, error) { return &api.User{ Login: "monalisa", Type: "User", @@ -147,7 +147,7 @@ func TestApp_Create(t *testing.T) { DefaultBranch: "main", }, nil }, - GetCodespacePreFlightFunc: func(ctx context.Context, nwo string) (*api.User, error) { + GetCodespaceBillableOwnerFunc: func(ctx context.Context, nwo string) (*api.User, error) { return &api.User{ Login: "monalisa", Type: "User", @@ -206,7 +206,7 @@ func TestApp_Create(t *testing.T) { DefaultBranch: "main", }, nil }, - GetCodespacePreFlightFunc: func(ctx context.Context, nwo string) (*api.User, error) { + GetCodespaceBillableOwnerFunc: func(ctx context.Context, nwo string) (*api.User, error) { return &api.User{ Login: "monalisa", Type: "User", @@ -237,7 +237,7 @@ func TestApp_Create(t *testing.T) { DefaultBranch: "main", }, nil }, - GetCodespacePreFlightFunc: func(ctx context.Context, nwo string) (*api.User, error) { + GetCodespaceBillableOwnerFunc: func(ctx context.Context, nwo string) (*api.User, error) { return &api.User{ Login: "monalisa", Type: "User", @@ -286,7 +286,7 @@ func TestApp_Create(t *testing.T) { name: "create codespace that requires accepting additional permissions", fields: fields{ apiClient: &apiClientMock{ - GetCodespacePreFlightFunc: func(ctx context.Context, nwo string) (*api.User, error) { + GetCodespaceBillableOwnerFunc: func(ctx context.Context, nwo string) (*api.User, error) { return &api.User{ Login: "monalisa", Type: "User", @@ -350,7 +350,7 @@ Alternatively, you can run "create" with the "--default-permissions" option to c DefaultBranch: "main", }, nil }, - GetCodespacePreFlightFunc: func(ctx context.Context, nwo string) (*api.User, error) { + GetCodespaceBillableOwnerFunc: func(ctx context.Context, nwo string) (*api.User, error) { return nil, fmt.Errorf("some error") }, }, @@ -375,7 +375,7 @@ Alternatively, you can run "create" with the "--default-permissions" option to c DefaultBranch: "main", }, nil }, - GetCodespacePreFlightFunc: func(ctx context.Context, nwo string) (*api.User, error) { + GetCodespaceBillableOwnerFunc: func(ctx context.Context, nwo string) (*api.User, error) { return &api.User{ Type: "Organization", Login: "megacorp", @@ -419,7 +419,7 @@ Alternatively, you can run "create" with the "--default-permissions" option to c DefaultBranch: "main", }, nil }, - GetCodespacePreFlightFunc: func(ctx context.Context, nwo string) (*api.User, error) { + GetCodespaceBillableOwnerFunc: func(ctx context.Context, nwo string) (*api.User, error) { return &api.User{ Type: "User", Login: "monalisa", diff --git a/pkg/cmd/codespace/mock_api.go b/pkg/cmd/codespace/mock_api.go index d12644de7..3999692f2 100644 --- a/pkg/cmd/codespace/mock_api.go +++ b/pkg/cmd/codespace/mock_api.go @@ -31,8 +31,8 @@ import ( // GetCodespaceFunc: func(ctx context.Context, name string, includeConnection bool) (*api.Codespace, error) { // panic("mock out the GetCodespace method") // }, -// GetCodespacePreFlightFunc: func(ctx context.Context, nwo string) (*api.User, error) { -// panic("mock out the GetCodespacePreFlight method") +// GetCodespaceBillableOwnerFunc: func(ctx context.Context, nwo string) (*api.User, error) { +// panic("mock out the GetCodespaceBillableOwner method") // }, // GetCodespaceRepoSuggestionsFunc: func(ctx context.Context, partialSearch string, params api.RepoSearchParameters) ([]string, error) { // panic("mock out the GetCodespaceRepoSuggestions method") @@ -83,8 +83,8 @@ type apiClientMock struct { // GetCodespaceFunc mocks the GetCodespace method. GetCodespaceFunc func(ctx context.Context, name string, includeConnection bool) (*api.Codespace, error) - // GetCodespacePreFlightFunc mocks the GetCodespacePreFlight method. - GetCodespacePreFlightFunc func(ctx context.Context, nwo string) (*api.User, error) + // GetCodespaceBillableOwnerFunc mocks the GetCodespaceBillableOwner method. + GetCodespaceBillableOwnerFunc func(ctx context.Context, nwo string) (*api.User, error) // GetCodespaceRepoSuggestionsFunc mocks the GetCodespaceRepoSuggestions method. GetCodespaceRepoSuggestionsFunc func(ctx context.Context, partialSearch string, params api.RepoSearchParameters) ([]string, error) @@ -154,8 +154,8 @@ type apiClientMock struct { // IncludeConnection is the includeConnection argument value. IncludeConnection bool } - // GetCodespacePreFlight holds details about calls to the GetCodespacePreFlight method. - GetCodespacePreFlight []struct { + // GetCodespaceBillableOwner holds details about calls to the GetCodespaceBillableOwner method. + GetCodespaceBillableOwner []struct { // Ctx is the ctx argument value. Ctx context.Context // Nwo is the nwo argument value. @@ -240,7 +240,7 @@ type apiClientMock struct { lockDeleteCodespace sync.RWMutex lockEditCodespace sync.RWMutex lockGetCodespace sync.RWMutex - lockGetCodespacePreFlight sync.RWMutex + lockGetCodespaceBillableOwner sync.RWMutex lockGetCodespaceRepoSuggestions sync.RWMutex lockGetCodespaceRepositoryContents sync.RWMutex lockGetCodespacesMachines sync.RWMutex @@ -435,10 +435,10 @@ func (mock *apiClientMock) GetCodespaceCalls() []struct { return calls } -// GetCodespacePreFlight calls GetCodespacePreFlightFunc. -func (mock *apiClientMock) GetCodespacePreFlight(ctx context.Context, nwo string) (*api.User, error) { - if mock.GetCodespacePreFlightFunc == nil { - panic("apiClientMock.GetCodespacePreFlightFunc: method is nil but apiClient.GetCodespacePreFlight was just called") +// GetCodespaceBillableOwner calls GetCodespaceBillableOwnerFunc. +func (mock *apiClientMock) GetCodespaceBillableOwner(ctx context.Context, nwo string) (*api.User, error) { + if mock.GetCodespaceBillableOwnerFunc == nil { + panic("apiClientMock.GetCodespaceBillableOwnerFunc: method is nil but apiClient.GetCodespaceBillableOwner was just called") } callInfo := struct { Ctx context.Context @@ -447,16 +447,16 @@ func (mock *apiClientMock) GetCodespacePreFlight(ctx context.Context, nwo string Ctx: ctx, Nwo: nwo, } - mock.lockGetCodespacePreFlight.Lock() - mock.calls.GetCodespacePreFlight = append(mock.calls.GetCodespacePreFlight, callInfo) - mock.lockGetCodespacePreFlight.Unlock() - return mock.GetCodespacePreFlightFunc(ctx, nwo) + mock.lockGetCodespaceBillableOwner.Lock() + mock.calls.GetCodespaceBillableOwner = append(mock.calls.GetCodespaceBillableOwner, callInfo) + mock.lockGetCodespaceBillableOwner.Unlock() + return mock.GetCodespaceBillableOwnerFunc(ctx, nwo) } -// GetCodespacePreFlightCalls gets all the calls that were made to GetCodespacePreFlight. +// GetCodespaceBillableOwnerCalls gets all the calls that were made to GetCodespaceBillableOwner. // Check the length with: -// len(mockedapiClient.GetCodespacePreFlightCalls()) -func (mock *apiClientMock) GetCodespacePreFlightCalls() []struct { +// len(mockedapiClient.GetCodespaceBillableOwnerCalls()) +func (mock *apiClientMock) GetCodespaceBillableOwnerCalls() []struct { Ctx context.Context Nwo string } { @@ -464,9 +464,9 @@ func (mock *apiClientMock) GetCodespacePreFlightCalls() []struct { Ctx context.Context Nwo string } - mock.lockGetCodespacePreFlight.RLock() - calls = mock.calls.GetCodespacePreFlight - mock.lockGetCodespacePreFlight.RUnlock() + mock.lockGetCodespaceBillableOwner.RLock() + calls = mock.calls.GetCodespaceBillableOwner + mock.lockGetCodespaceBillableOwner.RUnlock() return calls } From df3b40999ed6c46f98d4f10e641c2b853f1664a0 Mon Sep 17 00:00:00 2001 From: Jake Shorty Date: Mon, 20 Jun 2022 17:13:42 +0000 Subject: [PATCH 13/13] Flip conditional --- pkg/cmd/codespace/create.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/cmd/codespace/create.go b/pkg/cmd/codespace/create.go index 7092353c1..764fe4314 100644 --- a/pkg/cmd/codespace/create.go +++ b/pkg/cmd/codespace/create.go @@ -146,11 +146,11 @@ func (a *App) Create(ctx context.Context, opts createOptions) error { billableOwner, err := a.apiClient.GetCodespaceBillableOwner(ctx, userInputs.Repository) a.StopProgressIndicator() - if billableOwner != nil && billableOwner.Type == "Organization" { + if err != nil { + return fmt.Errorf("error checking codespace ownership: %w", err) + } else if billableOwner != nil && billableOwner.Type == "Organization" { cs := a.io.ColorScheme() fmt.Fprintln(a.io.Out, cs.Blue("✓ Codespaces usage for this repository is paid for by "+billableOwner.Login)) - } else if err != nil { - return fmt.Errorf("error checking codespace ownership: %w", err) } if promptForRepoAndBranch {