Skip to content

Commit 2d889e8

Browse files
k0ralk0ral
authored
Allow up to maxRedirects upon receiving HTTP 301 status (#2939)
1 parent aa3fcbe commit 2d889e8

12 files changed

+50
-50
lines changed

‎github/actions_artifacts.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -121,10 +121,10 @@ func (s *ActionsService) GetArtifact(ctx context.Context, owner, repo string, ar
121121
// DownloadArtifact gets a redirect URL to download an archive for a repository.
122122
//
123123
// GitHub API docs: https://docs.github.com/en/rest/actions/artifacts#download-an-artifact
124-
func (s *ActionsService) DownloadArtifact(ctx context.Context, owner, repo string, artifactID int64, followRedirects bool) (*url.URL, *Response, error) {
124+
func (s *ActionsService) DownloadArtifact(ctx context.Context, owner, repo string, artifactID int64, maxRedirects int) (*url.URL, *Response, error) {
125125
u := fmt.Sprintf("repos/%v/%v/actions/artifacts/%v/zip", owner, repo, artifactID)
126126

127-
resp, err := s.client.roundTripWithOptionalFollowRedirect(ctx, u, followRedirects)
127+
resp, err := s.client.roundTripWithOptionalFollowRedirect(ctx, u, maxRedirects)
128128
if err != nil {
129129
return nil, nil, err
130130
}

‎github/actions_artifacts_test.go‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -277,7 +277,7 @@ func TestActionsSerivice_DownloadArtifact(t *testing.T) {
277277
})
278278

279279
ctx := context.Background()
280-
url, resp, err := client.Actions.DownloadArtifact(ctx, "o", "r", 1, true)
280+
url, resp, err := client.Actions.DownloadArtifact(ctx, "o", "r", 1, 1)
281281
if err != nil {
282282
t.Errorf("Actions.DownloadArtifact returned error: %v", err)
283283
}
@@ -292,7 +292,7 @@ func TestActionsSerivice_DownloadArtifact(t *testing.T) {
292292

293293
const methodName = "DownloadArtifact"
294294
testBadOptions(t, methodName, func() (err error) {
295-
_, _, err = client.Actions.DownloadArtifact(ctx, "\n", "\n", -1, true)
295+
_, _, err = client.Actions.DownloadArtifact(ctx, "\n", "\n", -1, 1)
296296
return err
297297
})
298298

@@ -301,7 +301,7 @@ func TestActionsSerivice_DownloadArtifact(t *testing.T) {
301301
return nil, errors.New("failed to download artifact")
302302
})
303303
testBadOptions(t, methodName, func() (err error) {
304-
_, _, err = client.Actions.DownloadArtifact(ctx, "o", "r", 1, true)
304+
_, _, err = client.Actions.DownloadArtifact(ctx, "o", "r", 1, 1)
305305
return err
306306
})
307307
}
@@ -311,7 +311,7 @@ func TestActionsService_DownloadArtifact_invalidOwner(t *testing.T) {
311311
defer teardown()
312312

313313
ctx := context.Background()
314-
_, _, err := client.Actions.DownloadArtifact(ctx, "%", "r", 1, true)
314+
_, _, err := client.Actions.DownloadArtifact(ctx, "%", "r", 1, 1)
315315
testURLParseError(t, err)
316316
}
317317

@@ -320,7 +320,7 @@ func TestActionsService_DownloadArtifact_invalidRepo(t *testing.T) {
320320
defer teardown()
321321

322322
ctx := context.Background()
323-
_, _, err := client.Actions.DownloadArtifact(ctx, "o", "%", 1, true)
323+
_, _, err := client.Actions.DownloadArtifact(ctx, "o", "%", 1, 1)
324324
testURLParseError(t, err)
325325
}
326326

@@ -334,7 +334,7 @@ func TestActionsService_DownloadArtifact_StatusMovedPermanently_dontFollowRedire
334334
})
335335

