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(README): correct docs on authentication #413

Merged
merged 2 commits into from
Oct 20, 2021
Merged

fix(README): correct docs on authentication #413

merged 2 commits into from
Oct 20, 2021

Conversation

swain
Copy link
Contributor

@swain swain commented Oct 19, 2021

Motivation

The documentation about authentication strategies seems to be incorrect.

The documentation implies that an NPM_TOKEN environment variable will take precedence over (and replace any token within) an .npmrc value:

Verify the presence of the NPM_TOKEN environment variable, create or update the .npmrc file with the token and verify the token is valid.

Based on the code here, that implication is not correct -- .npmrc files take precedence over both NPM_TOKEN and the legacy env variables. In particular, the return statement on line 29 is causing the environment variables to be ignored.

Changes

  • Update the README to clearly communicate that .npmrc files will take precedence over the NPM_TOKEN environment variable.
  • Added a test illustrating the behavior.

@swain swain marked this pull request as ready for review October 19, 2021 15:51
@gr2m
Copy link
Member

gr2m commented Oct 19, 2021

Did you verify this behavior or is it just based on your assessment of the code? I do agree with your assessment but just want to be sure. Do you think we could add a test to confirm your assumption? I couldn't find one which tests the behavior if both an auth token is set in .npmrc and NPM_TOKEN is set, I think it would need to go into https://github.com/semantic-release/npm/blob/master/test/set-npmrc-auth.test.js

@swain
Copy link
Contributor Author

swain commented Oct 19, 2021

@gr2m Great call-out. I've added a new commit with an illustrating test.

Copy link
Member

@gr2m gr2m left a comment

Choose a reason for hiding this comment

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

Thanks, great PR 👍🏼

@gr2m gr2m changed the title fix: correct docs on authentication fix(README): correct docs on authentication Oct 20, 2021
@gr2m gr2m merged commit f089d9d into semantic-release:master Oct 20, 2021
@github-actions
Copy link

🎉 This PR is included in version 8.0.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

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.

2 participants