From 0b68aaab7edf7083679e0d257b4fc2e18aa5e26e Mon Sep 17 00:00:00 2001 From: Jose Garcia Date: Tue, 21 Sep 2021 09:59:16 -0400 Subject: [PATCH 01/16] Return error on 202 responses - Start implementing the retry/poll flow --- cmd/ghcs/create.go | 19 ++++++++++++++++++- internal/api/api.go | 7 ++++++- 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/cmd/ghcs/create.go b/cmd/ghcs/create.go index 2125176fd..ff3e13962 100644 --- a/cmd/ghcs/create.go +++ b/cmd/ghcs/create.go @@ -6,6 +6,7 @@ import ( "fmt" "os" "strings" + "time" "github.com/AlecAivazis/survey/v2" "github.com/fatih/camelcase" @@ -87,8 +88,20 @@ func create(opts *createOptions) error { log.Println("Creating your codespace...") - codespace, err := apiClient.CreateCodespace(ctx, userResult.User, repository, machine, branch, locationResult.Location) + codespace, err := apiClient.CreateCodespace( + ctx, userResult.User, repository, machine, branch, locationResult.Location, + ) if err != nil { + if err == api.ErrCreateAsyncRetry { + createRetryCtx, cancelRetry := context.WithTimeout(ctx, 2*time.Minute) + defer cancelRetry() + + codespace, err = pollForProvisionedCodespace(createRetryCtx, codespace) + if err != nil { + return fmt.Errorf("error creating codespace after retry: %w", err) + } + } + return fmt.Errorf("error creating codespace: %w", err) } @@ -105,6 +118,10 @@ func create(opts *createOptions) error { return nil } +func pollForProvisionedCodespace(ctx context.Context, provisioningCodespace *api.Codespace) (*api.Codespace, error) { + return nil, nil +} + // showStatus polls the codespace for a list of post create states and their status. It will keep polling // until all states have finished. Once all states have finished, we poll once more to check if any new // states have been introduced and stop polling otherwise. diff --git a/internal/api/api.go b/internal/api/api.go index 1246389e8..df9fd10c7 100644 --- a/internal/api/api.go +++ b/internal/api/api.go @@ -401,6 +401,8 @@ type createCodespaceRequest struct { SkuName string `json:"sku_name"` } +var ErrCreateAsyncRetry = errors.New("initial creation failed, retrying async") + func (a *API) CreateCodespace(ctx context.Context, user *User, repository *Repository, sku, branch, location string) (*Codespace, error) { requestBody, err := json.Marshal(createCodespaceRequest{repository.ID, branch, location, sku}) if err != nil { @@ -424,8 +426,11 @@ func (a *API) CreateCodespace(ctx context.Context, user *User, repository *Repos return nil, fmt.Errorf("error reading response body: %w", err) } - if resp.StatusCode > http.StatusAccepted { + switch { + case resp.StatusCode > http.StatusAccepted: return nil, jsonErrorResponse(b) + case resp.StatusCode == http.StatusAccepted: + return nil, ErrCreateAsyncRetry } var response Codespace From 323462ca5c3ed803da22b47f68e24d7d697c43bf Mon Sep 17 00:00:00 2001 From: Jose Garcia Date: Tue, 21 Sep 2021 12:37:11 -0400 Subject: [PATCH 02/16] Poll codespace on ErrCreateAsyncRetry error - Introduce tests for the poller - Attempt to fetch codespace for 2 mins --- cmd/ghcs/create.go | 50 +++++++++++++++++++--- cmd/ghcs/create_test.go | 93 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 137 insertions(+), 6 deletions(-) create mode 100644 cmd/ghcs/create_test.go diff --git a/cmd/ghcs/create.go b/cmd/ghcs/create.go index ff3e13962..93016bbf8 100644 --- a/cmd/ghcs/create.go +++ b/cmd/ghcs/create.go @@ -92,13 +92,19 @@ func create(opts *createOptions) error { ctx, userResult.User, repository, machine, branch, locationResult.Location, ) if err != nil { + // This error is returned by the API when the initial creation fails with a retryable error. + // A retryable error means that GitHub will retry to re-create Codespace and clients should poll + // the API and attempt to fetch the Codespace for the next two minutes. if err == api.ErrCreateAsyncRetry { - createRetryCtx, cancelRetry := context.WithTimeout(ctx, 2*time.Minute) - defer cancelRetry() + log.Print("Switching to async provisioning...") + pollctx, cancel := context.WithTimeout(ctx, 2*time.Minute) + defer cancel() + + codespace, err = pollForCodespace(pollctx, apiClient, log, userResult.User, codespace) + log.Print("\n") - codespace, err = pollForProvisionedCodespace(createRetryCtx, codespace) if err != nil { - return fmt.Errorf("error creating codespace after retry: %w", err) + return fmt.Errorf("error creating codespace with async provisioning: %s: %w", codespace.Name, err) } } @@ -118,8 +124,40 @@ func create(opts *createOptions) error { return nil } -func pollForProvisionedCodespace(ctx context.Context, provisioningCodespace *api.Codespace) (*api.Codespace, error) { - return nil, nil +type apiClient interface { + GetCodespaceToken(context.Context, string, string) (string, error) + GetCodespace(context.Context, string, string, string) (*api.Codespace, error) +} + +// pollForCodespace polls the Codespaces API every second fetching the codespace. +// If it succeeds at fetching the codespace, we consider the codespace provisioned. +// Context should be cancelled to stop polling. +func pollForCodespace( + ctx context.Context, client apiClient, log *output.Logger, user *api.User, provisioningCodespace *api.Codespace, +) (*api.Codespace, error) { + ticker := time.NewTicker(1 * time.Second) + defer ticker.Stop() + + for { + select { + case <-ctx.Done(): + return nil, ctx.Err() + case <-ticker.C: + log.Print(".") + token, err := client.GetCodespaceToken(ctx, user.Login, provisioningCodespace.Name) + if err != nil { + // Do nothing. We expect this to fail until the codespace is provisioned + continue + } + + codespace, err := client.GetCodespace(ctx, token, user.Login, provisioningCodespace.Name) + if err != nil { + return nil, fmt.Errorf("failed to get 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/cmd/ghcs/create_test.go b/cmd/ghcs/create_test.go new file mode 100644 index 000000000..36769dc14 --- /dev/null +++ b/cmd/ghcs/create_test.go @@ -0,0 +1,93 @@ +package main + +import ( + "context" + "errors" + "fmt" + "testing" + "time" + + "github.com/github/ghcs/cmd/ghcs/output" + "github.com/github/ghcs/internal/api" +) + +type mockAPIClient struct { + getCodespaceToken func(context.Context, string, string) (string, error) + getCodespace func(context.Context, string, string, string) (*api.Codespace, error) +} + +func (m *mockAPIClient) GetCodespaceToken(ctx context.Context, userLogin, codespaceName string) (string, error) { + if m.getCodespaceToken == nil { + return "", errors.New("mock api client GetCodespaceToken not implemented") + } + + return m.getCodespaceToken(ctx, userLogin, codespaceName) +} + +func (m *mockAPIClient) GetCodespace(ctx context.Context, token, userLogin, codespaceName string) (*api.Codespace, error) { + if m.getCodespace == nil { + return nil, errors.New("mock api client GetCodespace not implemented") + } + + return m.getCodespace(ctx, token, userLogin, codespaceName) +} + +func TestPollForCodespace(t *testing.T) { + logger := output.NewLogger(nil, nil, false) + user := &api.User{Login: "test"} + tmpCodespace := &api.Codespace{Name: "tmp-codespace"} + codespaceToken := "codespace-token" + + ctxTimeout := 1 * time.Second + exceedTime := 2 * time.Second + exceedProvisioningTime := false + + api := &mockAPIClient{ + getCodespaceToken: func(ctx context.Context, userLogin, codespace string) (string, error) { + if exceedProvisioningTime { + ticker := time.NewTicker(exceedTime) + defer ticker.Stop() + <-ticker.C + } + if userLogin != user.Login { + return "", fmt.Errorf("user does not match, got: %s, expected: %s", userLogin, user.Login) + } + if codespace != tmpCodespace.Name { + return "", fmt.Errorf("codespace does not match, got: %s, expected: %s", codespace, tmpCodespace.Name) + } + return codespaceToken, nil + }, + getCodespace: func(ctx context.Context, token, userLogin, codespace string) (*api.Codespace, error) { + if token != codespaceToken { + return nil, fmt.Errorf("token does not match, got: %s, expected: %s", token, codespaceToken) + } + if userLogin != user.Login { + return nil, fmt.Errorf("user does not match, got: %s, expected: %s", userLogin, user.Login) + } + if codespace != tmpCodespace.Name { + return nil, fmt.Errorf("codespace does not match, got: %s, expected: %s", codespace, tmpCodespace.Name) + } + return tmpCodespace, nil + }, + } + + ctx, cancel := context.WithTimeout(context.Background(), ctxTimeout) + defer cancel() + + codespace, err := pollForCodespace(ctx, api, logger, user, tmpCodespace) + if err != nil { + t.Error(err) + } + if tmpCodespace.Name != codespace.Name { + t.Errorf("returned codespace does not match, got: %s, expected: %s", codespace.Name, tmpCodespace.Name) + } + + exceedProvisioningTime = true + ctx, cancel = context.WithTimeout(ctx, ctxTimeout) + defer cancel() + + _, err = pollForCodespace(ctx, api, logger, user, tmpCodespace) + if err == nil { + t.Error("expected context deadline exceeded error, got nil") + } +} From 861811baf03d461e0d89113b07c80ff414c4e146 Mon Sep 17 00:00:00 2001 From: Jose Garcia Date: Tue, 21 Sep 2021 14:02:05 -0400 Subject: [PATCH 03/16] Upgrade pkg name after merge --- cmd/ghcs/create_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/ghcs/create_test.go b/cmd/ghcs/create_test.go index 36769dc14..3df900afc 100644 --- a/cmd/ghcs/create_test.go +++ b/cmd/ghcs/create_test.go @@ -1,4 +1,4 @@ -package main +package ghcs import ( "context" From 48e3473a953b9502a840bf9ea40fb818dca05f5b Mon Sep 17 00:00:00 2001 From: Jose Garcia Date: Tue, 21 Sep 2021 18:18:30 -0400 Subject: [PATCH 04/16] PR Feedback - Bring context.Timeout into the poller - Accept duration and interval - Other tidy up --- cmd/ghcs/create.go | 31 ++++++++++++------------------- cmd/ghcs/create_test.go | 26 ++++++++------------------ 2 files changed, 20 insertions(+), 37 deletions(-) diff --git a/cmd/ghcs/create.go b/cmd/ghcs/create.go index fd54a170c..a13807094 100644 --- a/cmd/ghcs/create.go +++ b/cmd/ghcs/create.go @@ -84,19 +84,17 @@ func create(opts *createOptions) error { log.Println("Creating your codespace...") - codespace, err := apiClient.CreateCodespace( - ctx, userResult.User, repository, machine, branch, locationResult.Location, - ) + codespace, err := apiClient.CreateCodespace(ctx, userResult.User, repository, machine, branch, locationResult.Location) if err != nil { // This error is returned by the API when the initial creation fails with a retryable error. // A retryable error means that GitHub will retry to re-create Codespace and clients should poll // the API and attempt to fetch the Codespace for the next two minutes. if err == api.ErrCreateAsyncRetry { log.Print("Switching to async provisioning...") - pollctx, cancel := context.WithTimeout(ctx, 2*time.Minute) - defer cancel() - codespace, err = pollForCodespace(pollctx, apiClient, log, userResult.User, codespace) + pollTimeout := 2 * time.Minute + pollInterval := 1 * time.Second + codespace, err = pollForCodespace(ctx, apiClient, log, pollTimeout, pollInterval, userResult.User.Login, codespace.Name) log.Print("\n") if err != nil { @@ -125,13 +123,13 @@ type apiClient interface { GetCodespace(context.Context, string, string, string) (*api.Codespace, error) } -// pollForCodespace polls the Codespaces API every second fetching the codespace. +// pollForCodespace polls the Codespaces GET endpoint on a given interval for a specified duration. // If it succeeds at fetching the codespace, we consider the codespace provisioned. -// Context should be cancelled to stop polling. -func pollForCodespace( - ctx context.Context, client apiClient, log *output.Logger, user *api.User, provisioningCodespace *api.Codespace, -) (*api.Codespace, error) { - ticker := time.NewTicker(1 * time.Second) +func pollForCodespace(ctx context.Context, client apiClient, log *output.Logger, duration, interval time.Duration, user, name string) (*api.Codespace, error) { + ctx, cancel := context.WithTimeout(ctx, duration) + defer cancel() + + ticker := time.NewTicker(interval) defer ticker.Stop() for { @@ -140,18 +138,13 @@ func pollForCodespace( return nil, ctx.Err() case <-ticker.C: log.Print(".") - token, err := client.GetCodespaceToken(ctx, user.Login, provisioningCodespace.Name) + token, err := client.GetCodespaceToken(ctx, user, name) if err != nil { // Do nothing. We expect this to fail until the codespace is provisioned continue } - codespace, err := client.GetCodespace(ctx, token, user.Login, provisioningCodespace.Name) - if err != nil { - return nil, fmt.Errorf("failed to get codespace: %w", err) - } - - return codespace, nil + return client.GetCodespace(ctx, token, user, name) } } } diff --git a/cmd/ghcs/create_test.go b/cmd/ghcs/create_test.go index 3df900afc..e86fa00e6 100644 --- a/cmd/ghcs/create_test.go +++ b/cmd/ghcs/create_test.go @@ -37,18 +37,13 @@ func TestPollForCodespace(t *testing.T) { user := &api.User{Login: "test"} tmpCodespace := &api.Codespace{Name: "tmp-codespace"} codespaceToken := "codespace-token" + ctx := context.Background() - ctxTimeout := 1 * time.Second - exceedTime := 2 * time.Second - exceedProvisioningTime := false + pollInterval := 50 * time.Millisecond + pollTimeout := 100 * time.Millisecond api := &mockAPIClient{ getCodespaceToken: func(ctx context.Context, userLogin, codespace string) (string, error) { - if exceedProvisioningTime { - ticker := time.NewTicker(exceedTime) - defer ticker.Stop() - <-ticker.C - } if userLogin != user.Login { return "", fmt.Errorf("user does not match, got: %s, expected: %s", userLogin, user.Login) } @@ -71,10 +66,7 @@ func TestPollForCodespace(t *testing.T) { }, } - ctx, cancel := context.WithTimeout(context.Background(), ctxTimeout) - defer cancel() - - codespace, err := pollForCodespace(ctx, api, logger, user, tmpCodespace) + codespace, err := pollForCodespace(ctx, api, logger, pollTimeout, pollInterval, user.Login, tmpCodespace.Name) if err != nil { t.Error(err) } @@ -82,12 +74,10 @@ func TestPollForCodespace(t *testing.T) { t.Errorf("returned codespace does not match, got: %s, expected: %s", codespace.Name, tmpCodespace.Name) } - exceedProvisioningTime = true - ctx, cancel = context.WithTimeout(ctx, ctxTimeout) - defer cancel() - - _, err = pollForCodespace(ctx, api, logger, user, tmpCodespace) - if err == nil { + // swap the durations to trigger a timeout + pollTimeout, pollInterval = pollInterval, pollTimeout + _, err = pollForCodespace(ctx, api, logger, pollTimeout, pollInterval, user.Login, tmpCodespace.Name) + if err != context.DeadlineExceeded { t.Error("expected context deadline exceeded error, got nil") } } From 86717f14a1a6c0ba1f9f45f55367863541c69d53 Mon Sep 17 00:00:00 2001 From: Jose Garcia Date: Wed, 22 Sep 2021 09:09:09 -0400 Subject: [PATCH 05/16] Implement codespaces.Provision - Move polling logic into the Provision function - Document the behavior expected of callers when an ErrCreateAsyncRetry is returned --- cmd/ghcs/create.go | 56 ++------------- internal/api/api.go | 4 ++ internal/codespaces/codespaces.go | 68 +++++++++++++++++++ .../codespaces/codespaces_test.go | 11 ++- 4 files changed, 89 insertions(+), 50 deletions(-) rename cmd/ghcs/create_test.go => internal/codespaces/codespaces_test.go (85%) diff --git a/cmd/ghcs/create.go b/cmd/ghcs/create.go index a13807094..91c8bcb8d 100644 --- a/cmd/ghcs/create.go +++ b/cmd/ghcs/create.go @@ -6,7 +6,6 @@ import ( "fmt" "os" "strings" - "time" "github.com/AlecAivazis/survey/v2" "github.com/fatih/camelcase" @@ -84,24 +83,14 @@ func create(opts *createOptions) error { log.Println("Creating your codespace...") - codespace, err := apiClient.CreateCodespace(ctx, userResult.User, repository, machine, branch, locationResult.Location) + codespace, err := codespaces.Provision(ctx, log, apiClient, &codespaces.ProvisionParams{ + User: userResult.User, + Repository: repository, + Branch: branch, + Machine: machine, + Location: locationResult.Location, + }) if err != nil { - // This error is returned by the API when the initial creation fails with a retryable error. - // A retryable error means that GitHub will retry to re-create Codespace and clients should poll - // the API and attempt to fetch the Codespace for the next two minutes. - if err == api.ErrCreateAsyncRetry { - log.Print("Switching to async provisioning...") - - pollTimeout := 2 * time.Minute - pollInterval := 1 * time.Second - codespace, err = pollForCodespace(ctx, apiClient, log, pollTimeout, pollInterval, userResult.User.Login, codespace.Name) - log.Print("\n") - - if err != nil { - return fmt.Errorf("error creating codespace with async provisioning: %s: %w", codespace.Name, err) - } - } - return fmt.Errorf("error creating codespace: %w", err) } @@ -118,37 +107,6 @@ func create(opts *createOptions) error { return nil } -type apiClient interface { - GetCodespaceToken(context.Context, string, string) (string, error) - GetCodespace(context.Context, string, string, string) (*api.Codespace, error) -} - -// pollForCodespace polls the Codespaces GET endpoint on a given interval for a specified duration. -// If it succeeds at fetching the codespace, we consider the codespace provisioned. -func pollForCodespace(ctx context.Context, client apiClient, log *output.Logger, duration, interval time.Duration, user, name string) (*api.Codespace, error) { - ctx, cancel := context.WithTimeout(ctx, duration) - defer cancel() - - ticker := time.NewTicker(interval) - defer ticker.Stop() - - for { - select { - case <-ctx.Done(): - return nil, ctx.Err() - case <-ticker.C: - log.Print(".") - token, err := client.GetCodespaceToken(ctx, user, name) - if err != nil { - // Do nothing. We expect this to fail until the codespace is provisioned - continue - } - - return client.GetCodespace(ctx, token, user, name) - } - } -} - // showStatus polls the codespace for a list of post create states and their status. It will keep polling // until all states have finished. Once all states have finished, we poll once more to check if any new // states have been introduced and stop polling otherwise. diff --git a/internal/api/api.go b/internal/api/api.go index 394efc6af..ad9177eff 100644 --- a/internal/api/api.go +++ b/internal/api/api.go @@ -431,6 +431,10 @@ func (a *API) CreateCodespace(ctx context.Context, user *User, repository *Repos case resp.StatusCode > http.StatusAccepted: return nil, jsonErrorResponse(b) case resp.StatusCode == http.StatusAccepted: + // When the API returns a 202, it means that the initial creation failed but it is + // being retried. For clients this means that they must implement a polling strategy + // to check for the codespace existence for the next two minutes. We return an error + // here so callers can detect and handle this condition. return nil, ErrCreateAsyncRetry } diff --git a/internal/codespaces/codespaces.go b/internal/codespaces/codespaces.go index 2933c9d8d..c67f88c3b 100644 --- a/internal/codespaces/codespaces.go +++ b/internal/codespaces/codespaces.go @@ -75,3 +75,71 @@ func ConnectToLiveshare(ctx context.Context, log logger, apiClient *api.API, use return lsclient.JoinWorkspace(ctx) } + +type apiClient interface { + CreateCodespace(ctx context.Context, user *api.User, repo *api.Repository, machine, branch, location string) (*api.Codespace, error) + GetCodespaceToken(ctx context.Context, userLogin, codespaceName string) (string, error) + GetCodespace(ctx context.Context, token, userLogin, codespaceName string) (*api.Codespace, error) +} + +// ProvisionParams are the required parameters for provisioning a Codespace. +type ProvisionParams struct { + User *api.User + Repository *api.Repository + Branch, Machine, Location string +} + +// Provision creates a codespace with the given parameters and handles polling in the case +// of initial creation failures. +func Provision(ctx context.Context, log logger, client apiClient, params *ProvisionParams) (*api.Codespace, error) { + codespace, err := client.CreateCodespace( + ctx, params.User, params.Repository, params.Machine, params.Branch, params.Location, + ) + if err != nil { + // This error is returned by the API when the initial creation fails with a retryable error. + // A retryable error means that GitHub will retry to re-create Codespace and clients should poll + // the API and attempt to fetch the Codespace for the next two minutes. + if err == api.ErrCreateAsyncRetry { + log.Print("Switching to async provisioning...") + + pollTimeout := 2 * time.Minute + pollInterval := 1 * time.Second + codespace, err = pollForCodespace(ctx, client, log, pollTimeout, pollInterval, params.User.Login, codespace.Name) + log.Print("\n") + + if err != nil { + return nil, fmt.Errorf("error creating codespace with async provisioning: %s: %w", codespace.Name, err) + } + } + + return nil, err + } + + return codespace, nil +} + +// pollForCodespace polls the Codespaces GET endpoint on a given interval for a specified duration. +// If it succeeds at fetching the codespace, we consider the codespace provisioned. +func pollForCodespace(ctx context.Context, client apiClient, log logger, duration, interval time.Duration, user, name string) (*api.Codespace, error) { + ctx, cancel := context.WithTimeout(ctx, duration) + defer cancel() + + ticker := time.NewTicker(interval) + defer ticker.Stop() + + for { + select { + case <-ctx.Done(): + return nil, ctx.Err() + case <-ticker.C: + log.Print(".") + token, err := client.GetCodespaceToken(ctx, user, name) + if err != nil { + // Do nothing. We expect this to fail until the codespace is provisioned + continue + } + + return client.GetCodespace(ctx, token, user, name) + } + } +} diff --git a/cmd/ghcs/create_test.go b/internal/codespaces/codespaces_test.go similarity index 85% rename from cmd/ghcs/create_test.go rename to internal/codespaces/codespaces_test.go index e86fa00e6..53aba0557 100644 --- a/cmd/ghcs/create_test.go +++ b/internal/codespaces/codespaces_test.go @@ -1,4 +1,4 @@ -package ghcs +package codespaces import ( "context" @@ -12,10 +12,19 @@ import ( ) type mockAPIClient struct { + createCodespace func(context.Context, *api.User, *api.Repository, string, string, string) (*api.Codespace, error) getCodespaceToken func(context.Context, string, string) (string, error) getCodespace func(context.Context, string, string, string) (*api.Codespace, error) } +func (m *mockAPIClient) CreateCodespace(ctx context.Context, user *api.User, repo *api.Repository, machine, branch, location string) (*api.Codespace, error) { + if m.createCodespace == nil { + return nil, errors.New("mock api client CreateCodespace not implemented") + } + + return m.createCodespace(ctx, user, repo, machine, branch, location) +} + func (m *mockAPIClient) GetCodespaceToken(ctx context.Context, userLogin, codespaceName string) (string, error) { if m.getCodespaceToken == nil { return "", errors.New("mock api client GetCodespaceToken not implemented") From 2a0ea1617b3fccd06d29c4e4b81fd6d4b815fa15 Mon Sep 17 00:00:00 2001 From: Jose Garcia Date: Wed, 22 Sep 2021 09:40:45 -0400 Subject: [PATCH 06/16] Handle specific error for GetCodespaceToken --- internal/api/api.go | 7 +++++++ internal/codespaces/codespaces.go | 8 ++++++-- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/internal/api/api.go b/internal/api/api.go index ad9177eff..58d01a428 100644 --- a/internal/api/api.go +++ b/internal/api/api.go @@ -208,6 +208,8 @@ type getCodespaceTokenResponse struct { RepositoryToken string `json:"repository_token"` } +var ErrNotProvisioned = errors.New("codespace not provisioned") + func (a *API) GetCodespaceToken(ctx context.Context, ownerLogin, codespaceName string) (string, error) { reqBody, err := json.Marshal(getCodespaceTokenRequest{true}) if err != nil { @@ -236,6 +238,11 @@ func (a *API) GetCodespaceToken(ctx context.Context, ownerLogin, codespaceName s } if resp.StatusCode != http.StatusOK { + + if resp.StatusCode == http.StatusUnprocessableEntity { + return "", ErrNotProvisioned + } + return "", jsonErrorResponse(b) } diff --git a/internal/codespaces/codespaces.go b/internal/codespaces/codespaces.go index c67f88c3b..1d60bcd21 100644 --- a/internal/codespaces/codespaces.go +++ b/internal/codespaces/codespaces.go @@ -135,8 +135,12 @@ func pollForCodespace(ctx context.Context, client apiClient, log logger, duratio log.Print(".") token, err := client.GetCodespaceToken(ctx, user, name) if err != nil { - // Do nothing. We expect this to fail until the codespace is provisioned - continue + if err == api.ErrNotProvisioned { + // Do nothing. We expect this to fail until the codespace is provisioned + continue + } + + return nil, fmt.Errorf("failed to get codespace token: %w", err) } return client.GetCodespace(ctx, token, user, name) From 8c5330d9e9691289c29bd6130efbca265985023c Mon Sep 17 00:00:00 2001 From: Jose Garcia Date: Wed, 22 Sep 2021 10:04:18 -0400 Subject: [PATCH 07/16] Rename error --- internal/api/api.go | 4 ++-- internal/codespaces/codespaces.go | 5 ++--- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/internal/api/api.go b/internal/api/api.go index 58d01a428..6b19b0703 100644 --- a/internal/api/api.go +++ b/internal/api/api.go @@ -409,7 +409,7 @@ type createCodespaceRequest struct { SkuName string `json:"sku_name"` } -var ErrCreateAsyncRetry = errors.New("initial creation failed, retrying async") +var ErrProvisioningInProgress = errors.New("provisioning in progress") func (a *API) CreateCodespace(ctx context.Context, user *User, repository *Repository, sku, branch, location string) (*Codespace, error) { requestBody, err := json.Marshal(createCodespaceRequest{repository.ID, branch, location, sku}) @@ -442,7 +442,7 @@ func (a *API) CreateCodespace(ctx context.Context, user *User, repository *Repos // being retried. For clients this means that they must implement a polling strategy // to check for the codespace existence for the next two minutes. We return an error // here so callers can detect and handle this condition. - return nil, ErrCreateAsyncRetry + return nil, ErrProvisioningInProgress } var response Codespace diff --git a/internal/codespaces/codespaces.go b/internal/codespaces/codespaces.go index 1d60bcd21..8a0e21b3d 100644 --- a/internal/codespaces/codespaces.go +++ b/internal/codespaces/codespaces.go @@ -99,11 +99,10 @@ func Provision(ctx context.Context, log logger, client apiClient, params *Provis // This error is returned by the API when the initial creation fails with a retryable error. // A retryable error means that GitHub will retry to re-create Codespace and clients should poll // the API and attempt to fetch the Codespace for the next two minutes. - if err == api.ErrCreateAsyncRetry { - log.Print("Switching to async provisioning...") - + if err == api.ErrProvisioningInProgress { pollTimeout := 2 * time.Minute pollInterval := 1 * time.Second + log.Print(".") codespace, err = pollForCodespace(ctx, client, log, pollTimeout, pollInterval, params.User.Login, codespace.Name) log.Print("\n") From d2d21996bc1a12a24f3b757e2fbc2ae933aa8a5e Mon Sep 17 00:00:00 2001 From: Jose Garcia Date: Wed, 22 Sep 2021 11:49:41 -0400 Subject: [PATCH 08/16] Move ProvisionCodespace to API client - Make CreateCodespace private along with its errors --- cmd/ghcs/create.go | 2 +- internal/api/api.go | 82 ++++++++++++++++++- .../codespaces_test.go => api/api_test.go} | 22 ++--- internal/codespaces/codespaces.go | 71 ---------------- 4 files changed, 86 insertions(+), 91 deletions(-) rename internal/{codespaces/codespaces_test.go => api/api_test.go} (77%) diff --git a/cmd/ghcs/create.go b/cmd/ghcs/create.go index 91c8bcb8d..eb6d1bea6 100644 --- a/cmd/ghcs/create.go +++ b/cmd/ghcs/create.go @@ -83,7 +83,7 @@ func create(opts *createOptions) error { log.Println("Creating your codespace...") - codespace, err := codespaces.Provision(ctx, log, apiClient, &codespaces.ProvisionParams{ + codespace, err := apiClient.ProvisionCodespace(ctx, log, &api.ProvisionCodespaceParams{ User: userResult.User, Repository: repository, Branch: branch, diff --git a/internal/api/api.go b/internal/api/api.go index 6b19b0703..c3ad0aadc 100644 --- a/internal/api/api.go +++ b/internal/api/api.go @@ -36,6 +36,7 @@ import ( "net/http" "strconv" "strings" + "time" "github.com/opentracing/opentracing-go" ) @@ -402,6 +403,81 @@ func (a *API) GetCodespacesSKUs(ctx context.Context, user *User, repository *Rep return response.SKUs, nil } +// ProvisionCodespaceParams are the required parameters for provisioning a Codespace. +type ProvisionCodespaceParams struct { + User *User + Repository *Repository + Branch, Machine, Location string +} + +type logger interface { + Print(v ...interface{}) (int, error) + Println(v ...interface{}) (int, error) +} + +// ProvisionCodespace creates a codespace with the given parameters and handles polling in the case +// of initial creation failures. +func (a *API) ProvisionCodespace(ctx context.Context, log logger, params *ProvisionCodespaceParams) (*Codespace, error) { + codespace, err := a.createCodespace( + ctx, params.User, params.Repository, params.Machine, params.Branch, params.Location, + ) + if err != nil { + // This error is returned by the API when the initial creation fails with a retryable error. + // A retryable error means that GitHub will retry to re-create Codespace and clients should poll + // the API and attempt to fetch the Codespace for the next two minutes. + if err == errProvisioningInProgress { + pollTimeout := 2 * time.Minute + pollInterval := 1 * time.Second + log.Print(".") + codespace, err = pollForCodespace(ctx, a, log, pollTimeout, pollInterval, params.User.Login, codespace.Name) + log.Print("\n") + + if err != nil { + return nil, fmt.Errorf("error creating codespace with async provisioning: %s: %w", codespace.Name, err) + } + } + + return nil, err + } + + return codespace, nil +} + +type apiClient interface { + GetCodespaceToken(ctx context.Context, userLogin, codespaceName string) (string, error) + GetCodespace(ctx context.Context, token, userLogin, codespaceName string) (*Codespace, error) +} + +// pollForCodespace polls the Codespaces GET endpoint on a given interval for a specified duration. +// If it succeeds at fetching the codespace, we consider the codespace provisioned. +func pollForCodespace(ctx context.Context, client apiClient, log logger, duration, interval time.Duration, user, name string) (*Codespace, error) { + ctx, cancel := context.WithTimeout(ctx, duration) + defer cancel() + + ticker := time.NewTicker(interval) + defer ticker.Stop() + + for { + select { + case <-ctx.Done(): + return nil, ctx.Err() + case <-ticker.C: + log.Print(".") + token, err := client.GetCodespaceToken(ctx, user, name) + if err != nil { + if err == ErrNotProvisioned { + // Do nothing. We expect this to fail until the codespace is provisioned + continue + } + + return nil, fmt.Errorf("failed to get codespace token: %w", err) + } + + return client.GetCodespace(ctx, token, user, name) + } + } +} + type createCodespaceRequest struct { RepositoryID int `json:"repository_id"` Ref string `json:"ref"` @@ -409,9 +485,9 @@ type createCodespaceRequest struct { SkuName string `json:"sku_name"` } -var ErrProvisioningInProgress = errors.New("provisioning in progress") +var errProvisioningInProgress = errors.New("provisioning in progress") -func (a *API) CreateCodespace(ctx context.Context, user *User, repository *Repository, sku, branch, location string) (*Codespace, error) { +func (a *API) createCodespace(ctx context.Context, user *User, repository *Repository, sku, branch, location string) (*Codespace, error) { requestBody, err := json.Marshal(createCodespaceRequest{repository.ID, branch, location, sku}) if err != nil { return nil, fmt.Errorf("error marshaling request: %w", err) @@ -442,7 +518,7 @@ func (a *API) CreateCodespace(ctx context.Context, user *User, repository *Repos // being retried. For clients this means that they must implement a polling strategy // to check for the codespace existence for the next two minutes. We return an error // here so callers can detect and handle this condition. - return nil, ErrProvisioningInProgress + return nil, errProvisioningInProgress } var response Codespace diff --git a/internal/codespaces/codespaces_test.go b/internal/api/api_test.go similarity index 77% rename from internal/codespaces/codespaces_test.go rename to internal/api/api_test.go index 53aba0557..eb5226a59 100644 --- a/internal/codespaces/codespaces_test.go +++ b/internal/api/api_test.go @@ -1,4 +1,4 @@ -package codespaces +package api import ( "context" @@ -8,21 +8,11 @@ import ( "time" "github.com/github/ghcs/cmd/ghcs/output" - "github.com/github/ghcs/internal/api" ) type mockAPIClient struct { - createCodespace func(context.Context, *api.User, *api.Repository, string, string, string) (*api.Codespace, error) getCodespaceToken func(context.Context, string, string) (string, error) - getCodespace func(context.Context, string, string, string) (*api.Codespace, error) -} - -func (m *mockAPIClient) CreateCodespace(ctx context.Context, user *api.User, repo *api.Repository, machine, branch, location string) (*api.Codespace, error) { - if m.createCodespace == nil { - return nil, errors.New("mock api client CreateCodespace not implemented") - } - - return m.createCodespace(ctx, user, repo, machine, branch, location) + getCodespace func(context.Context, string, string, string) (*Codespace, error) } func (m *mockAPIClient) GetCodespaceToken(ctx context.Context, userLogin, codespaceName string) (string, error) { @@ -33,7 +23,7 @@ func (m *mockAPIClient) GetCodespaceToken(ctx context.Context, userLogin, codesp return m.getCodespaceToken(ctx, userLogin, codespaceName) } -func (m *mockAPIClient) GetCodespace(ctx context.Context, token, userLogin, codespaceName string) (*api.Codespace, error) { +func (m *mockAPIClient) GetCodespace(ctx context.Context, token, userLogin, codespaceName string) (*Codespace, error) { if m.getCodespace == nil { return nil, errors.New("mock api client GetCodespace not implemented") } @@ -43,8 +33,8 @@ func (m *mockAPIClient) GetCodespace(ctx context.Context, token, userLogin, code func TestPollForCodespace(t *testing.T) { logger := output.NewLogger(nil, nil, false) - user := &api.User{Login: "test"} - tmpCodespace := &api.Codespace{Name: "tmp-codespace"} + user := &User{Login: "test"} + tmpCodespace := &Codespace{Name: "tmp-codespace"} codespaceToken := "codespace-token" ctx := context.Background() @@ -61,7 +51,7 @@ func TestPollForCodespace(t *testing.T) { } return codespaceToken, nil }, - getCodespace: func(ctx context.Context, token, userLogin, codespace string) (*api.Codespace, error) { + getCodespace: func(ctx context.Context, token, userLogin, codespace string) (*Codespace, error) { if token != codespaceToken { return nil, fmt.Errorf("token does not match, got: %s, expected: %s", token, codespaceToken) } diff --git a/internal/codespaces/codespaces.go b/internal/codespaces/codespaces.go index 8a0e21b3d..2933c9d8d 100644 --- a/internal/codespaces/codespaces.go +++ b/internal/codespaces/codespaces.go @@ -75,74 +75,3 @@ func ConnectToLiveshare(ctx context.Context, log logger, apiClient *api.API, use return lsclient.JoinWorkspace(ctx) } - -type apiClient interface { - CreateCodespace(ctx context.Context, user *api.User, repo *api.Repository, machine, branch, location string) (*api.Codespace, error) - GetCodespaceToken(ctx context.Context, userLogin, codespaceName string) (string, error) - GetCodespace(ctx context.Context, token, userLogin, codespaceName string) (*api.Codespace, error) -} - -// ProvisionParams are the required parameters for provisioning a Codespace. -type ProvisionParams struct { - User *api.User - Repository *api.Repository - Branch, Machine, Location string -} - -// Provision creates a codespace with the given parameters and handles polling in the case -// of initial creation failures. -func Provision(ctx context.Context, log logger, client apiClient, params *ProvisionParams) (*api.Codespace, error) { - codespace, err := client.CreateCodespace( - ctx, params.User, params.Repository, params.Machine, params.Branch, params.Location, - ) - if err != nil { - // This error is returned by the API when the initial creation fails with a retryable error. - // A retryable error means that GitHub will retry to re-create Codespace and clients should poll - // the API and attempt to fetch the Codespace for the next two minutes. - if err == api.ErrProvisioningInProgress { - pollTimeout := 2 * time.Minute - pollInterval := 1 * time.Second - log.Print(".") - codespace, err = pollForCodespace(ctx, client, log, pollTimeout, pollInterval, params.User.Login, codespace.Name) - log.Print("\n") - - if err != nil { - return nil, fmt.Errorf("error creating codespace with async provisioning: %s: %w", codespace.Name, err) - } - } - - return nil, err - } - - return codespace, nil -} - -// pollForCodespace polls the Codespaces GET endpoint on a given interval for a specified duration. -// If it succeeds at fetching the codespace, we consider the codespace provisioned. -func pollForCodespace(ctx context.Context, client apiClient, log logger, duration, interval time.Duration, user, name string) (*api.Codespace, error) { - ctx, cancel := context.WithTimeout(ctx, duration) - defer cancel() - - ticker := time.NewTicker(interval) - defer ticker.Stop() - - for { - select { - case <-ctx.Done(): - return nil, ctx.Err() - case <-ticker.C: - log.Print(".") - token, err := client.GetCodespaceToken(ctx, user, name) - if err != nil { - if err == api.ErrNotProvisioned { - // Do nothing. We expect this to fail until the codespace is provisioned - continue - } - - return nil, fmt.Errorf("failed to get codespace token: %w", err) - } - - return client.GetCodespace(ctx, token, user, name) - } - } -} From 70a2ea2e6aaf36cd8e9206adab640892c7892d0d Mon Sep 17 00:00:00 2001 From: Jose Garcia Date: Wed, 22 Sep 2021 13:19:26 -0400 Subject: [PATCH 09/16] PR Feedback - Rename ProvisionCodespace -> CreateCodespace - Rename createCodespace -> startCreate - Additional docs/comments - Simplify ProvisionCodespaceParams --- cmd/ghcs/create.go | 12 ++++++------ internal/api/api.go | 39 +++++++++++++++++++-------------------- 2 files changed, 25 insertions(+), 26 deletions(-) diff --git a/cmd/ghcs/create.go b/cmd/ghcs/create.go index eb6d1bea6..c0943549c 100644 --- a/cmd/ghcs/create.go +++ b/cmd/ghcs/create.go @@ -83,12 +83,12 @@ func create(opts *createOptions) error { log.Println("Creating your codespace...") - codespace, err := apiClient.ProvisionCodespace(ctx, log, &api.ProvisionCodespaceParams{ - User: userResult.User, - Repository: repository, - Branch: branch, - Machine: machine, - Location: locationResult.Location, + codespace, err := apiClient.CreateCodespace(ctx, log, &api.ProvisionCodespaceParams{ + User: userResult.User, + RepositoryID: repository, + Branch: branch, + Machine: machine, + Location: locationResult.Location, }) if err != nil { return fmt.Errorf("error creating codespace: %w", err) diff --git a/internal/api/api.go b/internal/api/api.go index c3ad0aadc..a1e580e0f 100644 --- a/internal/api/api.go +++ b/internal/api/api.go @@ -239,7 +239,6 @@ func (a *API) GetCodespaceToken(ctx context.Context, ownerLogin, codespaceName s } if resp.StatusCode != http.StatusOK { - if resp.StatusCode == http.StatusUnprocessableEntity { return "", ErrNotProvisioned } @@ -405,8 +404,8 @@ func (a *API) GetCodespacesSKUs(ctx context.Context, user *User, repository *Rep // ProvisionCodespaceParams are the required parameters for provisioning a Codespace. type ProvisionCodespaceParams struct { - User *User - Repository *Repository + User string + RepositoryID int Branch, Machine, Location string } @@ -415,21 +414,21 @@ type logger interface { Println(v ...interface{}) (int, error) } -// ProvisionCodespace creates a codespace with the given parameters and handles polling in the case -// of initial creation failures. -func (a *API) ProvisionCodespace(ctx context.Context, log logger, params *ProvisionCodespaceParams) (*Codespace, error) { - codespace, err := a.createCodespace( - ctx, params.User, params.Repository, params.Machine, params.Branch, params.Location, +// CreateCodespace creates a codespace with the given parameters and returns a non-nil error if it +// fails to create. +func (a *API) CreateCodespace(ctx context.Context, log logger, params *ProvisionCodespaceParams) (*Codespace, error) { + codespace, err := a.startCreate( + ctx, params.User, params.RepositoryID, params.Machine, params.Branch, params.Location, ) if err != nil { - // This error is returned by the API when the initial creation fails with a retryable error. - // A retryable error means that GitHub will retry to re-create Codespace and clients should poll - // the API and attempt to fetch the Codespace for the next two minutes. + // errProvisioningInProgress indicates that codespace creation did not complete + // within the GitHub API RPC time limit (10s), so it continues asynchronously. + // We must poll the server to discover the outcome. if err == errProvisioningInProgress { pollTimeout := 2 * time.Minute pollInterval := 1 * time.Second log.Print(".") - codespace, err = pollForCodespace(ctx, a, log, pollTimeout, pollInterval, params.User.Login, codespace.Name) + codespace, err = pollForCodespace(ctx, a, log, pollTimeout, pollInterval, params.User, codespace.Name) log.Print("\n") if err != nil { @@ -487,13 +486,17 @@ type createCodespaceRequest struct { var errProvisioningInProgress = errors.New("provisioning in progress") -func (a *API) createCodespace(ctx context.Context, user *User, repository *Repository, sku, branch, location string) (*Codespace, error) { - requestBody, err := json.Marshal(createCodespaceRequest{repository.ID, branch, location, sku}) +// startCreate starts the creation of a codespace. +// It may return success or an error, or errProvisioningInProgress indicating that the operation +// did not complete before the GitHub API's time limit for RPCs (10s), in which case the caller +// must poll the server to learn the outcome. +func (a *API) startCreate(ctx context.Context, user string, repository int, sku, branch, location string) (*Codespace, error) { + requestBody, err := json.Marshal(createCodespaceRequest{repository, branch, location, sku}) if err != nil { return nil, fmt.Errorf("error marshaling request: %w", err) } - req, err := http.NewRequest(http.MethodPost, githubAPI+"/vscs_internal/user/"+user.Login+"/codespaces", bytes.NewBuffer(requestBody)) + req, err := http.NewRequest(http.MethodPost, githubAPI+"/vscs_internal/user/"+user+"/codespaces", bytes.NewBuffer(requestBody)) if err != nil { return nil, fmt.Errorf("error creating request: %w", err) } @@ -514,11 +517,7 @@ func (a *API) createCodespace(ctx context.Context, user *User, repository *Repos case resp.StatusCode > http.StatusAccepted: return nil, jsonErrorResponse(b) case resp.StatusCode == http.StatusAccepted: - // When the API returns a 202, it means that the initial creation failed but it is - // being retried. For clients this means that they must implement a polling strategy - // to check for the codespace existence for the next two minutes. We return an error - // here so callers can detect and handle this condition. - return nil, errProvisioningInProgress + return nil, errProvisioningInProgress // RPC finished before result of creation known } var response Codespace From 208f1721b5a29834c9f6420b765b95dd41ce7020 Mon Sep 17 00:00:00 2001 From: Jose Garcia Date: Wed, 22 Sep 2021 13:21:02 -0400 Subject: [PATCH 10/16] Rename ProvisionCodespaceParams --- cmd/ghcs/create.go | 6 +++--- internal/api/api.go | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/cmd/ghcs/create.go b/cmd/ghcs/create.go index c0943549c..e37c1a200 100644 --- a/cmd/ghcs/create.go +++ b/cmd/ghcs/create.go @@ -83,9 +83,9 @@ func create(opts *createOptions) error { log.Println("Creating your codespace...") - codespace, err := apiClient.CreateCodespace(ctx, log, &api.ProvisionCodespaceParams{ - User: userResult.User, - RepositoryID: repository, + codespace, err := apiClient.CreateCodespace(ctx, log, &api.CreateCodespaceParams{ + User: userResult.User.Login, + RepositoryID: repository.ID, Branch: branch, Machine: machine, Location: locationResult.Location, diff --git a/internal/api/api.go b/internal/api/api.go index a1e580e0f..273c64435 100644 --- a/internal/api/api.go +++ b/internal/api/api.go @@ -402,8 +402,8 @@ func (a *API) GetCodespacesSKUs(ctx context.Context, user *User, repository *Rep return response.SKUs, nil } -// ProvisionCodespaceParams are the required parameters for provisioning a Codespace. -type ProvisionCodespaceParams struct { +// CreateCodespaceParams are the required parameters for provisioning a Codespace. +type CreateCodespaceParams struct { User string RepositoryID int Branch, Machine, Location string @@ -416,7 +416,7 @@ type logger interface { // CreateCodespace creates a codespace with the given parameters and returns a non-nil error if it // fails to create. -func (a *API) CreateCodespace(ctx context.Context, log logger, params *ProvisionCodespaceParams) (*Codespace, error) { +func (a *API) CreateCodespace(ctx context.Context, log logger, params *CreateCodespaceParams) (*Codespace, error) { codespace, err := a.startCreate( ctx, params.User, params.RepositoryID, params.Machine, params.Branch, params.Location, ) From 9a558bc58c0d6d2c9a50f6123242ba0e9bec1257 Mon Sep 17 00:00:00 2001 From: Jose Garcia Date: Wed, 22 Sep 2021 15:03:12 -0400 Subject: [PATCH 11/16] Early return if polling is not required - Add context to errors in poller --- cmd/ghcs/create.go | 4 ++-- internal/api/api.go | 36 +++++++++++++++++------------------- 2 files changed, 19 insertions(+), 21 deletions(-) diff --git a/cmd/ghcs/create.go b/cmd/ghcs/create.go index e37c1a200..0dffd5710 100644 --- a/cmd/ghcs/create.go +++ b/cmd/ghcs/create.go @@ -81,8 +81,7 @@ func create(opts *createOptions) error { return errors.New("there are no available machine types for this repository") } - log.Println("Creating your codespace...") - + log.Print("Creating your codespace...") codespace, err := apiClient.CreateCodespace(ctx, log, &api.CreateCodespaceParams{ User: userResult.User.Login, RepositoryID: repository.ID, @@ -90,6 +89,7 @@ func create(opts *createOptions) error { Machine: machine, Location: locationResult.Location, }) + log.Print("\n") if err != nil { return fmt.Errorf("error creating codespace: %w", err) } diff --git a/internal/api/api.go b/internal/api/api.go index 273c64435..eac2c3a88 100644 --- a/internal/api/api.go +++ b/internal/api/api.go @@ -209,6 +209,8 @@ type getCodespaceTokenResponse struct { RepositoryToken string `json:"repository_token"` } +// ErrNotProvisioned is returned by GetCodespacesToken to indicate that the +// creation of a codespace is not yet complete and that the caller should try again. var ErrNotProvisioned = errors.New("codespace not provisioned") func (a *API) GetCodespaceToken(ctx context.Context, ownerLogin, codespaceName string) (string, error) { @@ -420,26 +422,17 @@ func (a *API) CreateCodespace(ctx context.Context, log logger, params *CreateCod codespace, err := a.startCreate( ctx, params.User, params.RepositoryID, params.Machine, params.Branch, params.Location, ) - if err != nil { - // errProvisioningInProgress indicates that codespace creation did not complete - // within the GitHub API RPC time limit (10s), so it continues asynchronously. - // We must poll the server to discover the outcome. - if err == errProvisioningInProgress { - pollTimeout := 2 * time.Minute - pollInterval := 1 * time.Second - log.Print(".") - codespace, err = pollForCodespace(ctx, a, log, pollTimeout, pollInterval, params.User, codespace.Name) - log.Print("\n") - - if err != nil { - return nil, fmt.Errorf("error creating codespace with async provisioning: %s: %w", codespace.Name, err) - } - } - - return nil, err + if err != errProvisioningInProgress { + return codespace, err } - return codespace, nil + // errProvisioningInProgress indicates that codespace creation did not complete + // within the GitHub API RPC time limit (10s), so it continues asynchronously. + // We must poll the server to discover the outcome. + pollTimeout := 2 * time.Minute + pollInterval := 1 * time.Second + + return pollForCodespace(ctx, a, log, pollTimeout, pollInterval, params.User, codespace.Name) } type apiClient interface { @@ -472,7 +465,12 @@ func pollForCodespace(ctx context.Context, client apiClient, log logger, duratio return nil, fmt.Errorf("failed to get codespace token: %w", err) } - return client.GetCodespace(ctx, token, user, name) + codespace, err := client.GetCodespace(ctx, token, user, name) + if err != nil { + return nil, fmt.Errorf("failed to get codespace: %w", err) + } + + return codespace, nil } } } From 4e0ac15fe045012a2398690fefefff66c86d43a7 Mon Sep 17 00:00:00 2001 From: Jose Garcia Date: Wed, 22 Sep 2021 15:10:47 -0400 Subject: [PATCH 12/16] Add buffer to channels to avoid goroutine leak --- cmd/ghcs/create.go | 4 ++-- cmd/ghcs/ports.go | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/cmd/ghcs/create.go b/cmd/ghcs/create.go index 45aa794e6..b52690d1d 100644 --- a/cmd/ghcs/create.go +++ b/cmd/ghcs/create.go @@ -172,7 +172,7 @@ type getUserResult struct { // getUser fetches the user record associated with the GITHUB_TOKEN func getUser(ctx context.Context, apiClient *api.API) <-chan getUserResult { - ch := make(chan getUserResult) + ch := make(chan getUserResult, 1) go func() { user, err := apiClient.GetUser(ctx) ch <- getUserResult{user, err} @@ -187,7 +187,7 @@ type locationResult struct { // getLocation fetches the closest Codespace datacenter region/location to the user. func getLocation(ctx context.Context, apiClient *api.API) <-chan locationResult { - ch := make(chan locationResult) + ch := make(chan locationResult, 1) go func() { location, err := apiClient.GetCodespaceRegionLocation(ctx) ch <- locationResult{location, err} diff --git a/cmd/ghcs/ports.go b/cmd/ghcs/ports.go index aeecf0a07..8a4f855fa 100644 --- a/cmd/ghcs/ports.go +++ b/cmd/ghcs/ports.go @@ -123,7 +123,7 @@ type portAttribute struct { } func getDevContainer(ctx context.Context, apiClient *api.API, codespace *api.Codespace) <-chan devContainerResult { - ch := make(chan devContainerResult) + ch := make(chan devContainerResult, 1) go func() { contents, err := apiClient.GetCodespaceRepositoryContents(ctx, codespace, ".devcontainer/devcontainer.json") if err != nil { From 13d7804a359f8062817ec1e1da183da1e08a927a Mon Sep 17 00:00:00 2001 From: Jose Garcia Date: Thu, 23 Sep 2021 08:26:23 -0400 Subject: [PATCH 13/16] Remove API test, inline poller --- internal/api/api.go | 22 ++--------- internal/api/api_test.go | 82 ---------------------------------------- 2 files changed, 4 insertions(+), 100 deletions(-) delete mode 100644 internal/api/api_test.go diff --git a/internal/api/api.go b/internal/api/api.go index eac2c3a88..50c4a03de 100644 --- a/internal/api/api.go +++ b/internal/api/api.go @@ -429,24 +429,10 @@ func (a *API) CreateCodespace(ctx context.Context, log logger, params *CreateCod // errProvisioningInProgress indicates that codespace creation did not complete // within the GitHub API RPC time limit (10s), so it continues asynchronously. // We must poll the server to discover the outcome. - pollTimeout := 2 * time.Minute - pollInterval := 1 * time.Second - - return pollForCodespace(ctx, a, log, pollTimeout, pollInterval, params.User, codespace.Name) -} - -type apiClient interface { - GetCodespaceToken(ctx context.Context, userLogin, codespaceName string) (string, error) - GetCodespace(ctx context.Context, token, userLogin, codespaceName string) (*Codespace, error) -} - -// pollForCodespace polls the Codespaces GET endpoint on a given interval for a specified duration. -// If it succeeds at fetching the codespace, we consider the codespace provisioned. -func pollForCodespace(ctx context.Context, client apiClient, log logger, duration, interval time.Duration, user, name string) (*Codespace, error) { - ctx, cancel := context.WithTimeout(ctx, duration) + ctx, cancel := context.WithTimeout(ctx, 2*time.Minute) defer cancel() - ticker := time.NewTicker(interval) + ticker := time.NewTicker(1 * time.Second) defer ticker.Stop() for { @@ -455,7 +441,7 @@ func pollForCodespace(ctx context.Context, client apiClient, log logger, duratio return nil, ctx.Err() case <-ticker.C: log.Print(".") - token, err := client.GetCodespaceToken(ctx, user, name) + token, err := a.GetCodespaceToken(ctx, params.User, codespace.Name) if err != nil { if err == ErrNotProvisioned { // Do nothing. We expect this to fail until the codespace is provisioned @@ -465,7 +451,7 @@ func pollForCodespace(ctx context.Context, client apiClient, log logger, duratio return nil, fmt.Errorf("failed to get codespace token: %w", err) } - codespace, err := client.GetCodespace(ctx, token, user, name) + codespace, err = a.GetCodespace(ctx, token, params.User, codespace.Name) if err != nil { return nil, fmt.Errorf("failed to get codespace: %w", err) } diff --git a/internal/api/api_test.go b/internal/api/api_test.go deleted file mode 100644 index eb5226a59..000000000 --- a/internal/api/api_test.go +++ /dev/null @@ -1,82 +0,0 @@ -package api - -import ( - "context" - "errors" - "fmt" - "testing" - "time" - - "github.com/github/ghcs/cmd/ghcs/output" -) - -type mockAPIClient struct { - getCodespaceToken func(context.Context, string, string) (string, error) - getCodespace func(context.Context, string, string, string) (*Codespace, error) -} - -func (m *mockAPIClient) GetCodespaceToken(ctx context.Context, userLogin, codespaceName string) (string, error) { - if m.getCodespaceToken == nil { - return "", errors.New("mock api client GetCodespaceToken not implemented") - } - - return m.getCodespaceToken(ctx, userLogin, codespaceName) -} - -func (m *mockAPIClient) GetCodespace(ctx context.Context, token, userLogin, codespaceName string) (*Codespace, error) { - if m.getCodespace == nil { - return nil, errors.New("mock api client GetCodespace not implemented") - } - - return m.getCodespace(ctx, token, userLogin, codespaceName) -} - -func TestPollForCodespace(t *testing.T) { - logger := output.NewLogger(nil, nil, false) - user := &User{Login: "test"} - tmpCodespace := &Codespace{Name: "tmp-codespace"} - codespaceToken := "codespace-token" - ctx := context.Background() - - pollInterval := 50 * time.Millisecond - pollTimeout := 100 * time.Millisecond - - api := &mockAPIClient{ - getCodespaceToken: func(ctx context.Context, userLogin, codespace string) (string, error) { - if userLogin != user.Login { - return "", fmt.Errorf("user does not match, got: %s, expected: %s", userLogin, user.Login) - } - if codespace != tmpCodespace.Name { - return "", fmt.Errorf("codespace does not match, got: %s, expected: %s", codespace, tmpCodespace.Name) - } - return codespaceToken, nil - }, - getCodespace: func(ctx context.Context, token, userLogin, codespace string) (*Codespace, error) { - if token != codespaceToken { - return nil, fmt.Errorf("token does not match, got: %s, expected: %s", token, codespaceToken) - } - if userLogin != user.Login { - return nil, fmt.Errorf("user does not match, got: %s, expected: %s", userLogin, user.Login) - } - if codespace != tmpCodespace.Name { - return nil, fmt.Errorf("codespace does not match, got: %s, expected: %s", codespace, tmpCodespace.Name) - } - return tmpCodespace, nil - }, - } - - codespace, err := pollForCodespace(ctx, api, logger, pollTimeout, pollInterval, user.Login, tmpCodespace.Name) - if err != nil { - t.Error(err) - } - if tmpCodespace.Name != codespace.Name { - t.Errorf("returned codespace does not match, got: %s, expected: %s", codespace.Name, tmpCodespace.Name) - } - - // swap the durations to trigger a timeout - pollTimeout, pollInterval = pollInterval, pollTimeout - _, err = pollForCodespace(ctx, api, logger, pollTimeout, pollInterval, user.Login, tmpCodespace.Name) - if err != context.DeadlineExceeded { - t.Error("expected context deadline exceeded error, got nil") - } -} From 186b90b12e4d253d091a265714965bc96284c78f Mon Sep 17 00:00:00 2001 From: Jose Garcia Date: Thu, 23 Sep 2021 08:29:24 -0400 Subject: [PATCH 14/16] Rename request type --- internal/api/api.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/api/api.go b/internal/api/api.go index 50c4a03de..fdf5c5b55 100644 --- a/internal/api/api.go +++ b/internal/api/api.go @@ -461,7 +461,7 @@ func (a *API) CreateCodespace(ctx context.Context, log logger, params *CreateCod } } -type createCodespaceRequest struct { +type startCreateRequest struct { RepositoryID int `json:"repository_id"` Ref string `json:"ref"` Location string `json:"location"` @@ -475,7 +475,7 @@ var errProvisioningInProgress = errors.New("provisioning in progress") // did not complete before the GitHub API's time limit for RPCs (10s), in which case the caller // must poll the server to learn the outcome. func (a *API) startCreate(ctx context.Context, user string, repository int, sku, branch, location string) (*Codespace, error) { - requestBody, err := json.Marshal(createCodespaceRequest{repository, branch, location, sku}) + requestBody, err := json.Marshal(startCreateRequest{repository, branch, location, sku}) if err != nil { return nil, fmt.Errorf("error marshaling request: %w", err) } From 9654dc4bd3711ed6ec00a112355313827cfe95bf Mon Sep 17 00:00:00 2001 From: Jose Garcia Date: Thu, 23 Sep 2021 10:07:14 -0400 Subject: [PATCH 15/16] Update to go-liveshare v0.20.0 --- internal/codespaces/codespaces.go | 19 ++++++------------- 1 file changed, 6 insertions(+), 13 deletions(-) diff --git a/internal/codespaces/codespaces.go b/internal/codespaces/codespaces.go index 2933c9d8d..7b27b817e 100644 --- a/internal/codespaces/codespaces.go +++ b/internal/codespaces/codespaces.go @@ -61,17 +61,10 @@ func ConnectToLiveshare(ctx context.Context, log logger, apiClient *api.API, use log.Println("Connecting to your codespace...") - lsclient, err := liveshare.NewClient( - liveshare.WithConnection(liveshare.Connection{ - SessionID: codespace.Environment.Connection.SessionID, - SessionToken: codespace.Environment.Connection.SessionToken, - RelaySAS: codespace.Environment.Connection.RelaySAS, - RelayEndpoint: codespace.Environment.Connection.RelayEndpoint, - }), - ) - if err != nil { - return nil, fmt.Errorf("error creating Live Share client: %w", err) - } - - return lsclient.JoinWorkspace(ctx) + return liveshare.Connect(ctx, liveshare.Options{ + SessionID: codespace.Environment.Connection.SessionID, + SessionToken: codespace.Environment.Connection.SessionToken, + RelaySAS: codespace.Environment.Connection.RelaySAS, + RelayEndpoint: codespace.Environment.Connection.RelayEndpoint, + }) } From f1c35ba9daa06996205082f05a275ce97aa68297 Mon Sep 17 00:00:00 2001 From: Jose Garcia Date: Thu, 23 Sep 2021 10:21:01 -0400 Subject: [PATCH 16/16] Update docs --- internal/codespaces/codespaces.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/codespaces/codespaces.go b/internal/codespaces/codespaces.go index 7b27b817e..43809bab9 100644 --- a/internal/codespaces/codespaces.go +++ b/internal/codespaces/codespaces.go @@ -23,8 +23,8 @@ func connectionReady(codespace *api.Codespace) bool { codespace.Environment.State == api.CodespaceEnvironmentStateAvailable } -// ConnectToLiveshare creates a Live Share client and joins the Live Share session. -// It will start the Codespace if it is not already running, it will time out after 60 seconds if fails to start. +// ConnectToLiveshare waits for a Codespace to become running, +// and connects to it using a Live Share session. func ConnectToLiveshare(ctx context.Context, log logger, apiClient *api.API, userLogin, token string, codespace *api.Codespace) (*liveshare.Session, error) { var startedCodespace bool if codespace.Environment.State != api.CodespaceEnvironmentStateAvailable {