Skip to content

Commit 48e3473

Browse files
josebaliusjosebalius
committed
PR Feedback
- Bring context.Timeout into the poller - Accept duration and interval - Other tidy up
1 parent 861811b commit 48e3473

File tree

2 files changed

+20
-37
lines changed

2 files changed

+20
-37
lines changed

‎cmd/ghcs/create.go‎

Lines changed: 12 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -84,19 +84,17 @@ func create(opts *createOptions) error {
8484

8585
log.Println("Creating your codespace...")
8686

87-
codespace, err := apiClient.CreateCodespace(
88-
ctx, userResult.User, repository, machine, branch, locationResult.Location,
89-
)
87+
codespace, err := apiClient.CreateCodespace(ctx, userResult.User, repository, machine, branch, locationResult.Location)
9088
if err != nil {
9189
// This error is returned by the API when the initial creation fails with a retryable error.
9290
// A retryable error means that GitHub will retry to re-create Codespace and clients should poll
9391
// the API and attempt to fetch the Codespace for the next two minutes.
9492
if err == api.ErrCreateAsyncRetry {
9593
log.Print("Switching to async provisioning...")
96-
pollctx, cancel := context.WithTimeout(ctx, 2*time.Minute)
97-
defer cancel()
9894

99-
codespace, err = pollForCodespace(pollctx, apiClient, log, userResult.User, codespace)
95+
pollTimeout := 2 * time.Minute
96+
pollInterval := 1 * time.Second
97+
codespace, err = pollForCodespace(ctx, apiClient, log, pollTimeout, pollInterval, userResult.User.Login, codespace.Name)
10098
log.Print("\n")
10199

102100
if err != nil {
@@ -125,13 +123,13 @@ type apiClient interface {
125123
GetCodespace(context.Context, string, string, string) (*api.Codespace, error)
126124
}
127125

128-
// pollForCodespace polls the Codespaces API every second fetching the codespace.
126+
// pollForCodespace polls the Codespaces GET endpoint on a given interval for a specified duration.
129127
// If it succeeds at fetching the codespace, we consider the codespace provisioned.
130-
// Context should be cancelled to stop polling.
131-
func pollForCodespace(
132-
ctx context.Context, client apiClient, log *output.Logger, user *api.User, provisioningCodespace *api.Codespace,
133-
) (*api.Codespace, error) {
134-
ticker := time.NewTicker(1 * time.Second)
128+
func pollForCodespace(ctx context.Context, client apiClient, log *output.Logger, duration, interval time.Duration, user, name string) (*api.Codespace, error) {
129+
ctx, cancel := context.WithTimeout(ctx, duration)
130+
defer cancel()
131+
132+
ticker := time.NewTicker(interval)
135133
defer ticker.Stop()
136134

137135
for {
@@ -140,18 +138,13 @@ func pollForCodespace(
140138
return nil, ctx.Err()
141139
case <-ticker.C:
142140
log.Print(".")
143-
token, err := client.GetCodespaceToken(ctx, user.Login, provisioningCodespace.Name)
141+
token, err := client.GetCodespaceToken(ctx, user, name)
144142
if err != nil {
145143
// Do nothing. We expect this to fail until the codespace is provisioned
146144
continue
147145
}
148146

149-
codespace, err := client.GetCodespace(ctx, token, user.Login, provisioningCodespace.Name)
150-
if err != nil {
151-
return nil, fmt.Errorf("failed to get codespace: %w", err)
152-
}
153-
154-
return codespace, nil
147+
return client.GetCodespace(ctx, token, user, name)
155148
}
156149
}
157150
}

‎cmd/ghcs/create_test.go‎

Lines changed: 8 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -37,18 +37,13 @@ func TestPollForCodespace(t *testing.T) {
3737
user := &api.User{Login: "test"}
3838
tmpCodespace := &api.Codespace{Name: "tmp-codespace"}
3939
codespaceToken := "codespace-token"
40+
ctx := context.Background()
4041

41-
ctxTimeout := 1 * time.Second
42-
exceedTime := 2 * time.Second
43-
exceedProvisioningTime := false
42+
pollInterval := 50 * time.Millisecond
43+
pollTimeout := 100 * time.Millisecond
4444

4545
api := &mockAPIClient{
4646
getCodespaceToken: func(ctx context.Context, userLogin, codespace string) (string, error) {
47-
if exceedProvisioningTime {
48-
ticker := time.NewTicker(exceedTime)
49-
defer ticker.Stop()
50-
<-ticker.C
51-
}
5247
if userLogin != user.Login {
5348
return "", fmt.Errorf("user does not match, got: %s, expected: %s", userLogin, user.Login)
5449
}
@@ -71,23 +66,18 @@ func TestPollForCodespace(t *testing.T) {
7166
},
7267
}
7368

74-
ctx, cancel := context.WithTimeout(context.Background(), ctxTimeout)
75-
defer cancel()
76-
77-
codespace, err := pollForCodespace(ctx, api, logger, user, tmpCodespace)
69+
codespace, err := pollForCodespace(ctx, api, logger, pollTimeout, pollInterval, user.Login, tmpCodespace.Name)
7870
if err != nil {
7971
t.Error(err)
8072
}
8173
if tmpCodespace.Name != codespace.Name {
8274
t.Errorf("returned codespace does not match, got: %s, expected: %s", codespace.Name, tmpCodespace.Name)
8375
}
8476

85-
exceedProvisioningTime = true
86-
ctx, cancel = context.WithTimeout(ctx, ctxTimeout)
87-
defer cancel()
88-
89-
_, err = pollForCodespace(ctx, api, logger, user, tmpCodespace)
90-
if err == nil {
77+
// swap the durations to trigger a timeout
78+
pollTimeout, pollInterval = pollInterval, pollTimeout
79+
_, err = pollForCodespace(ctx, api, logger, pollTimeout, pollInterval, user.Login, tmpCodespace.Name)
80+
if err != context.DeadlineExceeded {
9181
t.Error("expected context deadline exceeded error, got nil")
9282
}
9383
}

0 commit comments

Comments
 (0)