feat: Add DisableRateLimitCheck option to client #3607
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3607 +/- ##
=======================================
Coverage 91.33% 91.33%
=======================================
Files 184 184
Lines 16166 16169 +3
=======================================
+ Hits 14765 14768 +3
Misses 1227 1227
Partials 174 174 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Thank you, @stevehipwell!
A couple minor tweaks, please, then we should be ready for merging after getting a second LGTM+Approval from any other contributor to this repo.
@alexandear - might you have time for a code review? Thank you!
|
@gmlewis I've fixed the issues you found. |
There was a problem hiding this comment.
Thank you, @stevehipwell!
LGTM.
Awaiting second LGTM+Approval from any other contributor to this repo.
README.md
Outdated
| log.Println("hit rate limit") | ||
| var rateErr *github.RateLimitError | ||
| if errors.As(err, &rateError) { | ||
| log.Printf("hit primary rate limit, used %d of %d\n", rateErr.Rate.Used, rateErr.rate.Limit) |
There was a problem hiding this comment.
| log.Printf("hit primary rate limit, used %d of %d\n", rateErr.Rate.Used, rateErr.rate.Limit) | |
| log.Printf("hit primary rate limit, used %d of %d\n", rateErr.Rate.Used, rateErr.Rate.Limit) |
| To detect a primary API rate limit error, you can check if the error is a | ||
| `RateLimitError`. |
There was a problem hiding this comment.
| To detect a primary API rate limit error, you can check if the error is a | |
| `RateLimitError`. | |
| To detect a primary API rate limit error, you can check if the error is a `github.RateLimitError`. |
There was a problem hiding this comment.
Really? The text in this block appears to be formatted at 80 characters so that's what I've stuck to.
There was a problem hiding this comment.
I'm not sure we need to strictly stick to 80 characters. Many lines in the README already exceed that limit - for example:
Line 109 in 0c6bd91
Also, we currently don’t have any linter or formatting rule that enforces a line length limit in the README.
There was a problem hiding this comment.
This block was limited to 80 characters and I've kept it this way other than where there is a link (inspired by the Google Go formatting doc). Given that there is no linting or formatting rules and the README isn't consistent I don't think there is any value in being pedantic here (personally I prefer unbounded line lengths).
| To detect an API secondary rate limit error, you can check if the error is an | ||
| `AbuseRateLimitError`. |
There was a problem hiding this comment.
| To detect an API secondary rate limit error, you can check if the error is an | |
| `AbuseRateLimitError`. | |
| To detect an API secondary rate limit error, you can check if the error is an `github.AbuseRateLimitError`. |
There was a problem hiding this comment.
Same as above.
README.md
Outdated
| log.Println("hit secondary rate limit") | ||
| var rateErr *github.AbuseRateLimitError | ||
| if errors.As(err, &rateError) { | ||
| log.Printf("hit secondary rate limit, retry after %v\n", rateErr.RetryAfter) |
There was a problem hiding this comment.
| log.Printf("hit secondary rate limit, retry after %v\n", rateErr.RetryAfter) | |
| log.Printf("hit secondary rate limit, retry after %v\n", rateErr.RetryAfter) |
| You can use [gofri/go-github-ratelimit](https://github.com/gofri/go-github-ratelimit) to handle | ||
| secondary rate limit sleep-and-retry for you, as well as primary rate limit abuse-prevention and callback triggering. | ||
| If you need to make a request even if the rate limit has been hit you can use | ||
| the `BypassRateLimitCheck` method to bypass the rate limit check and make the |
There was a problem hiding this comment.
Let's add github. for consistency:
| the `BypassRateLimitCheck` method to bypass the rate limit check and make the | |
| the `github.BypassRateLimitCheck` method to bypass the rate limit check and make the |
| ["About secondary rate limits"](https://docs.github.com/en/rest/using-the-rest-api/rate-limits-for-the-rest-api?apiVersion=2022-11-28#about-secondary-rate-limits). | ||
| If the client is an [OAuth app](https://docs.github.com/en/rest/using-the-rest-api/rate-limits-for-the-rest-api#primary-rate-limit-for-oauth-apps) | ||
| you can use the apps higher rate limit to request public data by using the | ||
| `UnauthenticatedRateLimitedTransport` to make calls as the app instead of as |
There was a problem hiding this comment.
| `UnauthenticatedRateLimitedTransport` to make calls as the app instead of as | |
| `github.UnauthenticatedRateLimitedTransport` to make calls as the app instead of as |
| // DisableRateLimitCheck stops the client checking for rate limits or tracking | ||
| // them. This is different to setting BypassRateLimitCheck in the context, |
There was a problem hiding this comment.
| // DisableRateLimitCheck stops the client checking for rate limits or tracking | |
| // them. This is different to setting BypassRateLimitCheck in the context, | |
| // DisableRateLimitCheck stops the client checking for rate limits or tracking them. | |
| // This is different to setting BypassRateLimitCheck in the context, |
| return &Response{ | ||
| Response: err.Response, | ||
| }, err | ||
| if !c.DisableRateLimitCheck { |
There was a problem hiding this comment.
Could we add a unit test for this new field?
|
Please resolve the conflicts when you have a chance. Also, feel free to ignore all of my comments except the one about adding a unit test - that one is a must-have. |
Signed-off-by: Steve Hipwell <steve.hipwell@gmail.com>
4928013 to
ad32dc4
Compare
|
@alexandear I've added a unit test for the new disable feature and one for the existing bypass feature that didn't have one. @gmlewis I rebased to fix the merge conflict, I hope that's OK (I've left the commit structure as it was). |
|
Thank you, @stevehipwell and @alexandear! |
This PR adds the
DisableRateLimitCheckoption to the client to disable all client level rate limit logic. It also updates the documentation on the rate limit logic.Fixes #3585
@gmlewis apologies if there are quality issues with this PR, my VS Code is broken for this codebase so I put off the work as long as I could.