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

fix: use "secondary" instead of "abuse" for rate limit #2266

Merged
merged 1 commit into from Aug 9, 2022

Conversation

nimadini
Copy link
Contributor

@nimadini nimadini commented Aug 4, 2022

GitHub renamed abuse limits to secondary rate limits in 2021:
https://docs.github.com/en/rest/overview/resources-in-the-rest-api#secondary-rate-limits

@octokit/plugin-throttling followed suit in March 2022, and released
version 3.6.0 with this new name (and deprecated onAbuseLimit):
octokit/plugin-throttling.js#455

This commit updates the codebase to use the new name.


View rendered README.md

GitHub renamed `abuse limits` to `secondary rate limits` in 2021:
https://docs.github.com/en/rest/overview/resources-in-the-rest-api#secondary-rate-limits

`@octokit/plugin-throttling` followed suit in March 2022, and released
version `3.6.0` with this new name (and deprecated `onAbuseLimit`):
octokit/plugin-throttling.js#455

This commit updates the codebase to use the new name.
@ghost ghost added this to Inbox in JS Aug 4, 2022
@wolfy1339
Copy link
Member

Could you follow the same changes as in @octokit/plugin-throttling, and add an alias for the old name that logs a deprecation message.

@gr2m
Copy link
Contributor

gr2m commented Aug 4, 2022

Could you follow the same changes as in @octokit/plugin-throttling, and add an alias for the old name that logs a deprecation message.

I think the deprecation message should already be shown, as the options are simply passed through to @octokit/plugin-throttling. Can you confirm @nimadini?

@gr2m gr2m added the Type: Bug Something isn't working as documented label Aug 4, 2022
@ghost ghost moved this from Inbox to Bugs in JS Aug 4, 2022
@nimadini
Copy link
Contributor Author

nimadini commented Aug 4, 2022

Thanks for the quick review!

I confirmed the deprecation message (from @octokit/plugin-throttling) is printed when a secondary rate limit is hit.

To test it, I pointed my Octokit client to a dummy server that returns a 403 with an error message that has the secondary rate keyword in it. (See https://gist.github.com/nimadini/516b82ff69e374259ce23909c1a6f6da for details.)

Maybe an earlier place @octokit/plugin-throttling could warn users is during construction of the Octokit client, instead of later on when the secondary rate limit is hit. Specially because by design @octokit/plugin-throttling implements GitHub's best practices to avoid hitting the secondary rate limit in the first place (and it does a great job at that). So in practice, this warning message may never be shown to the user. 🙂

@nimadini
Copy link
Contributor Author

nimadini commented Aug 9, 2022

Confirmed above @gr2m. 🙂 Can you please take another look when you have a minute?

@gr2m gr2m changed the title Use "secondary" instead of "abuse" for rate limit. fix: use "secondary" instead of "abuse" for rate limit Aug 9, 2022
@gr2m gr2m merged commit 650d10a into octokit:main Aug 9, 2022
JS automation moved this from Bugs to Done Aug 9, 2022
@github-actions
Copy link

github-actions bot commented Aug 9, 2022

🎉 This PR is included in version 2.0.5 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Something isn't working as documented
Projects
No open projects
JS
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants