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 Travis CI not detecting uncommitted changes to package-lock.json #13462

Closed
wants to merge 1 commit into from

Conversation

noisysocks
Copy link
Member

@noisysocks noisysocks commented Jan 24, 2019

Fixes issue identified in #12378 (comment).

npm run check-local-changes was not detecting uncommited changes to package-lock.json because Travis CI was set to use npm ci instead of npm install.

Having Travis cache node_modules ensures that there is no regression of CI performance, see #13103 (comment).

@noisysocks noisysocks requested review from aduth and ntwb January 24, 2019 00:29
@noisysocks noisysocks added the [Type] Build Tooling Issues or PRs related to build tooling label Jan 24, 2019
@aduth
Copy link
Member

aduth commented Jan 24, 2019

From https://docs.npmjs.com/cli/ci.html:

If dependencies in the package lock do not match those in package.json, npm ci will exit with an error, instead of updating the package lock.

Do we just need to be handling the failing exit?

@noisysocks
Copy link
Member Author

TIL. Moving the command to script is probably all that we need to do here then.

`npm run check-local-changes` was not detecting uncommited changes to
`package-lock.json` because Travis CI was set to ignore nonzero exit
codes coming from `npm ci`
@noisysocks noisysocks force-pushed the fix/check-local-changes branch from a5f8c07 to 1254bb9 Compare January 24, 2019 05:57
@ntwb
Copy link
Member

ntwb commented Jan 24, 2019

Moving the command to script is probably all that we need to do here then.

https://docs.travis-ci.com/user/job-lifecycle#breaking-the-build

If any of the commands in the first four phases of the job lifecycle return a non-zero exit code, the build is broken:

If before_install, install or before_script returns a non-zero exit code, the build is errored and stops immediately.
If script returns a non-zero exit code, the build is failed, but continues to run before being marked as failed.

I don't think we want that in script, no point continuing on with the CI build if it will end up with a failed status anyway, better of in one of the before_install, install or before_script sections to halt the build immediately when errored

@gziolo
Copy link
Member

gziolo commented Jan 24, 2019

Can we land lock file changes first? :)

@aduth
Copy link
Member

aduth commented Jan 25, 2019

@ntwb From those texts, it begs the question then: Why isn't it currently failing, if npm ci is meant to exit with an error when a package-lock.json and when the Travis build is meant to error and stop immediately when a script in install errors?

@ntwb
Copy link
Member

ntwb commented Jan 31, 2019

I created #13613 to test the above, it is working as expected for me with the current npm ci step in the install section https://github.com/WordPress/gutenberg/blob/master/.travis.yml#L34-L35

The job errors when there is a change in package.json file but the package-lock.json file has not been updated with that change:

$ nvm install --latest-npm
Found '/home/travis/build/WordPress/gutenberg/.nvmrc' with version <lts/*>
Downloading and installing node v10.15.1...
Local cache found: $NVM_DIR/.cache/bin/node-v10.15.1-linux-x64/node-v10.15.1-linux-x64.tar.xz
Checksums match! Using existing downloaded archive $NVM_DIR/.cache/bin/node-v10.15.1-linux-x64/node-v10.15.1-linux-x64.tar.xz
Now using node v10.15.1 (npm v6.4.1)
Attempting to upgrade to the latest working version of npm...
0: command not found
0: command not found
* Installing latest `npm`; if this does not work on your node version, please report a bug!
/home/travis/.nvm/versions/node/v10.15.1/bin/npm -> /home/travis/.nvm/versions/node/v10.15.1/lib/node_modules/npm/bin/npm-cli.js
/home/travis/.nvm/versions/node/v10.15.1/bin/npx -> /home/travis/.nvm/versions/node/v10.15.1/lib/node_modules/npm/bin/npx-cli.js
+ npm@6.7.0
added 52 packages from 7 contributors, removed 13 packages and updated 41 packages in 6.522s
* npm upgraded to: v6.7.0
0.66s$ npm ci
npm ERR! cipm can only install packages when your package.json and package-lock.json or npm-shrinkwrap.json are in sync. Please update your lock file with `npm install` before continuing.
npm ERR! 
npm ERR! 
npm ERR! Invalid: lock file's lerna@3.4.3 does not satisfy lerna@3.10.7
npm ERR! 
npm ERR! A complete log of this run can be found in:
npm ERR!     /home/travis/.npm/_logs/2019-01-31T10_59_39_229Z-debug.log
The command "npm ci" failed and exited with 1 during .

Your build has been stopped.

I tried to add fast_finish: true so that the entire build will terminate when a job errors such as the .1 job above, unfortunately, that doesn't work as I expected.

I'm not sure why the original issue where this issue was detected did not work, but in my testing which was to use only a modified package.json file without any change or update to the package-lock.json lock file Travis CI and npm ci are working as expected for me.

@gziolo gziolo added the Needs Decision Needs a decision to be actionable or relevant label Feb 7, 2019
@aduth
Copy link
Member

aduth commented Apr 24, 2019

From readings of the documentation and prior testing, it seems like this should already be working as intended, or at least the change proposed here would likely not have an impact.

The specific fix to package-lock.json appears to have been addressed separately.

@aduth aduth closed this Apr 24, 2019
@aduth aduth deleted the fix/check-local-changes branch April 24, 2019 20:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Decision Needs a decision to be actionable or relevant [Type] Build Tooling Issues or PRs related to build tooling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants