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 various commands asking for a password #3862

Merged
merged 1 commit into from
Jul 17, 2017
Merged

Fix various commands asking for a password #3862

merged 1 commit into from
Jul 17, 2017

Conversation

johnf
Copy link
Contributor

@johnf johnf commented Jul 8, 2017

Summary

Commands like publish were asking for a password instead of using the token in .npmrc.

The core code path that was causing the issue is https://github.com/yarnpkg/yarn/blob/master/src/registries/npm-registry.js#L218-L221

// If sending a request to the Yarn registry, we must also send it the auth token for the npm registry
if (baseRegistry === YARN_REGISTRY) {
  registries.push(DEFAULT_REGISTRY);
}

The problem was baseRegistry = "https://registry.yarnpkg.com" while YARN_REGISTRY = "https://registry.yarnpkg.com/". Notice the trailing slash.

I've reverted the code added in 3382071#diff-b053bee294c216269844e5874039b6caR172
which solves the problem. Unclear to me why it was added though.

Test plan

yarn publish no longer asks for a password.

@arcanis
Copy link
Member

arcanis commented Jul 9, 2017

Ping @lumaxis? Do you remind the rational behind this change?

@arcanis arcanis self-assigned this Jul 9, 2017
@johnf
Copy link
Contributor Author

johnf commented Jul 15, 2017

@lumaxis @arcanis ping

Should I rewrite this to just make this bit of code above work instead of the current change?
This would remove any possible side effects.

@lumaxis
Copy link
Contributor

lumaxis commented Jul 17, 2017

@arcanis @johnf Heya 👋 Sorry for the slow response on this, was out sick last week and am only slowly catching up on email.
I didn't add this specific change myself as I was also partly building on top of the work of someone else. 😕

@arcanis
Copy link
Member

arcanis commented Jul 17, 2017

No problem, thanks! Let's merge this then, all tests are green so we can be reasonably confident this won't break anything :)

@arcanis arcanis merged commit f7aa742 into yarnpkg:master Jul 17, 2017
@johnf johnf deleted the fix_token_auth branch August 15, 2017 08:12
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.

3 participants