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

[Feature Request] Return Octokit::RateLimit struct as context for 401 / Forbidden errors to have better error handling #1265

Closed
jatindhankhar opened this issue Jun 12, 2020 · 3 comments
Labels
Type: Feature New feature or request

Comments

@jatindhankhar
Copy link
Contributor

jatindhankhar commented Jun 12, 2020

At times when Github Rate Limiting kicks in, Octokit starts throwing Octokit::Forbidden, usually it is TooManyRequests or AbuseDetected

In such cases it is help to get the Octokit::RateLimit so the consumer can appropriate take action of retrying the error ( for example using the resets_at / resets_in to trigger retry after the prescribed time )

For AbuseDetected we can also expose Retry-After from the response headers

If this seems something that have chances of merging, I can submit a PR.

From the surface, the changes seems simple, just need to modify the error initialiser

lib/octokit/error.rb

def initialize(response=nil,context=nil)
      @response = response
      super(build_error_message)
      context = Octokit::RateLimit.from_response(response) if self.superclass == Octokit::Forbidden
    end

So consumer can use it

    sidekiq_retry_in do |attempt, exception|
    case exception
    when Octokit::TooManyRequests
      exception.context.resets_in
    when Octokit::AbuseDetected
      exception.context.resets_in
    end
  end
@jatindhankhar jatindhankhar changed the title [Feature Request] Return Octokit::RateLimit struct as context for 401 / Forbidden error for better error handling [Feature Request] Return Octokit::RateLimit struct as context for 401 / Forbidden errors to have better error handling Jun 12, 2020
@tarebyte
Copy link
Member

This sounds like a neat feature, we'd love to see a PR @jatindhankhar

@jatindhankhar
Copy link
Contributor Author

Hi @tarebyte,

Thanks for validating the initial idea, here is the PR that should work - #1270

Please check whenever you have a free moment :)

@jatindhankhar
Copy link
Contributor Author

Closing since PR #1270 was merged

@nickfloyd nickfloyd added Type: Feature New feature or request and removed feature 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

No branches or pull requests

3 participants