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

Best practices for package-lock.json files on Travis #183

Open
chosak opened this issue Sep 21, 2018 · 5 comments
Open

Best practices for package-lock.json files on Travis #183

chosak opened this issue Sep 21, 2018 · 5 comments

Comments

@chosak
Copy link
Member

chosak commented Sep 21, 2018

How should we handle Travis changing package-lock.json files when we build Python wheels there?

Our cfgov-refresh satellite packages are configured to use Travis to build installable Python wheel files when a new tag/release is created on GitHub. We use the Travis GitHub Releases feature for this.

The way we want the workflow to happen:

  1. Developer manually creates a tag/release on GitHub, let's say to version 1.0.
  2. New tag triggers a Travis build, as Travis is configured to build on tags to master.
  3. Travis build includes running the frontend build (if there is one) and then creation of a Python wheel including Python code and the built frontend assets.
  4. Python wheel, which should be properly named with the new git tag, gets uploaded to GitHub and attached to the release.

Importantly, we use the setuptools-git-version package to set the Python package version based on the git status. The way this works is that if your git status is clean and on a tag, the package version (and wheel file) will be versioned with that tag, e.g. on tag 1.0 it'd generate a wheel named something like package-1.0-py2-none-any.whl. If your local git status has diverged from a tag, either by having additional commits or by having locally modified files the version will indicate as such, e.g. package-1.0.dev3+438a0f9-py2-none-any.whl (the 3 indicates the number of commits since the tag, and the 438a0f9 is some kind of hash of the changes).

If Travis were just checking out code at a tag and building a wheel, we'd expect the wheel name to always be clean and perfectly match the tag. You wouldn't expect the Travis build to either make any commits or "dirty" the local git environment by making file changes -- but it can, in the case of package-lock.json.

If the package-lock.json file as generated by the last developer who committed doesn't match the versions installed by Travis, the file will be modified locally. This can happen even if developers do everything right, due to differences between Mac and Linux (see npm/npm#17722 for details). This in turn causes a dirty git workspace, which causes incorrect Python wheel names.

We've implemented some workarounds for this on a few of our satellites, but I'm not sure what the right approach is. I'm opening this issue to solicit feedback from FEWDs and to reach a team consensus.

Approach 1: ignore package-lock.json

If we have Travis run npm config set package-lock false, npm will ignore the package-lock.json file completely. It won't read from it and won't update it, and will only use the package.json file. One place we do this now is on TDP. This "fixes" the dirty wheel name issue, as we're no longer modifying a local file, but it isn't perfect.

# .travis.yml
install:
  # Tell npm to ignore package-lock.json to avoid making local file
  # changes that would dirty the git workspace.
  - npm config set package-lock false
  - npm install
script:
  # Other testing steps here.
  - python setup.py bdist_wheel

The problem with this approach is that the Travis build might use versions that are very different from what developers use locally. It means that the package-lock.json file no longer accurately describes the versions of things used to build the released/deployed wheel file of the package. If package.json contains unconstrained versions (e.g. "cf-core": "^4.2.0") then there's no restriction on what Travis might use (although, perhaps this is a problem with defining things that way).

Approach 2: use package-lock.json but discard local changes

Instead of disabling use of package-lock.json, let Travis use it and modify it, but then discard it before building the Python wheel. This file shouldn't be getting included in the wheel anyway, so it matters less.

# .travis.yml
install:
  - npm install
script:
  # Other testing steps here.
  # We don't want changes to package-lock.json to show up as local git
  # changes when the Python wheel tag is determined.
  - git checkout package-lock.json
  - python setup.py bdist_wheel

This might be better than the first approach because it's likely that Travis will use more of what is in package-lock.json, but there's still no guarantee that it would use all of what's in there. And, like the first approach, the package-lock.json still doesn't necessarily describe the exact versions that were used to build the released/deployed wheel.

Approach 3: let Travis update package-lock.json

One potential idea would be to let Travis maintain package-lock.json. On builds, it could make necessary changes and then commit them back to the repo. This would ensure that the version of package-lock.json in the repo always exactly matches what was used to build the release.

The ccdb5-ui repo currently has an implementation of this, although I've submitted a PR that removes this to make it work the same way as the rest of our satellites.


Which of these are least bad? I know that @wpears has suggested moving to yarn as an alternative to npm. Any other ideas to consider @cfpb/cfgov-frontends?

@cfarm
Copy link
Contributor

cfarm commented Sep 24, 2018

I like approach 3, it maintains the way that package-lock.json is meant to work and keeps its changes in version control. We already have bots committing changes to our repos as part of release processes, so there is precedent for this type of solution.

@wpears
Copy link
Member

wpears commented Sep 24, 2018

Here's the (pretty outdated) yarn PR. I'm still sold that yarn w/ the offline cache is currently the best packaging solution for all our various use cases. Also, I'm a fan of the clean-cut switchover, which I don't think will be too disruptive or burdensome.

Also, if cfpb/consumerfinance.gov#4482 is merged in, it will be even less disruptive, as yarn can be included in the docker image and not require an install for docker users.

@willbarton
Copy link
Member

@wpears we'd have to update all the satellite apps to use yarn, wouldn't we? How trivial/non-trivial is that?

@ascott1
Copy link
Member

ascott1 commented May 20, 2019

Would the use of npm ci offer a potential simple solution? Specifically, should we configure the front-end build step to use npm ci in place of npm install in our Travis builds?

I'm not sure that it would resole the underlying issue that @chosak has flagged:

This can happen even if developers do everything right, due to differences between Mac and Linux

Here's the description of how it works:

This command is similar to npm-install, except it’s meant to be used in automated environments such as test platforms, continuous integration, and deployment – or any situation where you want to make sure you’re doing a clean install of your dependencies. It can be significantly faster than a regular npm install by skipping certain user-oriented features. It is also more strict than a regular install, which can help catch errors or inconsistencies caused by the incrementally-installed local environments of most npm users.

In short, the main differences between using npm install and npm ci are:

  • The project must have an existing package-lock.json or npm-shrinkwrap.json.
  • 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.
  • npm ci can only install entire projects at a time: individual dependencies cannot be added with this command.
  • If a node_modules is already present, it will be automatically removed before npm ci begins its install.
  • It will never write to package.json or any of the package-locks: installs are essentially frozen.

@anselmbradford
Copy link
Member

We've moved to yarn and have yarn.lock files now. Perhaps this issue can be updated/closed?

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

No branches or pull requests

6 participants