336336
ctx := context.Background()
337-
_, resp, _ := client.Actions.DownloadArtifact(ctx, "o", "r", 1, false)
337+
_, resp, _ := client.Actions.DownloadArtifact(ctx, "o", "r", 1, 0)
338338
if resp.StatusCode != http.StatusMovedPermanently {
339339
t.Errorf("Actions.DownloadArtifact return status %d, want %d", resp.StatusCode, http.StatusMovedPermanently)
340340
}
@@ -355,7 +355,7 @@ func TestActionsService_DownloadArtifact_StatusMovedPermanently_followRedirects(
355355
})
356356

357357
ctx := context.Background()
358-
url, resp, err := client.Actions.DownloadArtifact(ctx, "o", "r", 1, true)
358+
url, resp, err := client.Actions.DownloadArtifact(ctx, "o", "r", 1, 1)
359359
if err != nil {
360360
t.Errorf("Actions.DownloadArtifact return error: %v", err)
361361
}

‎github/actions_workflow_jobs.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -115,10 +115,10 @@ func (s *ActionsService) GetWorkflowJobByID(ctx context.Context, owner, repo str
115115
// GetWorkflowJobLogs gets a redirect URL to download a plain text file of logs for a workflow job.
116116
//
117117
// GitHub API docs: https://docs.github.com/en/rest/actions/workflow-jobs#download-job-logs-for-a-workflow-run
118-
func (s *ActionsService) GetWorkflowJobLogs(ctx context.Context, owner, repo string, jobID int64, followRedirects bool) (*url.URL, *Response, error) {
118+
func (s *ActionsService) GetWorkflowJobLogs(ctx context.Context, owner, repo string, jobID int64, maxRedirects int) (*url.URL, *Response, error) {
119119
u := fmt.Sprintf("repos/%v/%v/actions/jobs/%v/logs", owner, repo, jobID)
120120

121-
resp, err := s.client.roundTripWithOptionalFollowRedirect(ctx, u, followRedirects)
121+
resp, err := s.client.roundTripWithOptionalFollowRedirect(ctx, u, maxRedirects)
122122
if err != nil {
123123
return nil, nil, err
124124
}

‎github/actions_workflow_jobs_test.go‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -138,7 +138,7 @@ func TestActionsService_GetWorkflowJobLogs(t *testing.T) {
138138
})
139139

140140
ctx := context.Background()
141-
url, resp, err := client.Actions.GetWorkflowJobLogs(ctx, "o", "r", 399444496, true)
141+
url, resp, err := client.Actions.GetWorkflowJobLogs(ctx, "o", "r", 399444496, 1)
142142
if err != nil {
143143
t.Errorf("Actions.GetWorkflowJobLogs returned error: %v", err)
144144
}
@@ -152,7 +152,7 @@ func TestActionsService_GetWorkflowJobLogs(t *testing.T) {
152152

153153
const methodName = "GetWorkflowJobLogs"
154154
testBadOptions(t, methodName, func() (err error) {
155-
_, _, err = client.Actions.GetWorkflowJobLogs(ctx, "\n", "\n", 399444496, true)
155+
_, _, err = client.Actions.GetWorkflowJobLogs(ctx, "\n", "\n", 399444496, 1)
156156
return err
157157
})
158158

@@ -161,7 +161,7 @@ func TestActionsService_GetWorkflowJobLogs(t *testing.T) {
161161
return nil, errors.New("failed to get workflow logs")
162162
})
163163
testBadOptions(t, methodName, func() (err error) {
164-
_, _, err = client.Actions.GetWorkflowJobLogs(ctx, "o", "r", 399444496, true)
164+
_, _, err = client.Actions.GetWorkflowJobLogs(ctx, "o", "r", 399444496, 1)
165165
return err
166166
})
167167
}
@@ -176,7 +176,7 @@ func TestActionsService_GetWorkflowJobLogs_StatusMovedPermanently_dontFollowRedi
176176
})
177177

178178
ctx := context.Background()
179-
_, resp, _ := client.Actions.GetWorkflowJobLogs(ctx, "o", "r", 399444496, false)
179+
_, resp, _ := client.Actions.GetWorkflowJobLogs(ctx, "o", "r", 399444496, 0)
180180
if resp.StatusCode != http.StatusMovedPermanently {
181181
t.Errorf("Actions.GetWorkflowJobLogs returned status: %d, want %d", resp.StatusCode, http.StatusMovedPermanently)
182182
}
@@ -199,7 +199,7 @@ func TestActionsService_GetWorkflowJobLogs_StatusMovedPermanently_followRedirect
199199
})
200200

201201
ctx := context.Background()
202-
url, resp, err := client.Actions.GetWorkflowJobLogs(ctx, "o", "r", 399444496, true)
202+
url, resp, err := client.Actions.GetWorkflowJobLogs(ctx, "o", "r", 399444496, 1)
203203
if err != nil {
204204
t.Errorf("Actions.GetWorkflowJobLogs returned error: %v", err)
205205
}

‎github/actions_workflow_runs.go‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -211,10 +211,10 @@ func (s *ActionsService) GetWorkflowRunAttempt(ctx context.Context, owner, repo
211211
// GetWorkflowRunAttemptLogs gets a redirect URL to download a plain text file of logs for a workflow run for attempt number.
212212
//
213213
// GitHub API docs: https://docs.github.com/en/rest/actions/workflow-runs#download-workflow-run-attempt-logs
214-
func (s *ActionsService) GetWorkflowRunAttemptLogs(ctx context.Context, owner, repo string, runID int64, attemptNumber int, followRedirects bool) (*url.URL, *Response, error) {
214+
func (s *ActionsService) GetWorkflowRunAttemptLogs(ctx context.Context, owner, repo string, runID int64, attemptNumber int, maxRedirects int) (*url.URL, *Response, error) {
215215
u := fmt.Sprintf("repos/%v/%v/actions/runs/%v/attempts/%v/logs", owner, repo, runID, attemptNumber)
216216

217-
resp, err := s.client.roundTripWithOptionalFollowRedirect(ctx, u, followRedirects)
217+
resp, err := s.client.roundTripWithOptionalFollowRedirect(ctx, u, maxRedirects)
218218
if err != nil {
219219
return nil, nil, err
220220
}
@@ -287,10 +287,10 @@ func (s *ActionsService) CancelWorkflowRunByID(ctx context.Context, owner, repo
287287
// GetWorkflowRunLogs gets a redirect URL to download a plain text file of logs for a workflow run.
288288
//
289289
// GitHub API docs: https://docs.github.com/en/rest/actions/workflow-runs#download-workflow-run-logs
290-
func (s *ActionsService) GetWorkflowRunLogs(ctx context.Context, owner, repo string, runID int64, followRedirects bool) (*url.URL, *Response, error) {
290+
func (s *ActionsService) GetWorkflowRunLogs(ctx context.Context, owner, repo string, runID int64, maxRedirects int) (*url.URL, *Response, error) {
291291
u := fmt.Sprintf("repos/%v/%v/actions/runs/%v/logs", owner, repo, runID)
292292

293-
resp, err := s.client.roundTripWithOptionalFollowRedirect(ctx, u, followRedirects)
293+
resp, err := s.client.roundTripWithOptionalFollowRedirect(ctx, u, maxRedirects)
294294
if err != nil {
295295
return nil, nil, err
296296
}

‎github/actions_workflow_runs_test.go‎

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -198,7 +198,7 @@ func TestActionsService_GetWorkflowRunAttemptLogs(t *testing.T) {
198198
})
199199

200200
ctx := context.Background()
201-
url, resp, err := client.Actions.GetWorkflowRunAttemptLogs(ctx, "o", "r", 399444496, 2, true)
201+
url, resp, err := client.Actions.GetWorkflowRunAttemptLogs(ctx, "o", "r", 399444496, 2, 1)
202202
if err != nil {
203203
t.Errorf("Actions.GetWorkflowRunAttemptLogs returned error: %v", err)
204204
}
@@ -212,7 +212,7 @@ func TestActionsService_GetWorkflowRunAttemptLogs(t *testing.T) {
212212

213213
const methodName = "GetWorkflowRunAttemptLogs"
214214
testBadOptions(t, methodName, func() (err error) {
215-
_, _, err = client.Actions.GetWorkflowRunAttemptLogs(ctx, "\n", "\n", 399444496, 2, true)
215+
_, _, err = client.Actions.GetWorkflowRunAttemptLogs(ctx, "\n", "\n", 399444496, 2, 1)
216216
return err
217217
})
218218
}
@@ -227,7 +227,7 @@ func TestActionsService_GetWorkflowRunAttemptLogs_StatusMovedPermanently_dontFol
227227
})
228228

229229
ctx := context.Background()
230-
_, resp, _ := client.Actions.GetWorkflowRunAttemptLogs(ctx, "o", "r", 399444496, 2, false)
230+
_, resp, _ := client.Actions.GetWorkflowRunAttemptLogs(ctx, "o", "r", 399444496, 2, 0)
231231
if resp.StatusCode != http.StatusMovedPermanently {
232232
t.Errorf("Actions.GetWorkflowRunAttemptLogs returned status: %d, want %d", resp.StatusCode, http.StatusMovedPermanently)
233233
}
@@ -250,7 +250,7 @@ func TestActionsService_GetWorkflowRunAttemptLogs_StatusMovedPermanently_followR
250250
})
251251

252252
ctx := context.Background()
253-
url, resp, err := client.Actions.GetWorkflowRunAttemptLogs(ctx, "o", "r", 399444496, 2, true)
253+
url, resp, err := client.Actions.GetWorkflowRunAttemptLogs(ctx, "o", "r", 399444496, 2, 1)
254254
if err != nil {
255255
t.Errorf("Actions.GetWorkflowRunAttemptLogs returned error: %v", err)
256256
}
@@ -266,7 +266,7 @@ func TestActionsService_GetWorkflowRunAttemptLogs_StatusMovedPermanently_followR
266266

267267
const methodName = "GetWorkflowRunAttemptLogs"
268268
testBadOptions(t, methodName, func() (err error) {
269-
_, _, err = client.Actions.GetWorkflowRunAttemptLogs(ctx, "\n", "\n", 399444496, 2, true)
269+
_, _, err = client.Actions.GetWorkflowRunAttemptLogs(ctx, "\n", "\n", 399444496, 2, 1)
270270
return err
271271
})
272272
}
@@ -397,7 +397,7 @@ func TestActionsService_GetWorkflowRunLogs(t *testing.T) {
397397
})
398398

