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

feat: way to avoid github API limit (add tests + docs) #123

Merged
merged 10 commits into from
Dec 28, 2018

Conversation

jakebolam
Copy link
Collaborator

@jakebolam jakebolam commented Dec 23, 2018

Follow-up to #122, adding tests and docs. Also converted the pattern to follow what was done for gitlab.

What:
Add a way to avoid GITHUB API LIMIT by supplying an optional GITHUB_AUTH token to increase github rate limiting from 50 requests to 5000 (and limited by user, not IP)

Why:
Fixes: #121

How:
By Me

Checklist:

  • Documentation
  • Tests
  • Ready to be merged
  • Added myself to contributors table

README.md Outdated
## Contributors

Thanks goes to these wonderful people
([emoji key](https://github.com/kentcdodds/all-contributors#emoji-key)):

<!-- ALL-CONTRIBUTORS-LIST:START - Do not remove or modify this section -->
<!-- prettier-ignore -->
| [<img src="https://avatars.githubusercontent.com/u/3869412?v=3" width="100px;"/><br /><sub><b>Jeroen Engels</b></sub>](https://github.com/jfmengels)<br />[💻](https://github.com/jfmengels/all-contributors-cli/commits?author=jfmengels "Code") [📖](https://github.com/jfmengels/all-contributors-cli/commits?author=jfmengels "Documentation") [⚠️](https://github.com/jfmengels/all-contributors-cli/commits?author=jfmengels "Tests") | [<img src="https://avatars.githubusercontent.com/u/1500684?v=3" width="100px;"/><br /><sub><b>Kent C. Dodds</b></sub>](http://kentcdodds.com/)<br />[📖](https://github.com/jfmengels/all-contributors-cli/commits?author=kentcdodds "Documentation") [💻](https://github.com/jfmengels/all-contributors-cli/commits?author=kentcdodds "Code") | [<img src="https://avatars.githubusercontent.com/u/14871650?v=3" width="100px;"/><br /><sub><b>João Guimarães</b></sub>](https://github.com/jccguimaraes)<br />[💻](https://github.com/jfmengels/all-contributors-cli/commits?author=jccguimaraes "Code") | [<img src="https://avatars.githubusercontent.com/u/1282980?v=3" width="100px;"/><br /><sub><b>Ben Briggs</b></sub>](http://beneb.info)<br />[💻](https://github.com/jfmengels/all-contributors-cli/commits?author=ben-eb "Code") | [<img src="https://avatars.githubusercontent.com/u/22768990?v=3" width="100px;"/><br /><sub><b>Itai Steinherz</b></sub>](https://github.com/itaisteinherz)<br />[📖](https://github.com/jfmengels/all-contributors-cli/commits?author=itaisteinherz "Documentation") [💻](https://github.com/jfmengels/all-contributors-cli/commits?author=itaisteinherz "Code") | [<img src="https://avatars.githubusercontent.com/u/5701162?v=3" width="100px;"/><br /><sub><b>Alex Jover</b></sub>](https://github.com/alexjoverm)<br />[💻](https://github.com/jfmengels/all-contributors-cli/commits?author=alexjoverm "Code") [📖](https://github.com/jfmengels/all-contributors-cli/commits?author=alexjoverm "Documentation") |
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it appears running yarn kcd-scripts contributors generate does not work locally 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Turns out .all-contributorsrc JSON file was invalid and was crashing silently on generate

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Spent an hour debugging in node trying to find this. Have added better support with this in PR: https://github.com/jfmengels/all-contributors-cli/pull/124/files

@jakebolam jakebolam changed the title feat: way to avoid github API limit (add tests + docs) (wip) feat: way to avoid github API limit (add tests + docs) Dec 23, 2018
@jakebolam jakebolam changed the title (wip) feat: way to avoid github API limit (add tests + docs) feat: way to avoid github API limit (add tests + docs) Dec 23, 2018
@@ -232,7 +232,7 @@
"avatar_url": "https://avatars2.githubusercontent.com/u/8073251?v=4",
"profile": "https://github.com/xuchaoying",
"contributions": [
"code",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was causing all-contributors to crash hard with a useless error message Cannot read property 'then' of null I'm going to propose we improve the parsing of the config file, to alert on issues like this

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@jakebolam
Copy link
Collaborator Author

jakebolam commented Dec 23, 2018

@kentcdodds, follow up to #122, added tests/docs and fixed all-contributors (was previously broken with an invalid json files) (have added a guard for this in https://github.com/jfmengels/all-contributors-cli/pull/124/files).

@@ -210,6 +210,10 @@ These are the keys you can specify:
}
```

In some cases you may see the error message 'GitHub API rate limit exceeded for xxx'. You may need to set an environment variable named `PRIVATE_TOKEN` in order to circumvent this [GitHub rate limit](https://developer.github.com/v3/rate_limit/).
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see anywhere in the code that reads the PRIVATE_TOKEN environment variable. Am I missing something?

Copy link
Collaborator Author

@jakebolam jakebolam Dec 27, 2018

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Gotcha. Thanks!

@kentcdodds
Copy link
Collaborator

I can merge this, but the build is broken and we can't have a release until it's fixed (the release is automated). I don't have time to look into it I'm afraid. If you'd like to investigate and open a PR for that, I'd appreciate it. Thanks!

@kentcdodds kentcdodds merged commit 194d000 into all-contributors:master Dec 28, 2018
@jakebolam jakebolam deleted the patch-1 branch December 29, 2018 06:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GitHub API rate limit exceeded for xxx
2 participants