-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
feat: Allow blocking until primary rate limit is reset #3117
feat: Allow blocking until primary rate limit is reset #3117
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3117 +/- ##
==========================================
- Coverage 97.72% 92.87% -4.85%
==========================================
Files 153 170 +17
Lines 13390 11445 -1945
==========================================
- Hits 13085 10630 -2455
- Misses 215 724 +509
- Partials 90 91 +1 ☔ View full report in Codecov by Sentry. |
It would also be nice to add one or more unit tests that would increase the code coverage for the new code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, @erezrokah !
LGTM.
Awaiting second LGTM+Approval from any other contributor to this repo before merging.
Thanks for the quick review and feedback, I also added a 1 second buffer to the sleep time to avoid any edge cases |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @erezrokah .
Still awaiting second LGTM+Approval from any other contributor to this repo before merging.
All contributions, including code reviews, by other volunteers is greatly appreciated, as we have several outstanding PRs that need reviews. Thanks.
Hi @gmlewis I'm wondering if I should update the PR to take into account context cancellation, by changing timer := time.NewTimer(time.Until(reset) + buffer)
select {
case <-ctx.Done():
if !timer.Stop() {
<-timer.C
}
return nil, ctx.Err()
case <-timer.C:
} |
Ah, that's an excellent idea, @erezrokah - I think you are right that this should be added. |
Added 22eba4c. In one case I had to wrap the context error in a rate limit error. I can also refactor the code a bit to have |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple nits, otherwise LGTM.
Co-authored-by: Glenn Lewis <6598971+gmlewis@users.noreply.github.com>
Co-authored-by: Glenn Lewis <6598971+gmlewis@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, @erezrokah !
LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Thank you, @be0x74a ! |
This is now available here: https://github.com/google/go-github/releases/tag/v62.0.0 |
Fixes #3114
Feedback very much welcomed, some notes:
bypassRateLimitCheck
with the exception thatbypassRateLimitCheck
is private, not sure if to use another dedicated exportedrequestContext
type or also makebypassRateLimitCheck
public for consistencyreq.Context().Value(SleepUntilPrimaryRateLimitResetWhenRateLimited) == true
is also an option (instead of!=nil
) and might be more readable, but that is not consistent with thebypassRateLimitCheck
check