399399
ctx := context.Background()
400-
url, resp, err := client.Actions.GetWorkflowRunLogs(ctx, "o", "r", 399444496, true)
400+
url, resp, err := client.Actions.GetWorkflowRunLogs(ctx, "o", "r", 399444496, 1)
401401
if err != nil {
402402
t.Errorf("Actions.GetWorkflowRunLogs returned error: %v", err)
403403
}
@@ -411,7 +411,7 @@ func TestActionsService_GetWorkflowRunLogs(t *testing.T) {
411411

412412
const methodName = "GetWorkflowRunLogs"
413413
testBadOptions(t, methodName, func() (err error) {
414-
_, _, err = client.Actions.GetWorkflowRunLogs(ctx, "\n", "\n", 399444496, true)
414+
_, _, err = client.Actions.GetWorkflowRunLogs(ctx, "\n", "\n", 399444496, 1)
415415
return err
416416
})
417417
}
@@ -426,7 +426,7 @@ func TestActionsService_GetWorkflowRunLogs_StatusMovedPermanently_dontFollowRedi
426426
})
427427

428428
ctx := context.Background()
429-
_, resp, _ := client.Actions.GetWorkflowRunLogs(ctx, "o", "r", 399444496, false)
429+
_, resp, _ := client.Actions.GetWorkflowRunLogs(ctx, "o", "r", 399444496, 0)
430430
if resp.StatusCode != http.StatusMovedPermanently {
431431
t.Errorf("Actions.GetWorkflowJobLogs returned status: %d, want %d", resp.StatusCode, http.StatusMovedPermanently)
432432
}
@@ -449,7 +449,7 @@ func TestActionsService_GetWorkflowRunLogs_StatusMovedPermanently_followRedirect
449449
})
450450

