-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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: publish --dry-run require login & publishConfig loaded after registry check #2445
Conversation
Should I split the last fix as a separate PR? Currently the coverage error will affect other PR made in this year. |
Can you please remove the dry-run and birthday tests, since they're unrelated to the issue of merging in Also, this needs a test that fails without the patch, and passes with the patch. Thanks! |
Sure, I see there's 7.4.0, will rebase & change the code. |
7bd1549
to
d43156e
Compare
@dr-js Thank you for doing this! Let me know if I can assist in any way. |
@ruyadorno There is one test added for the fix, should I add some other type of tests for this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 looks great, thanks @dr-js !
d43156e
to
d2f8af2
Compare
Fix(already fixed)publish --dry-run
with v7.3.0 will require login (ENEEDAUTH).Fix
publishConfig
frompackage.json
loaded after the registry check.This PR delayed publish login check till config merge (from the code should wait for 1 more file read or pacote manifest load), and added publish read registry only from publishConfig test.
References
Fixes #2411
Related PR also fix dry-run #2422