search

Agoric / agoric-sdk

monorepo for the Agoric Javascript smart contract platform
Apache License 2.0
327 stars 206 forks source link

check that yarn.lock doesn't change during install #660

Closed warner closed 1 year ago

warner commented 4 years ago

I did a git pull today, and ran yarn just in case, and was surprised to see that it modified yarn.lock. Usually we make a practice of committing an updated yarn.lock in the same PR as the changes to package.json which provoked the lockfile changes, but something went wrong in a recent PR and they diverged a bit.

I was thinking of adding a CI check that the contents of yarn.lock before and after the execution of yarn are the same. What do people think?

We commit our lockfile so that (at least notionally) humans are the ones making changes to our dependency tree, in PRs where they can be reviewed, rather than having the set of packages we use get selected at the last moment, on some downstream developer's machine. This means updates to packages must be done as commits, where perhaps they'll be a bit more deliberate.

Changes to yarn.lock don't commute very well, however. If one PR changes a package.json, and thus also changes yarn.lock, and that PR must be rebased past some other changes to yarn.lock, it's usually really hard to deal with the merge conflict. My usual approach is to put the yarn.lock change in its own commit, at the end of my PR, so that if someone (maybe me) needs to rebase it, I can snip off that commit, rebase everything else, and then re-run yarn to regenerate a new final commit. This is a hassle, and I can't expect everyone to follow this practice.

I'm not sure what the best practices are here. I imagine other projects have run into this before, and that there might be tooling available (just as "dependabot" can file PRs to update yarn.lock -type files). Or maybe we should reconsider keeping this lockfile in version control?

erights commented 4 years ago

I did a git pull today, and ran yarn just in case, and was surprised to see that it modified yarn.lock.

Even if yarn.lock were perfectly correct at the time of that commit, can't this happen anyway if new versions of dependent packages have been published on npm, and yarn finds a more recent version is a better solution to the simultaneous package.json constraints than the choices that were available at the time?

erights commented 4 years ago

We commit our lockfile so that (at least notionally) humans are the ones making changes to our dependency tree, in PRs where they can be reviewed, rather than having the set of packages we use get selected at the last moment, on some downstream developer's machine.

Is it realistic to expect that a human reviewer will ever manually read a yarn.lock file when nothing mysteriously bad is happening? Rather, I see yarn.lock as a way to snapshot the concrete choices that were made at that time, when things were presumably working.

This enables us to ensure that this tested working state is reproduced in production. And if something bad happens later in development, we have the ability to perfectly reproduce past working states for comparison.

(Figuring these things out on yarn.lock is good practice for the issues that will arise managing TOFU-based least authority linkage policies. I suspect that'll be harder but not way harder.)

mhofman commented 1 year ago

Fixed in https://github.com/Agoric/agoric-sdk/pull/7128 and previously in https://github.com/Agoric/agoric-sdk/commit/05afb653d54973ddf405dd56e84b9c68f00c4461 and https://github.com/Agoric/agoric-sdk/pull/5895