Skip to content

Enable Client.RateLimits to bypass the rate limit check - #1907

Merged
gmlewis merged 2 commits into
google:masterfrom
sa-spag:rate-limits
Jul 8, 2021
Merged

gmlewis merged 2 commits into
google:masterfrom
sa-spag:rate-limits

Conversation

@sa-spag

@sa-spag sa-spag commented Jun 22, 2021

Copy link
Copy Markdown
Contributor

Fixes #1899.

@google-cla google-cla Bot added the cla: yes Indication that the PR author has signed a Google Contributor License Agreement. label Jun 22, 2021

@gmlewis gmlewis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Very nice, @sa-spag !
I really like this solution... I was concerned that there would be much more disruption, but this looks very nice and clean... thank you!
LGTM.

Awaiting second LGTM before merging.

@gmlewis
gmlewis requested a review from wesleimp June 22, 2021 17:20
@gmlewis

gmlewis commented Jun 22, 2021

Copy link
Copy Markdown
Collaborator

Whups, sorry, it looks like the unit tests are not passing:

    github_test.go:1708: rate.Reset.Time > now RateLimits resp = &http.Response{Status:"404 Not Found", StatusCode:404, Proto:"HTTP/1.1", ProtoMajor:1, ProtoMinor:1, Header:http.Header{"Content-Length":[]string{"19"}, "Content-Type":[]string{"text/plain; charset=utf-8"}, "Date":[]string{"Tue, 22 Jun 2021 17:21:16 GMT"}, "X-Content-Type-Options":[]string{"nosniff"}}, Body:io.nopCloser{Reader:(*bytes.Buffer)(0xc0003c1860)}, ContentLength:19, TransferEncoding:[]string(nil), Close:false, Uncompressed:false, Trailer:http.Header(nil), Request:(*http.Request)(0xc0002f5e00), TLS:(*tls.ConnectionState)(nil)}, want StatusCode=403
--- FAIL: TestRateLimits_coverage (0.00s)

@gmlewis
gmlewis self-requested a review June 22, 2021 17:28

@gmlewis gmlewis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Changing approval until unit tests pass.

@sa-spag

sa-spag commented Jun 22, 2021

Copy link
Copy Markdown
Contributor Author

Very nice, @sa-spag !
I really like this solution... I was concerned that there would be much more disruption, but this looks very nice and clean... thank you!

Credits to @ViBiOh for the context.WithValue trick I had no clue about.

Whups, sorry, it looks like the unit tests are not passing...

Whups indeed, my bad. Got a little too confident, it is now fixed.

@codecov

codecov Bot commented Jun 22, 2021

Copy link
Copy Markdown

Codecov Report

Merging #1907 (7da44c3) into master (75644ea) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1907   +/-   ##
=======================================
  Coverage   97.65%   97.65%           
=======================================
  Files         105      105           
  Lines        6786     6788    +2     
=======================================
+ Hits         6627     6629    +2     
  Misses         86       86           
  Partials       73       73           
Impacted Files Coverage Δ
github/github.go 97.02% <100.00%> (+0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 75644ea...7da44c3. Read the comment docs.

Comment thread github/github_test.go
client.rateLimits[0].Reset.Time = time.Now().Add(10 * time.Minute)
resp, err = f()
if bypass := resp.Request.Context().Value(bypassRateLimitCheck); bypass != nil {
return

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm concerned that we are not checking the value of err here... might there be a case where we get an error that will be ignored here?

@sa-spag sa-spag Jun 23, 2021 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This helper explicitly sets the client up to trigger errors, and at this stage a rate limit error. As is, the test would fail if we check for err != nil for methodName == "RateLimits" as the request would result in a HTTP 404, since no handler has been created. We can set one up on TestRateLimits_coverage but I think it makes the test less straightforward. Client.RateLimits rate limit check bypass behavior is already covered by the newly introduced TestRateLimits_overQuota.

@gmlewis gmlewis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM.

Awaiting second LGTM before merging.

@gmlewis

gmlewis commented Jun 24, 2021

Copy link
Copy Markdown
Collaborator

Friendly ping @wesleimp

@Parker77 Parker77 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM.

@gmlewis
gmlewis merged commit cfcb8e1 into google:master Jul 8, 2021
@sa-spag
sa-spag deleted the rate-limits branch July 9, 2021 12:00
jlaportebot added a commit to jlaportebot/go-github that referenced this pull request Jun 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes Indication that the PR author has signed a Google Contributor License Agreement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Client.RateLimits shouldn't return RateLimitError

3 participants