Skip to content
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

Correctly raise Octokit::TooManyRequests when hitting secondary rate limit #1382

Merged

Conversation

jasonopslevel
Copy link
Contributor

@jasonopslevel jasonopslevel commented Dec 13, 2021

The server returns 403 for a large swath of errors, and then client then
re-raises them as more appropriate errors via inspecting the body of the
response. Adding a case to raise Octokit::TooManyRequests for the secondary
rate limit error as well.

For example, the body returned for a 'primary' rate limit:

API rate limit exceeded for installation ID XXXXXXXX. // See: https://docs.github.com/rest/overview/resources-in-the-rest-api#rate-limiting

and the body for the secondary rate limit:

You have exceeded a secondary rate limit. Please wait a few minutes before you try again. // See: https://docs.github.com/en/free-pro-team@latest/rest/overview/resources-in-the-rest-api#secondary-rate-limits

…limit

The server returns 403 for a large swath of errors, and then client then
re-raises them as more appropriate errors via inspecting the body of the
reponse. Adding a case to raise Octokit::TooManyRequests for the secondary
rate limit error as well.
@@ -68,6 +68,8 @@ def self.error_for_401(headers)
def self.error_for_403(body)
if body =~ /rate limit exceeded/i
Copy link
Contributor Author

Choose a reason for hiding this comment

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

An alternative would be to instead just do:

Suggested change
if body =~ /rate limit exceeded/i
if body =~ /rate limit/i

However, went with the more explicit approach for now.

Copy link

@joelthecoder joelthecoder left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me! I just encountered this same case, so thank you for putting in the work! 😄

@nickfloyd nickfloyd merged commit 6fb1139 into octokit:4-stable May 24, 2022
@nickfloyd nickfloyd added Type: Feature New feature or request and removed improvement labels Oct 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants