Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow up to maxRedirects upon receiving HTTP 301 status #2939

Merged
merged 1 commit into from
Sep 25, 2023

Conversation

k0ral
Copy link
Contributor

@k0ral k0ral commented Sep 25, 2023

The roundTripWithOptionalFollowRedirect() method currently takes a boolean parameter followRedirects, used to support HTTP 301 redirections. However, at most 1 redirection is handled at the moment (the parameter is set to false after the first iteration.)

I've stumbled upon multiple repositories where https://api.github.com/repos/<OWNER>/<PROJECT>/branches/<BRANCH> redirects twice ; as a result, the GetBranch() method only follows the first redirection, then returns the 2nd HTTP 301 response.

This MR proposes to generalize the followRedirects boolean into a maxRedirects integer, to support an arbitrary number of redirections. This impacts multiple methods, not just GetBranch().

⚠️ This is a breaking change.

@codecov
Copy link

codecov bot commented Sep 25, 2023

Codecov Report

Merging #2939 (7d6dbef) into master (aa3fcbe) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #2939   +/-   ##
=======================================
  Coverage   98.17%   98.17%           
=======================================
  Files         143      143           
  Lines       12602    12602           
=======================================
  Hits        12372    12372           
  Misses        156      156           
  Partials       74       74           
Files Changed Coverage Δ
github/actions_artifacts.go 95.71% <100.00%> (ø)
github/actions_workflow_jobs.go 100.00% <100.00%> (ø)
github/actions_workflow_runs.go 100.00% <100.00%> (ø)
github/github.go 98.11% <100.00%> (ø)
github/repos.go 98.89% <100.00%> (ø)
github/repos_contents.go 87.97% <100.00%> (ø)

@gmlewis gmlewis added the Breaking API Change PR will require a bump to the major version num in next release. Look here to see the change(s). label Sep 25, 2023
@gmlewis gmlewis changed the title feat: Allow for more than 1 recursive redirection upon receiving HTTP 301 status Allow up to maxRedirects upon receiving HTTP 301 status Sep 25, 2023
Copy link
Collaborator

@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, @k0ral !
LGTM.

Awaiting second LGTM+Approval from any other contributor to this repo before merging.

@gmlewis gmlewis added the NeedsReview PR is awaiting a review before merging. label Sep 25, 2023
@gmlewis gmlewis removed the NeedsReview PR is awaiting a review before merging. label Sep 25, 2023
@gmlewis
Copy link
Collaborator

gmlewis commented Sep 25, 2023

Thank you, @valbeat !
Merging.

@gmlewis gmlewis merged commit 2d889e8 into google:master Sep 25, 2023
7 checks passed
@WillAbides
Copy link
Contributor

I know this is already merged, but I would like to suggest that since we are doing a breaking change for these methods we consider updating them to accept an options struct instead of just the maxRedirects option. This will allow future options to be added without a breaking change. If they previously used an options struct this wouldn't need to be a breaking change. We can't avoid a break now, but we can avoid future breaks.

@gmlewis
Copy link
Collaborator

gmlewis commented Oct 4, 2023

Good idea, @WillAbides - do you want to make a PR that addresses this and we will hold off on the next release until it is merged?

@gmlewis
Copy link
Collaborator

gmlewis commented Oct 11, 2023

@WillAbides - I was requested to cut a new release, but just remembered that you wanted to change this maxRedirects API to use options instead, and thereby break the API twice before a new release instead of breaking the API twice between two different major releases.

Do you want me to hold off on cutting the new release, or shall we simply go ahead and break it twice?

I'm leaning toward breaking it twice at the moment, since I like to be responsive when users want a new release and it has been about a month since the last one.

@gmlewis
Copy link
Collaborator

gmlewis commented Oct 11, 2023

I'll go ahead on the release. We can work on an improved interface for the next breaking release.

@WillAbides
Copy link
Contributor

@gmlewis I agree with going ahead with the release because the need to move to options is hypothetical at this point.

suzuki-shunsuke added a commit to aquaproj/aqua that referenced this pull request Oct 16, 2023
* fix(deps): update module github.com/google/go-github/v55 to v56

* fix(google/go-github): address the breaking change of go-github

- google/go-github#2939
- #2340 (comment)

---------

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Shunsuke Suzuki <[email protected]>
Co-authored-by: Shunsuke Suzuki <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking API Change PR will require a bump to the major version num in next release. Look here to see the change(s).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants