From 500f3595921d65d2c866ca4c003b7d4b83e673b7 Mon Sep 17 00:00:00 2001 From: David Gardiner Date: Mon, 30 Oct 2023 11:22:38 -0700 Subject: [PATCH] Don't poll for addition codespace permissions if user chooses to create without them (#8267) --- internal/codespaces/api/api.go | 3 +- pkg/cmd/codespace/common.go | 2 +- pkg/cmd/codespace/create.go | 51 ++++++++++++++++++---------------- pkg/cmd/codespace/mock_api.go | 14 +++------- 4 files changed, 33 insertions(+), 37 deletions(-) diff --git a/internal/codespaces/api/api.go b/internal/codespaces/api/api.go index 950a8ea52..9e79eafe1 100644 --- a/internal/codespaces/api/api.go +++ b/internal/codespaces/api/api.go @@ -640,7 +640,7 @@ func (a *API) GetCodespacesMachines(ctx context.Context, repoID int, branch, loc } // GetCodespacesPermissionsCheck returns a bool indicating whether the user has accepted permissions for the given repo and devcontainer path. -func (a *API) GetCodespacesPermissionsCheck(ctx context.Context, repoID int, branch string, location string, devcontainerPath string) (bool, error) { +func (a *API) GetCodespacesPermissionsCheck(ctx context.Context, repoID int, branch string, devcontainerPath string) (bool, error) { reqURL := fmt.Sprintf("%s/repositories/%d/codespaces/permissions_check", a.githubAPI, repoID) req, err := http.NewRequest(http.MethodGet, reqURL, nil) if err != nil { @@ -648,7 +648,6 @@ func (a *API) GetCodespacesPermissionsCheck(ctx context.Context, repoID int, bra } q := req.URL.Query() - q.Add("location", location) q.Add("ref", branch) q.Add("devcontainer_path", devcontainerPath) req.URL.RawQuery = q.Encode() diff --git a/pkg/cmd/codespace/common.go b/pkg/cmd/codespace/common.go index 78dae09e4..99d1f339c 100644 --- a/pkg/cmd/codespace/common.go +++ b/pkg/cmd/codespace/common.go @@ -77,7 +77,7 @@ type apiClient interface { EditCodespace(ctx context.Context, codespaceName string, params *api.EditCodespaceParams) (*api.Codespace, error) GetRepository(ctx context.Context, nwo string) (*api.Repository, error) GetCodespacesMachines(ctx context.Context, repoID int, branch string, location string, devcontainerPath string) ([]*api.Machine, error) - GetCodespacesPermissionsCheck(ctx context.Context, repoID int, branch string, location string, devcontainerPath string) (bool, error) + GetCodespacesPermissionsCheck(ctx context.Context, repoID int, branch string, devcontainerPath string) (bool, error) 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) diff --git a/pkg/cmd/codespace/create.go b/pkg/cmd/codespace/create.go index 485f8e57c..fa484173d 100644 --- a/pkg/cmd/codespace/create.go +++ b/pkg/cmd/codespace/create.go @@ -312,7 +312,7 @@ func (a *App) Create(ctx context.Context, opts createOptions) error { return fmt.Errorf("error creating codespace: %w", err) } - codespace, err = a.handleAdditionalPermissions(ctx, createParams, aerr.AllowPermissionsURL, userInputs.Location) + codespace, err = a.handleAdditionalPermissions(ctx, createParams, aerr.AllowPermissionsURL) if err != nil { // this error could be a cmdutil.SilentError (in the case that the user opened the browser) so we don't want to wrap it return err @@ -336,7 +336,7 @@ func (a *App) Create(ctx context.Context, opts createOptions) error { return nil } -func (a *App) handleAdditionalPermissions(ctx context.Context, createParams *api.CreateCodespaceParams, allowPermissionsURL string, location string) (*api.Codespace, error) { +func (a *App) handleAdditionalPermissions(ctx context.Context, createParams *api.CreateCodespaceParams, allowPermissionsURL string) (*api.Codespace, error) { var ( isInteractive = a.io.CanPrompt() cs = a.io.ColorScheme() @@ -380,17 +380,39 @@ func (a *App) handleAdditionalPermissions(ctx context.Context, createParams *api if err := a.browser.Browse(allowPermissionsURL); err != nil { return nil, fmt.Errorf("error opening browser: %w", err) } + + // Poll until the user has accepted the permissions or timeout + if err := a.pollForPermissions(ctx, createParams); err != nil { + return nil, fmt.Errorf("error polling for permissions: %w", err) + } } - // Poll until the user has accepted the permissions or timeout - err := a.RunWithProgress("Waiting for permissions to be accepted in the browser", func() (err error) { + // if the user chose to create the codespace without the permissions, + // we can continue with the create opting out of the additional permissions + createParams.PermissionsOptOut = true + + var codespace *api.Codespace + err := a.RunWithProgress("Creating codespace", func() (err error) { + codespace, err = a.apiClient.CreateCodespace(ctx, createParams) + return + }) + + if err != nil { + return nil, fmt.Errorf("error creating codespace: %w", err) + } + + return codespace, nil +} + +func (a *App) pollForPermissions(ctx context.Context, createParams *api.CreateCodespaceParams) error { + return a.RunWithProgress("Waiting for permissions to be accepted in the browser", func() (err error) { ctx, cancel := context.WithTimeout(ctx, permissionsPollingTimeout) defer cancel() done := make(chan error, 1) go func() { for { - accepted, err := a.apiClient.GetCodespacesPermissionsCheck(ctx, createParams.RepositoryID, createParams.Branch, location, createParams.DevContainerPath) + accepted, err := a.apiClient.GetCodespacesPermissionsCheck(ctx, createParams.RepositoryID, createParams.Branch, createParams.DevContainerPath) if err != nil { done <- err return @@ -413,25 +435,6 @@ func (a *App) handleAdditionalPermissions(ctx context.Context, createParams *api return fmt.Errorf("timed out waiting for permissions to be accepted in the browser") } }) - if err != nil { - return nil, fmt.Errorf("error polling for permissions: %w", err) - } - - // if the user chose to create the codespace without the permissions, - // we can continue with the create opting out of the additional permissions - createParams.PermissionsOptOut = true - - var codespace *api.Codespace - err = a.RunWithProgress("Creating codespace", func() (err error) { - codespace, err = a.apiClient.CreateCodespace(ctx, createParams) - return - }) - - if err != nil { - return nil, fmt.Errorf("error creating codespace: %w", err) - } - - return codespace, nil } // showStatus polls the codespace for a list of post create states and their status. It will keep polling diff --git a/pkg/cmd/codespace/mock_api.go b/pkg/cmd/codespace/mock_api.go index d6cf37eeb..e4de0ae7a 100644 --- a/pkg/cmd/codespace/mock_api.go +++ b/pkg/cmd/codespace/mock_api.go @@ -41,7 +41,7 @@ import ( // GetCodespacesMachinesFunc: func(ctx context.Context, repoID int, branch string, location string, devcontainerPath string) ([]*codespacesAPI.Machine, error) { // panic("mock out the GetCodespacesMachines method") // }, -// GetCodespacesPermissionsCheckFunc: func(ctx context.Context, repoID int, branch string, location string, devcontainerPath string) (bool, error) { +// GetCodespacesPermissionsCheckFunc: func(ctx context.Context, repoID int, branch string, devcontainerPath string) (bool, error) { // panic("mock out the GetCodespacesPermissionsCheck method") // }, // GetOrgMemberCodespaceFunc: func(ctx context.Context, orgName string, userName string, codespaceName string) (*codespacesAPI.Codespace, error) { @@ -103,7 +103,7 @@ type apiClientMock struct { GetCodespacesMachinesFunc func(ctx context.Context, repoID int, branch string, location string, devcontainerPath string) ([]*codespacesAPI.Machine, error) // GetCodespacesPermissionsCheckFunc mocks the GetCodespacesPermissionsCheck method. - GetCodespacesPermissionsCheckFunc func(ctx context.Context, repoID int, branch string, location string, devcontainerPath string) (bool, error) + GetCodespacesPermissionsCheckFunc func(ctx context.Context, repoID int, branch string, devcontainerPath string) (bool, error) // GetOrgMemberCodespaceFunc mocks the GetOrgMemberCodespace method. GetOrgMemberCodespaceFunc func(ctx context.Context, orgName string, userName string, codespaceName string) (*codespacesAPI.Codespace, error) @@ -216,8 +216,6 @@ type apiClientMock struct { RepoID int // Branch is the branch argument value. Branch string - // Location is the location argument value. - Location string // DevcontainerPath is the devcontainerPath argument value. DevcontainerPath string } @@ -632,7 +630,7 @@ func (mock *apiClientMock) GetCodespacesMachinesCalls() []struct { } // GetCodespacesPermissionsCheck calls GetCodespacesPermissionsCheckFunc. -func (mock *apiClientMock) GetCodespacesPermissionsCheck(ctx context.Context, repoID int, branch string, location string, devcontainerPath string) (bool, error) { +func (mock *apiClientMock) GetCodespacesPermissionsCheck(ctx context.Context, repoID int, branch string, devcontainerPath string) (bool, error) { if mock.GetCodespacesPermissionsCheckFunc == nil { panic("apiClientMock.GetCodespacesPermissionsCheckFunc: method is nil but apiClient.GetCodespacesPermissionsCheck was just called") } @@ -640,19 +638,17 @@ func (mock *apiClientMock) GetCodespacesPermissionsCheck(ctx context.Context, re Ctx context.Context RepoID int Branch string - Location string DevcontainerPath string }{ Ctx: ctx, RepoID: repoID, Branch: branch, - Location: location, DevcontainerPath: devcontainerPath, } mock.lockGetCodespacesPermissionsCheck.Lock() mock.calls.GetCodespacesPermissionsCheck = append(mock.calls.GetCodespacesPermissionsCheck, callInfo) mock.lockGetCodespacesPermissionsCheck.Unlock() - return mock.GetCodespacesPermissionsCheckFunc(ctx, repoID, branch, location, devcontainerPath) + return mock.GetCodespacesPermissionsCheckFunc(ctx, repoID, branch, devcontainerPath) } // GetCodespacesPermissionsCheckCalls gets all the calls that were made to GetCodespacesPermissionsCheck. @@ -663,14 +659,12 @@ func (mock *apiClientMock) GetCodespacesPermissionsCheckCalls() []struct { Ctx context.Context RepoID int Branch string - Location string DevcontainerPath string } { var calls []struct { Ctx context.Context RepoID int Branch string - Location string DevcontainerPath string } mock.lockGetCodespacesPermissionsCheck.RLock()