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

Handle rate limiting gracefully #34

Open
3 tasks
bnb opened this issue Nov 17, 2018 · 12 comments
Open
3 tasks

Handle rate limiting gracefully #34

bnb opened this issue Nov 17, 2018 · 12 comments
Labels
bug Something isn't working enhancement New feature or request
Milestone

Comments

@bnb
Copy link
Member

bnb commented Nov 17, 2018

When testing to make sure nothing breaks, I semi-often get rate-limited by GitHub. This is because there are (IIRC) 50 queries accepted before 1-hour rate limiting kicks in.

Obviously this shouldn't be happening for a normal user (though I'd applaud a user that was working on dozens of good-first-issues), but it may happen for contributors.

Checklist:

  • Handle errors in the CLI gracefully
  • Allow consumers of the module to pass GitHub authentication, which we would then pass to @octokit/rest
  • Documentation for both of the above
@bnb bnb added bug Something isn't working enhancement New feature or request labels Nov 17, 2018
@bnb bnb added this to the 1.0.0 milestone Nov 17, 2018
@ghost
Copy link

ghost commented Nov 18, 2018

Found this in the docs about rate limits:

The Search API has a custom rate limit. For requests using Basic Authentication, OAuth, or client ID and secret, you can make up to 30 requests per minute. For unauthenticated requests, the rate limit allows you to make up to 10 requests per minute.

https://developer.github.com/v3/search/#rate-limit

@ghost
Copy link

ghost commented Nov 18, 2018

Also, not sure how much users will give GitHub credentials to avoid these limits. So not sure if it is worth adding an authentication functionality just for this? Otherwise I would like to give it a shot, found this article about a CLI application getting and storing a OAuth token etc.

@bnb
Copy link
Member Author

bnb commented Nov 20, 2018

I believe it's 5k/hour or so – I have experience with this from building out NodeKitten.

@bnb
Copy link
Member Author

bnb commented Nov 20, 2018

For implementation of this, I believe we can just accept an object and pass it directly to @octokit/rest.js, which is the lib we're using for GitHub. See the Authentication examples in their README.md for an example – in NodeKitten I used token authentication but we could just let the user decide how they want to authenticate by passing an object as @octokit/rest.js would expect.

@bnb bnb closed this as completed Nov 20, 2018
@bnb bnb reopened this Nov 20, 2018
@bnb
Copy link
Member Author

bnb commented Nov 20, 2018

Oops didn't mean to close 😁

@maddhruv
Copy link
Member

I believe it's 5k/hour or so

if the rate is 5K ☘️ I don't think we/any user at any point exceed this rate
(except in case of bruting 😉 )

@ghost
Copy link

ghost commented Nov 21, 2018

I think the search that we use is limited at a lower rate than normal requests. Have tried it out, and can hit the limit if i make a dozen requests in a minute.

@ghost
Copy link

ghost commented Nov 21, 2018

For implementation of this, I believe we can just accept an object and pass it directly to @octokit/rest.js

If we don't store the authentication in a way, the user will have to login/authenticate each time we run? Don't think that is a fun UX if we ask that every time.

@bnb
Copy link
Member Author

bnb commented Nov 21, 2018

@kennethvandenberghe so this is more for when the user require()s the module to use the output programmatically and less when the user is using it from the CLI.

IMO it's less needed on the CLI since I can't really think of a use case in which a user (not a contributor) is genuinely using the CLI to find a Good First Issue.

That said, we may want to add the ability for contributors to add a file locally like github-auth.json that includes personal auth tokens so we have less of an issue when working on contributing? Perhaps that could be split out into a different issue.

@maddhruv
Copy link
Member

  • As for a module consumption scenario - we can add auth token as an option to it
  • and for the cli, I don't think a user might end up making hundreds to request/hour, hence no need in the CLI

@ghost
Copy link

ghost commented Nov 23, 2018

So to conclude stuff here so i can start fixing this stuff finally, what is the goal specifically @bnb?

@bnb
Copy link
Member Author

bnb commented Nov 24, 2018

@kennethvandenberghe enable the user to pass an object to the module when calling it that we will pass directly to @octokit/rest.js.

We don't need to be specific about what that object looks like, since @octokit/rest.js already has mechanisms for that – we just need to pass that object to @octokit/rest.js if it is provided.

Here's the examples from the @octokit/rest.js README: https://github.com/octokit/rest.js#authentication

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants