reporter-web-app: Support bootstrapping NPM and Yarn #862
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fviernau
force-pushed
the
fix-npm-bootstrap
branch
from
666bcd1 to
40fbb3d
Compare
fviernau
force-pushed
the
fix-npm-bootstrap
branch
from
40fbb3d to
8241767
Compare
mnonnenmacher
requested changes
Sep 13, 2018
reporter-web-app/build.gradle
Outdated
|
|
||
| // Installing NPM is needed for installing Yarn because | ||
| // the Gradle Node plugin install Yarn via NPM. | ||
| npmVersion = '6.4.0' |
There was a problem hiding this comment.
Shouldn't we use the latest 6.4.1?
There was a problem hiding this comment.
@fviernau and I agreed to use 6.4.0 to align with what we currently use in .appveyor.yml and .travis.yml.
reporter-web-app/build.gradle
Outdated
| @@ -5,7 +5,11 @@ plugins { | |||
| node { | |||
| version = '8.11.4' | |||
| yarnVersion = '1.9.4' | |||
|
|
|||
| // Installing NPM is needed for installing Yarn because | |||
There was a problem hiding this comment.
Commit message: Second paragraph is weird, what about: "Also set a fixed NPM version to ensure reproducible builds." Also it's a bit weird to have [2] before [1]. And nit: "Support" should be capitalized in the title for consistency with other commits.
There was a problem hiding this comment.
More commit message nits: "... installs Yarn ...", "... to not negatively ..."
sschuberth
changed the title
sschuberth
requested changes
Sep 13, 2018
reporter-web-app/build.gradle
Outdated
| // Installing NPM is needed for installing Yarn because | ||
| // the Gradle Node plugin install Yarn via NPM. | ||
| npmVersion = '6.4.0' | ||
| // Setting download flag is required for bootstrapping NPM |
There was a problem hiding this comment.
Missing dot at end of sentence.
mnonnenmacher
changed the title
fviernau
force-pushed
the
fix-npm-bootstrap
branch
2 times, most recently
from
4d758ab to
d29babb
Compare
NPM needs to be installed in order to bootstrap Yarn since the Gradle Node plugin installs Yarn via NPM, see also [1] [2]. For this installation to work the download flag needs to be set. Also set a fixed NPM version for the reproducibility of builds. [1] srs/gradle-node-plugin#175 [2] srs/gradle-node-plugin#186
fviernau
force-pushed
the
fix-npm-bootstrap
branch
from
d29babb to
e7ce944
Compare
mnonnenmacher
approved these changes
Sep 14, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
NPM needs to be installed in order to bootstrap Yarn
since the Gradle Node plugin install Yarn via NPM. For
this installation to work the download flag needs to
be set.
Also for not nefatively affecting reproducibility of
build setting a fixed NPM version.
See also the related issue [2] and open pull request [1].
[1] srs/gradle-node-plugin#186
[2] srs/gradle-node-plugin#175
This change is