feat!: Refactor request context

[stevehipwell]

Closes #4127

This PR makes the following changes:

• Switch to using http.NewRequestWithContext so we no longer need the withContext helper
• Refactor all HTTP calls to use client.BareDo to get benefits such as rate limit support

[stevehipwell]

@gmlewis I'm going to completely rebase this PR as I need to split it into 3.

[stevehipwell]

#4152 replaces the app engine removal part of this PR.

[stevehipwell]

@gmlewis I've rebased this PR and it should be ready for review now.

[stevehipwell]

@gmlewis I had to update the extraneousnew linter now that client.Do only has 2 args, could you please pay particular attention to that change as I don't have the full context there.

[codecov]

Codecov Report

❌ Patch coverage is 99.86922% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.18%. Comparing base (3430163) to head (58e431f).
⚠️ Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
test/fields/fields.go	0.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4151      +/-   ##
==========================================
- Coverage   93.18%   93.18%   -0.01%     
==========================================
  Files         210      210              
  Lines       24568    24564       -4     
==========================================
- Hits        22894    22889       -5     
- Misses       1478     1479       +1     
  Partials      196      196              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[gmlewis]

In case a force-push is performed, I'm going to stop my review here before examining the remaining 182 files.

[stevehipwell]

@gmlewis I won't force push this, I only force pushed to realign after we split the PR up and I did provide a warning that I was doing it. Could you possibly review the linter, the only real changes are there and in github.go.

[gmlewis]

@gmlewis I won't force push this, I only force pushed to realign after we split the PR up and I did provide a warning that I was doing it. Could you possibly review the linter, the only real changes are there and in github.go.

OK, the linter LGTM.

[stevehipwell]

@gmlewis I've made the requested docs changes.

[gmlewis]

One question, otherwise LGTM.

cc: @alexandear - @zyfy29 - @Not-Dhananjay-Mishra - @munlicode

[gmlewis]

LGTM

[Not-Dhananjay-Mishra]

LGTM 🚀

[stevehipwell]

@gmlewis how do you want me to resolve the conflict?

[gmlewis]

@stevehipwell - apologies, but could you please resolve conflicts before I merge?
Oh, and we need to hear from @alexandear too.

[stevehipwell]

@gmlewis how do you want me to resolve the conflict?

@gmlewis I'd normally rebase to fix a conflict like this, but I know you don't like the commits to be modified.

[gmlewis]

@gmlewis I'd normally rebase to fix a conflict like this, but I know you don't like the commits to be modified

1. Go to your master branch and git pull
2. Go to your branch and git merge master
3. Manually resolve conflicts or git mergetool if you have already set up a tool like kdiff3
4. git push ... to this PRs branch

[stevehipwell]

@gmlewis merge commit it is, was just checking.

[gmlewis]

Although if you have a better mechanism, I am fine with that in this huge PR.

[stevehipwell]

@gmlewis assuming that you're squash merging and you'd rather the commits aren't modified, a merge commit is likely to be the best compromise.

[stevehipwell]

@gmlewis it should be ready again.

[stevehipwell]

@alexandear I think we're just waiting on you now?

[stevehipwell]

@gmlewis I think we're ready to merge now.

[alexandear]

Why is this added? Before this PR we don't have if !s.client.DisableRateLimitCheck

[alexandear]

If I comment out the check

tests still pass:

❯ go test -failfast -count=1 ./...
ok      github.com/google/go-github/v85/github  2.472s
?       github.com/google/go-github/v85/test/fields     [no test files]
?       github.com/google/go-github/v85/test/integration        [no test files]

[stevehipwell]

It short circuits the context modification as it's never going to be retrieved with s.client.DisableRateLimitCheck set to true.

[alexandear]

I propose to move this change to a separate PR and add a test.

[stevehipwell]

I don't want to remove this change as it's demonstrably "correct". @alexandear if you have a suggestion for how to test this I'm happy to add a test, but it doesn't look testable to me.

[alexandear]

Should be something like the following:

func TestRateLimits_bypassRateLimitCheckContext(t *testing.T) {
	t.Parallel()
	tests := []struct {
		name                  string
		disableRateLimitCheck bool
		wantBypassValue       any
	}{
		{
			name:                  "DisableRateLimitCheck disabled",
			disableRateLimitCheck: false,
			wantBypassValue:       true,
		},
		{
			name:                  "DisableRateLimitCheck enabled",
			disableRateLimitCheck: true,
			wantBypassValue:       nil,
		},
	}

	for _, tt := range tests {
		t.Run(tt.name, func(t *testing.T) {
			t.Parallel()
			client, mux, _ := setup(t)
			client.DisableRateLimitCheck = tt.disableRateLimitCheck

			mux.HandleFunc("/rate_limit", func(w http.ResponseWriter, r *http.Request) {
				testMethod(t, r, "GET")
				fmt.Fprint(w, `{"resources":{}}`)
			})

			_, resp, err := client.RateLimit.Get(t.Context())
			if err != nil {
				t.Errorf("RateLimits returned error: %v", err)
			}

			if got, want := resp.Request.Context().Value(BypassRateLimitCheck), tt.wantBypassValue; got != want {
				t.Errorf("Request context bypass value = %v, want %v", got, want)
			}
		})
	}
}