-
-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Improve npm in ci #1455
Improve npm in ci #1455
Conversation
I'm also thinking about if we can just pin the nodejs version? Since I didn't see a requirement to install the latest version. |
.travis.yml
Outdated
@@ -18,6 +18,7 @@ cache: | |||
- $HOME/.cabal | |||
- $TRAVIS_BUILD_DIR/.cache | |||
- $TRAVIS_BUILD_DIR/node_modules | |||
- $TRAVIS_BUILD_DIR/versions |
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.
The benefit here would be "the amount of time it takes to untar the cache" - I intentionally did not cache this part - we might make all sorts of changes to an installed node version over the course of tests, and making tests deterministic is far more important than making them fast. Let's remove this commit.
@@ -17,6 +17,7 @@ cache: | |||
- $HOME/.ghc | |||
- $HOME/.cabal | |||
- $TRAVIS_BUILD_DIR/.cache | |||
- $TRAVIS_BUILD_DIR/node_modules |
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.
If we're going to do this, then after npm install
, we need to always run npm prune
.
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.
Sure.
d56e21e
to
d970447
Compare
.travis.yml
Outdated
before_install: | ||
- $SHELL --version 2> /dev/null || dpkg -s $SHELL 2> /dev/null || which $SHELL | ||
- curl --version | ||
- wget --version | ||
- if [ -n "${SHELLCHECK-}" ]; then cabal update && cabal install transformers-0.4.3.0 ShellCheck && shellcheck --version ; fi | ||
- if [ -z "${SHELLCHECK-}" ]; then nvm install node && npm install && npm ls urchin doctoc; fi | ||
- if [ -z "${SHELLCHECK-}" ]; then nvm install node && npm install && npm prune && npm ls urchin doctoc; fi |
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.
not a blocker, but it occurs to me that this step should probably go in install
, not before_install
?
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.
I agree, that's the place what it already been for a while, I can send another commit to move both npm
and cabel
into install
section, what do you think?
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.
Sounds good!
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.
done
This should make the CI faster.