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 retry and throttle octokit plugins #487

Merged

Conversation

achingbrain
Copy link
Contributor

Refactors the client to use @octokit/plugin-retry and @octokit/plugin-throttling as GitHub occasionally changes it's API and these plugins can abstract those changes away from this module.

  • Removes lib/definitions/rate-limit.js
  • Adds lib/definitions/retry.js and lib/definitions/throttle.js to handle retry/throttle settings
  • Updates tests to be more like GitHub (returing the correct rate limit response headers, etc)

Fixes #299 and semantic-release/semantic-release#2204
See also #480 and #378

Refactors the client to use [@octokit/plugin-retry](https://www.npmjs.com/package/@octokit/plugin-retry)
and [@octokit/plugin-throttling](https://www.npmjs.com/package/@octokit/plugin-throttling)
as GitHub occasionally changes it's API and these plugins can abstract
those changes away from this module.

- Removes `lib/definitions/rate-limit.js`
- Adds `lib/definitions/retry.js` and `lib/definitions/throttle.js` to handle retry/throttle settings
- Updates tests to be more like GitHub (returing the correct rate limit response headers, etc)

Fixes semantic-release#299 and semantic-release/semantic-release#2204
See also semantic-release#480 and semantic-release#378
@achingbrain
Copy link
Contributor Author

I'm trialing this fix in a bunch of repos - so far, so good.

If anyone else wants to give it a go you can use my temporary fork with this patch applied.

Change your @semantic-release/github dep in package.json to be:

"@semantic-release/github": "https://registry.npmjs.org/@achingbrain/semantic-release-github/-/semantic-release-github-0.0.0.tgz",

No other changes required.

@lapastillaroja
Copy link

It would be great if this PR can be reviewed and marged. I'm facing same problem in one of my projects

@rathpc
Copy link

rathpc commented May 12, 2022

Would love to get this reviewed and merged!! Can we update to resolve conflicts and tag some people for review?

@achingbrain
Copy link
Contributor Author

Conflicts are gone, not sure who would review - @travi @gr2m ?

@gr2m
Copy link
Member

gr2m commented May 19, 2022

I'm still on parental leave, and first week back to work will be busy, but I have it on my radar

@PocketlawErik
Copy link

@gr2m Wonderful to hear!

Running into this same problem right now. All your hard work is much appreciated! Thanks!

Please let me know if there is anything I can do to help 🙏❤️

@xorph
Copy link

xorph commented Sep 20, 2022

@gr2m Any ETA for a review of this PR? We have to deal a lot with the secondary rate limits in our release process lately.

@gr2m
Copy link
Member

gr2m commented Sep 20, 2022

No update yet, sorry. I'm still on parental leave and life keeps me busy. We focus the little time we have right now on #418

@stipsan
Copy link

stipsan commented Oct 27, 2022

Hi @gr2m, our usage of semantic-release at sanity.io is only increasing as it's such a boost to our productivity.

We're also impacted by this problem now and then, is there a way we can help @gr2m?
Either by reviewing and fixing things adjacent to ESM or would you prefer us to join the effort to land ESM #418? We've been doing a lot of ESM migrations in our own projects this year so it's familiar grounds to us 🥲

alandtse added a commit to alandtse/CrashLoggerSSE that referenced this pull request Nov 7, 2022
alandtse added a commit to alandtse/skyrim_vr_address_library that referenced this pull request Dec 8, 2022
alandtse added a commit to alandtse/fallout_vr_address_library that referenced this pull request Dec 8, 2022
ryanbas21 added a commit to cerebrl/forgerock-web-login-framework that referenced this pull request Dec 10, 2022
…the merge happens

this semantic-release/github#487 pr is a fix to the issue where the github
package hits a retry error. semantic release does publish though.
ryanbas21 added a commit to cerebrl/forgerock-web-login-framework that referenced this pull request Dec 10, 2022
use semantic release github fork while
semantic-release/github#487 is still waiting to
be merged. the issue is semantic release times out in release on the
github package but does publish.

also fixes the error where I put only one file in the package.json.
instead we copy the README.md from the root package into the package/
file before we publish
ryanbas21 added a commit to cerebrl/forgerock-web-login-framework that referenced this pull request Dec 10, 2022
…the merge happens

use semantic release github fork while
semantic-release/github#487 is still waiting to
be merged. the issue is semantic release times out in release on the
github package but does publish.

also fixes the error where I put only one file in the package.json.
instead we copy the README.md from the root package into the package/
file before we publish
ryanbas21 added a commit to cerebrl/forgerock-web-login-framework that referenced this pull request Dec 10, 2022
…the merge happens

use semantic release github fork while
semantic-release/github#487 is still waiting to
be merged. the issue is semantic release times out in release on the
github package but does publish.

also fixes the error where I put only one file in the package.json.
instead we copy the README.md from the root package into the package/
file before we publish
@achingbrain
Copy link
Contributor Author

@travi @gr2m is there any chance you could find some time to review this PR? I've updated it to resolve the recent conflicts, it solves a very real problem when using this module in monorepos that publish lots of modules in one go.

Please let me know if there's anything I can do to help you get it across the line?

@rlch
Copy link

rlch commented Dec 15, 2022

+1 -- we constantly run into rate-limiting issues in our delivery workflows. Getting quite tedious having to constantly monitor and manually retry failed actions.

achingbrain added a commit to ipfs/aegir that referenced this pull request Dec 15, 2022
nx trips over the tarball dep we have on `@semantic-release/github` when
it tries to parse project deps from the `package-lock.json` in CI so
disable lockfiles in monorepos.

This will no longer be necessary when semantic-release/github#487
is merged.
achingbrain added a commit to ipfs/aegir that referenced this pull request Dec 15, 2022
nx trips over the tarball dep we have on `@semantic-release/github` when
it tries to parse project deps from the `package-lock.json` in CI so
disable lockfiles in monorepos.

This will no longer be necessary when
semantic-release/github#487 is merged.
@rathpc
Copy link

rathpc commented Dec 19, 2022

sorry for pinging directly but we really need traction on this PR to get it merged. It has been 8 months already and this issue is plaguing many people.

Can one of you review and get this merged please? @boennemann @gr2m @kbrandwijk @keplersj @pvdlg @travi @UziTech

Copy link
Contributor

@Nargonath Nargonath 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 no semantic-release expert but the rest LGTM

lib/definitions/throttle.js Outdated Show resolved Hide resolved
@gr2m
Copy link
Member

gr2m commented May 13, 2023

Okay more like May 12 but that's how life goes sometimes. Thank you all for your patience and your help. I'm looking into it now

Renari added a commit to Renari/kikoeru that referenced this pull request May 13, 2023
@gr2m gr2m self-assigned this May 26, 2023
@gr2m
Copy link
Member

gr2m commented May 27, 2023

It's happening dot gif

If I had more time I'd add tests for lib/semantic-release-octokit.js but it's a tad complicated if we make it right, and tests will change in the upcoming ESM version, so chose to ... not.

I'm looking into the failing Node 14.17 tests. If y'all could give the latest version a try, I'd be much obliged

Comment on lines 5 to 6
retries: 3,
retryAfterBaseValue: 1000,
Copy link
Member

Choose a reason for hiding this comment

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

@gr2m
Copy link
Member

gr2m commented May 27, 2023

I published the current version as @gr2m/semantic-release-github-487@0.0.0-development for easier testing.

You can use aliasing to test it out. In your package.json

  "devDependencies": {
    "semantic-release": "^21.0.2",
    "@semantic-release/github": "npm:@gr2m/semantic-release-github-487@0.0.0-development"
  },

@gr2m
Copy link
Member

gr2m commented May 27, 2023

Alright I tested it some, it's Saturday, let's give this a go. I'm confident in this not introducing any new problems, but we can really only see once it's used out there.

@gr2m gr2m merged commit 3dc59ec into semantic-release:master May 27, 2023
4 checks passed
@gr2m
Copy link
Member

gr2m commented May 27, 2023

We want to thank you all for your patience and support, really 💐❤️

@github-actions
Copy link

🎉 This PR is included in version 8.0.8 🎉

The release is available on:

Your semantic-release bot 📦🚀

alandtse added a commit to alandtse/CrashLoggerSSE that referenced this pull request Sep 5, 2023
llyyr pushed a commit to llyyr/kikoeru that referenced this pull request Sep 22, 2023
soedirgo added a commit to supabase/postgres-meta that referenced this pull request Mar 6, 2024
We keep getting this error from semantic-release:

HttpError: You have exceeded a secondary rate limit. Please wait a few minutes before you try again.

This is supposedly fixed in semantic-release/github#487.
soedirgo added a commit to supabase/postgres-meta that referenced this pull request Mar 6, 2024
We keep getting this error from semantic-release:

HttpError: You have exceeded a secondary rate limit. Please wait a few minutes before you try again.

This is supposedly fixed in semantic-release/github#487.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

replace custom throttling code with @octokit/plugin-throttling