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

Remove npm dependency #2670

Merged
merged 1 commit into from
Nov 10, 2020
Merged

Remove npm dependency #2670

merged 1 commit into from
Nov 10, 2020

Conversation

esanzgar
Copy link
Contributor

Rely exclusively on yarn.

@esanzgar esanzgar requested a review from robertknight October 23, 2020 10:15
@codecov
Copy link

codecov bot commented Oct 23, 2020

Codecov Report

Merging #2670 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2670   +/-   ##
=======================================
  Coverage   97.73%   97.73%           
=======================================
  Files         199      199           
  Lines        7517     7517           
  Branches     1648     1648           
=======================================
  Hits         7347     7347           
  Misses        170      170           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0e74bb6...3fbc1a2. Read the comment docs.

Copy link
Member

@robertknight robertknight left a comment

Choose a reason for hiding this comment

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

I had a couple of queries about the yarn publish change given the comments just above the changed line.

Jenkinsfile Outdated Show resolved Hide resolved
scripts/wait-for-npm-release.sh Show resolved Hide resolved
@robertknight robertknight changed the title Remove npm depency Remove npm dependency Oct 27, 2020
@esanzgar esanzgar force-pushed the remove-npm-dependency branch from 90d068a to 6f463b0 Compare October 28, 2020 13:31
@esanzgar esanzgar requested a review from robertknight October 28, 2020 13:37
Copy link
Member

@robertknight robertknight left a comment

Choose a reason for hiding this comment

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

I had a query about the ENV_TOKEN environment variable.

Jenkinsfile Outdated Show resolved Hide resolved
@esanzgar esanzgar force-pushed the remove-npm-dependency branch from 6f463b0 to ef28894 Compare November 2, 2020 12:11
@esanzgar esanzgar requested a review from robertknight November 2, 2020 12:17
@esanzgar
Copy link
Contributor Author

esanzgar commented Nov 2, 2020

I had a query about the ENV_TOKEN environment variable.

I corrected it to NPM_TOKEN.

More here about the NPM_TOKEN:
https://docs.npmjs.com/using-private-packages-in-a-ci-cd-workflow

Copy link
Member

@robertknight robertknight left a comment

Choose a reason for hiding this comment

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

I did a little more research this morning and I have another query about the NPM_TOKEN line.

Jenkinsfile Outdated
// See https://github.com/yarnpkg/yarn/pull/3391.
sh "echo '//registry.npmjs.org/:_authToken=${env.NPM_TOKEN}' >> \$HOME/.npmrc"
sh "npm publish --tag ${npmTag}"
sh "NPM_TOKEN=${env.NPM_TOKEN} yarn publish --no-interactive --tag ${npmTag} --new-version=${newPkgVersion}"
Copy link
Member

Choose a reason for hiding this comment

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

Have you confirmed that yarn publish definitely uses NPM_TOKEN if it is set this way and .npmrc is not present?

From what I can tell looking at the yarn issues there is no built-in support for NPM_TOKEN in yarn itself. Instead there is a feature where .npmrc can contain references to environment variables and there is a convention to write //registry.npmjs.org/:_authToken=${NPM_TOKEN} into the .npmrc file. See yarnpkg/yarn#1207.

I did however find a reference to NPM_AUTH_TOKEN (and also YARN_AUTH_TOKEN) in the yarn sources: https://github.com/yarnpkg/yarn/blob/a4708b29ac74df97bac45365cba4f1d62537ceb7/src/cli/commands/login.js#L64

The other issue I see here is that a .npmrc file could currently be left over from a previous build and I'm not sure what happens if the auth token is set in both places.

The simplest remedy I can see here is to keep the "sh echo '..." line unless you're sure that setting NPM_TOKEN on its own works, and just change the line that invokes npm publish.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I confirm that it works with NPM_TOKEN.

I have reverted the sh "echo... line.

Rely exclusively on `yarn`.
@esanzgar esanzgar force-pushed the remove-npm-dependency branch from ef28894 to 3fbc1a2 Compare November 6, 2020 12:58
@esanzgar esanzgar requested a review from robertknight November 6, 2020 12:59
Copy link
Member

@robertknight robertknight left a comment

Choose a reason for hiding this comment

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

LGTM

@esanzgar esanzgar merged commit 0079b15 into master Nov 10, 2020
@esanzgar esanzgar deleted the remove-npm-dependency branch November 10, 2020 09:10
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.

2 participants