feat(checksums): add integrity field with sha512 authentication to yarn.lock #5042
Conversation
|
|
Regarding CI issues: The issues on previous versions of node in appveyor seem to be mostly due to different behaviour of the Buffer object. These can be trivially remedied and I'll get to them in the next few days. |
|
I recommend you make sure your tests also check that this behaves if you hand it an I also recommend you add tests to make sure |
|
Thanks @zkat ! I'll add those test cases (the |
|
Hey @zkat - I added the test cases (and made the relevant code changes) you mentioned. If you have the time and feel like commenting, I'd very much appreciate it! @BYK - the CircleCI builds for node6 and node8 are failing. I think this is because they cache |
|
@imsnif thanks a lot for the PR and heads up about the cache issues. I've submitted #5060 to fix the invalid cache setup. You can merge from that and see if it fixes your builds. Or wait until it lands on master and merge from there. Right now, as long as Linux tests pass, I'm okay until right before merging. |
|
@imsnif merged to master. Merge from there and your builds should pass. I'll review the code later. I've also pinged a few more people since this is a fairly large change. Thanks a lot for all the effort you've put into this. |
|
@BYK @arcanis @bestander - Hello friends! I hope you don't mind me making this public ping regarding the review progress. I'd very much like to continue working on this and see it merged. Hoping we'll find time soon! |
|
@imsnif, thanks for pinging, this is a right thing to do, no need to apologize. |
|
Hey @bestander , thanks! |
The hashes are added automatically when you run the latest Yarn version. So good to get this merged so it won't show up as added lines when people run `$ yarn` in the repo. It doesn't change any functionality. This is merely to reduce noise. More info: yarnpkg/yarn#5042
Field was added in yarn@1.1.0. See yarnpkg/yarn#5042 for more information.
yarn >= 1.10.0 adds sha512 integrity checks to the yarn.lock file. For more details see yarnpkg/yarn#5042. The behaviour is backwards compatible.
The field was added in yarn@1.1.0. See yarnpkg/yarn#5042 for more information. It fully compatible with previous versions of Yarn
Yarn 1.10.x adds these by default any time you run `yarn`. There's not a lot in the way of release notes, but see yarnpkg/yarn#5042 . Sounds like a good idea! And they're said to be backward compatible with older versions of `yarn`, as you'd hope. So, go ahead and add them.
Added in yarn v1.10.0, see yarnpkg/yarn#5042
Summary
Continuing the discussion in: #816
This turned out to be a feature that touched quite a few parts of the code base. The diff is a little big, mostly because this required (minor) changes in a lot of the tests and fixtures (since we've added a new field to yarn.lock).
I tried to adjust the commits to "tell a story", to make this PR easier to follow. That being said, if anything is unclear, I would be happy to explain or adjust it to make it more understandable (I have made an effort to do so in the code as much as possible).
Since this is not a trivial change, what guided me through it is the will to concentrate on it as much as possible without changing other things. Thus for example, I did not change the
validationStreamin other non-tarball fetchers to use thessrimodule, even though it creates an inconsistency, so as not to distract from the feature at hand. I think this PR does introduce some wanted changes (such as the stream example) but I believe they are all very minor, and would be best fixed in additional refactoring PRs (that I or someone else will perform once this is approved and merged).Test plan
I added both integration and unit tests where appropriate.
Please note though: since there are no tests for
publish(unless I missed them!) I did not add tests, because I felt it would be out of scope. I tested this manually withverdaccioand it worked. I would urge the reviewer to do the same to make sure I was not mistaken!