451451
ctx := context.Background()
452-
url, resp, err := client.Actions.GetWorkflowRunLogs(ctx, "o", "r", 399444496, true)
452+
url, resp, err := client.Actions.GetWorkflowRunLogs(ctx, "o", "r", 399444496, 1)
453453
if err != nil {
454454
t.Errorf("Actions.GetWorkflowJobLogs returned error: %v", err)
455455
}
@@ -465,7 +465,7 @@ func TestActionsService_GetWorkflowRunLogs_StatusMovedPermanently_followRedirect
465465

466466
const methodName = "GetWorkflowRunLogs"
467467
testBadOptions(t, methodName, func() (err error) {
468-
_, _, err = client.Actions.GetWorkflowRunLogs(ctx, "\n", "\n", 399444496, true)
468+
_, _, err = client.Actions.GetWorkflowRunLogs(ctx, "\n", "\n", 399444496, 1)
469469
return err
470470
})
471471
}

‎github/github.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1558,7 +1558,7 @@ func formatRateReset(d time.Duration) string {
15581558

15591559
// When using roundTripWithOptionalFollowRedirect, note that it
15601560
// is the responsibility of the caller to close the response body.
1561-
func (c *Client) roundTripWithOptionalFollowRedirect(ctx context.Context, u string, followRedirects bool, opts ...RequestOption) (*http.Response, error) {
1561+
func (c *Client) roundTripWithOptionalFollowRedirect(ctx context.Context, u string, maxRedirects int, opts ...RequestOption) (*http.Response, error) {
15621562
req, err := c.NewRequest("GET", u, nil, opts...)
15631563
if err != nil {
15641564
return nil, err
@@ -1577,10 +1577,10 @@ func (c *Client) roundTripWithOptionalFollowRedirect(ctx context.Context, u stri
15771577
}
15781578

15791579
// If redirect response is returned, follow it
1580-
if followRedirects && resp.StatusCode == http.StatusMovedPermanently {
1580+
if maxRedirects > 0 && resp.StatusCode == http.StatusMovedPermanently {
15811581
_ = resp.Body.Close()
15821582
u = resp.Header.Get("Location")
1583-
resp, err = c.roundTripWithOptionalFollowRedirect(ctx, u, false, opts...)
1583+
resp, err = c.roundTripWithOptionalFollowRedirect(ctx, u, maxRedirects-1, opts...)
15841584
}
15851585
return resp, err
15861586
}

‎github/repos.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1274,10 +1274,10 @@ func (s *RepositoriesService) ListBranches(ctx context.Context, owner string, re
12741274
// GetBranch gets the specified branch for a repository.
12751275
//
12761276
// GitHub API docs: https://docs.github.com/en/rest/branches/branches#get-a-branch
1277-
func (s *RepositoriesService) GetBranch(ctx context.Context, owner, repo, branch string, followRedirects bool) (*Branch, *Response, error) {
1277+
func (s *RepositoriesService) GetBranch(ctx context.Context, owner, repo, branch string, maxRedirects int) (*Branch, *Response, error) {
12781278
u := fmt.Sprintf("repos/%v/%v/branches/%v", owner, repo, branch)
12791279

1280-
resp, err := s.client.roundTripWithOptionalFollowRedirect(ctx, u, followRedirects)
1280+
resp, err := s.client.roundTripWithOptionalFollowRedirect(ctx, u, maxRedirects)
12811281
if err != nil {
12821282
return nil, nil, err
12831283
}

‎github/repos_contents.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -313,12 +313,12 @@ const (
313313
// or github.Zipball constant.
314314
//
315315
// GitHub API docs: https://docs.github.com/en/rest/repos/contents/#get-archive-link
316-
func (s *RepositoriesService) GetArchiveLink(ctx context.Context, owner, repo string, archiveformat ArchiveFormat, opts *RepositoryContentGetOptions, followRedirects bool) (*url.URL, *Response, error) {
316+
func (s *RepositoriesService) GetArchiveLink(ctx context.Context, owner, repo string, archiveformat ArchiveFormat, opts *RepositoryContentGetOptions, maxRedirects int) (*url.URL, *Response, error) {
317317
u := fmt.Sprintf("repos/%s/%s/%s", owner, repo, archiveformat)
318318
if opts != nil && opts.Ref != "" {
319319
u += fmt.Sprintf("/%s", opts.Ref)
320320
}
321-
resp, err := s.client.roundTripWithOptionalFollowRedirect(ctx, u, followRedirects)
321+
resp, err := s.client.roundTripWithOptionalFollowRedirect(ctx, u, maxRedirects)
322322
if err != nil {
323323
return nil, nil, err
324324
}

‎github/repos_contents_test.go‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -694,7 +694,7 @@ func TestRepositoriesService_GetArchiveLink(t *testing.T) {
694694
http.Redirect(w, r, "http://github.com/a", http.StatusFound)
695695
})
696696
ctx := context.Background()
697-
url, resp, err := client.Repositories.GetArchiveLink(ctx, "o", "r", Tarball, &RepositoryContentGetOptions{Ref: "yo"}, true)
697+
url, resp, err := client.Repositories.GetArchiveLink(ctx, "o", "r", Tarball, &RepositoryContentGetOptions{Ref: "yo"}, 1)
698698
if err != nil {
699699
t.Errorf("Repositories.GetArchiveLink returned error: %v", err)
700700
}
@@ -708,7 +708,7 @@ func TestRepositoriesService_GetArchiveLink(t *testing.T) {
708708

709709
const methodName = "GetArchiveLink"
710710
testBadOptions(t, methodName, func() (err error) {
711-
_, _, err = client.Repositories.GetArchiveLink(ctx, "\n", "\n", Tarball, &RepositoryContentGetOptions{}, true)
711+
_, _, err = client.Repositories.GetArchiveLink(ctx, "\n", "\n", Tarball, &RepositoryContentGetOptions{}, 1)
712712
return err
713713
})
714714

@@ -717,7 +717,7 @@ func TestRepositoriesService_GetArchiveLink(t *testing.T) {
717717
return nil, errors.New("failed to get archive link")
718718
})
719719
testBadOptions(t, methodName, func() (err error) {
720-
_, _, err = client.Repositories.GetArchiveLink(ctx, "o", "r", Tarball, &RepositoryContentGetOptions{}, true)
720+
_, _, err = client.Repositories.GetArchiveLink(ctx, "o", "r", Tarball, &RepositoryContentGetOptions{}, 1)
721721
return err
722722
})
723723
}
@@ -730,7 +730,7 @@ func TestRepositoriesService_GetArchiveLink_StatusMovedPermanently_dontFollowRed
730730
http.Redirect(w, r, "http://github.com/a", http.StatusMovedPermanently)
731731
})
732732
ctx := context.Background()
733-
_, resp, _ := client.Repositories.GetArchiveLink(ctx, "o", "r", Tarball, &RepositoryContentGetOptions{}, false)
733+
_, resp, _ := client.Repositories.GetArchiveLink(ctx, "o", "r", Tarball, &RepositoryContentGetOptions{}, 0)
734734
if resp.StatusCode != http.StatusMovedPermanently {
735735
t.Errorf("Repositories.GetArchiveLink returned status: %d, want %d", resp.StatusCode, http.StatusMovedPermanently)
736736
}
@@ -750,7 +750,7 @@ func TestRepositoriesService_GetArchiveLink_StatusMovedPermanently_followRedirec
750750
http.Redirect(w, r, "http://github.com/a", http.StatusFound)
751751
})
752752
ctx := context.Background()
753-
url, resp, err := client.Repositories.GetArchiveLink(ctx, "o", "r", Tarball, &RepositoryContentGetOptions{}, true)
753+
url, resp, err := client.Repositories.GetArchiveLink(ctx, "o", "r", Tarball, &RepositoryContentGetOptions{}, 1)
754754
if err != nil {
755755
t.Errorf("Repositories.GetArchiveLink returned error: %v", err)
756756
}

0 commit comments

Comments
 (0)