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.

Sign up for GitHub

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

Jump to bottom

feat(http): Force default rate limits for some known hosts #30207

Merged
merged 16 commits into from
Jul 22, 2024
Merged

feat(http): Force default rate limits for some known hosts #30207

merged 16 commits into from
Jul 22, 2024

Conversation

zharinov
Copy link
Collaborator

@zharinov zharinov commented Jul 16, 2024 •
edited
Loading

Changes

  • Default limits can be overriden only towards more restriction
  • 8 requests per second for rubygems.org
  • 1 request per second for https://crates.io/api
  • 16 concurrent requests for each host by default

Context

Documentation (please check one with an [x])

  • I have updated the documentation, or
  • No documentation update is required

How I've tested my work (please select one)

I have verified these changes via:

  • Code inspection only, or
  • Newly added/modified unit tests, or
  • No unit tests but ran on a real repository, or
  • Both unit tests + ran on a real repository

HonkingGoose
HonkingGoose reviewed Jul 17, 2024
View reviewed changes
Copy link
Collaborator

@HonkingGoose HonkingGoose left a comment

Choose a reason for hiding this comment

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

Do you have a source for the new rate limit numbers?

Maybe the project has documentation that says: "please limit to x requests per y time"?

viceice
viceice reviewed Jul 17, 2024
View reviewed changes
Copy link
Member

@viceice viceice left a comment

Choose a reason for hiding this comment

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

otherwise LGTM

lib/util/http/queue.ts Outdated Show resolved Hide resolved
lib/util/http/throttle.ts Outdated Show resolved Hide resolved
@zharinov zharinov requested review from viceice and rarkins July 17, 2024 20:13
HonkingGoose
HonkingGoose suggested changes Jul 18, 2024
View reviewed changes
lib/util/http/rate-limit.spec.ts Outdated Show resolved Hide resolved
lib/util/http/rate-limit.spec.ts Outdated Show resolved Hide resolved
lib/util/http/rate-limit.spec.ts Outdated Show resolved Hide resolved
@zharinov @HonkingGoose
Apply suggestions from code review
88946da 
Co-authored-by: HonkingGoose <34918129+HonkingGoose@users.noreply.github.com>
HonkingGoose
HonkingGoose approved these changes Jul 18, 2024
View reviewed changes
Copy link
Collaborator

@HonkingGoose HonkingGoose left a comment

Choose a reason for hiding this comment

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

I'm happy with the test descriptions now. I'll let the maintainers review the rest.

viceice
viceice reviewed Jul 18, 2024
View reviewed changes
lib/util/http/rate-limits.ts Show resolved Hide resolved
lib/util/http/rate-limits.ts Outdated Show resolved Hide resolved
@zharinov
Copy link
Collaborator Author

This PR seems to be the only blocker for #27369

@viceice viceice added this pull request to the merge queue Jul 22, 2024
Merged via the queue into renovatebot:main with commit 8d183d6 Jul 22, 2024
38 checks passed
@viceice viceice deleted the feat/known-hosts-rate-limits branch July 22, 2024 17:06
@renovate-release
Copy link
Collaborator

🎉 This issue has been resolved in version 37.439.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@HonkingGoose HonkingGoose HonkingGoose approved these changes

@rarkins rarkins rarkins approved these changes

@viceice viceice viceice approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Apply default rate limits for hosts/registries with known rate limit needs
5 participants
@zharinov @renovate-release @viceice @rarkins @HonkingGoose