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: avoid multiple verifyConditions invocation #416

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

antongolub
Copy link
Contributor

closes #414

@antongolub antongolub force-pushed the avoid-multiple-verify-conditions-invocation branch from 266c970 to a8981db Compare October 27, 2021 19:17
@antongolub antongolub force-pushed the avoid-multiple-verify-conditions-invocation branch from a8981db to 66eb0e4 Compare October 27, 2021 19:20
@EricCrosson
Copy link

Nice approach!

@antongolub
Copy link
Contributor Author

antongolub commented Oct 31, 2021

@gr2m, @travi, could anyone take a look? We really need this fix asap. The current workaround is janky: we have to patch plugin inners to bypass npm registry rate limit.

@travi
Copy link
Member

travi commented Nov 4, 2021

We really need this fix asap.

i recommend that you fork and publish a version under you own scope if you need this quickly. we are a very small team of volunteer maintainers and have other priorities than those that impact your timeline. this looks like a small change, but we need to consider the implications that this could have on a process that has been working well for lots of users for a long time.

it appears that this is only a problem because you are using semantic-release in a monorepo, which is an unsupported use case. we recognize that there are tools that enable support on top of our core, but officially it is unsupported. we do want to make support possible through tools like the one your are using, but that support is a lower priority than our core functionality.

further, does this change even fully solve the problem, or does it just lower the chance of the problem? will this problem resurface if your monorepo grows to have even more packages?

@antongolub
Copy link
Contributor Author

antongolub commented Nov 4, 2021

We really appreciate and we are grateful to the semrel team for all the work that you do.

  1. Correct. We can continue unsafe and fragile monkey-patching. We can also fork the plugin. Let's just imagine. The problem affects all msr users with monorepos of ~10 packages and more. We'd like to save them from the need to reconfigure all of these projects. Moreover, some custom plugins are tightly bound / inherited from npm plugin, and the chain of changes for them becomes even larger.
  2. Technically yes. We trigger verify-auth as many times as the number of packages we're going to release. I guess multiple npm whoami calls run into the limit on the npmjs registry side.
  3. semrel does not provide monorepos support. That's clear and this is your vision, which we do not even dispute. That's why another volunteer community has been trying to implement this functionality on its own for several years.

@gr2m
Copy link
Member

gr2m commented Nov 18, 2021

fyi I filed a support ticked with GitHub about npm's responses in case of a rate limit. The ticket got escalated but I didn't hear back yet. I asked about how to recognize a rate-limitted response so that we can throttle/retry requests.

@antongolub
Copy link
Contributor Author

Nice! We're now experimenting with a custom npm plugin. Approach with verifyConditions promise memorization seems to be working.

@antongolub antongolub force-pushed the avoid-multiple-verify-conditions-invocation branch from 683f0da to e5544b1 Compare November 19, 2021 17:48
@gr2m
Copy link
Member

gr2m commented Nov 29, 2021

I received a response from support

You'll receive a response with status code 429 when there are too many requests. This will generally happen on the search endpoint, but also applies to package /downloads URLs.

@travi
Copy link
Member

travi commented Nov 29, 2021

You'll receive a response with status code 429 when there are too many requests.

would be nice to understand if there is a Retry-After header included, which is common with 429 responses. would it be worth following up to clarify that detail? i assume they did not provide a link to specific documentation around this?

This will generally happen on the search endpoint, but also applies to package /downloads URLs.

it sounds like this is occurring with the whoami request, which doesn't seem like it is specifically mentioned (since i assume that would hit a different endpoint than search or package-downloads). do you get the impression that this is applied more generally than that response suggests?

@moroine
Copy link

moroine commented Jan 27, 2022

Hi, do we have any update on this? I'm facing the same issue randomly using multi-semantic-release

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.

Broken verify-auth implementation
5 participants