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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add rate limiting Octokit plugin #1885

Closed
wants to merge 1 commit into from

Conversation

erezrokah
Copy link
Contributor

@erezrokah erezrokah commented Mar 19, 2023

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes ##1503 馃
Also related to #1440.
Could replace #1504

This PR adds the recommended throttling plugin, see https://octokit.github.io/rest.js/v19#throttling.

This probably needs more testing and consideration before merging as it can have a big impact. Opening for feedback.
I can follow up on the failing tests if maintainers think this is a good approach

@erezrokah erezrokah requested review from a team as code owners March 19, 2023 15:42
@product-auto-label product-auto-label bot added the size: s Pull request size is small. label Mar 19, 2023
@erezrokah
Copy link
Contributor Author

Maybe we can put this behind an option so the default behavior is the same

Copy link
Contributor

@chingor13 chingor13 left a comment

Choose a reason for hiding this comment

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

I would prefer if we dependency inject the configured octokit APIs rather than trying to configure it here. This would let the caller use any octokit plugins without our GitHub class needing to understand every configuration option.

For example, in the GitHub app we run, we provide an authenticated Octokit instance created by probot. This would mean the instantiator of the GitHub wrapper instance would need to configure the throttle plugin. Within this repo, it would mean configuring it only in the CLI.

@erezrokah
Copy link
Contributor Author

Going to close as stale and I'll re-open once I have time to address the comments

@erezrokah erezrokah closed this Oct 31, 2023
@erezrokah erezrokah deleted the feat/add_throttling branch October 31, 2023 08:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: s Pull request size is small.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants