Allow up to maxRedirects upon receiving HTTP 301 status

[k0ral]

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]

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]

Thank you, @k0ral !
LGTM.

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

[gmlewis]

Thank you, @valbeat !
Merging.

[WillAbides]

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]

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]

@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]

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

[WillAbides]

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