This repository has been archived by the owner on Sep 9, 2020. It is now read-only.
Vendor verification #1912
Merged
Vendor verification #1912
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sdboyer
requested review from
carolynvs,
darkowlzz,
ibrasho and
jmank88
as code owners
|
This is great. Went through the dep changes and it makes sense. Since this removes inputs-digest, I've added #1496 to the fixes list. |
|
@darkowlzz awesome, thank you! |
First steps towards a better in-sync checking system that does not rely on a merge-conflict-prone explicit hash digest.
This mostly supplants the hash comparison-based checking, though it's still in rough form.
This is the first step towards being able to a more expansive type - one that carries the pruning and digest information - directly within the existing Lock interface.
This is a start at isolating verification logic into a discrete package. Not sure how far we'll be able to make this go without creating some import loops.
THis includes changes across both dep and gps towards transparently working with vendor trees that are both configurably pruned and verifiable.
We're now relying entirely on real validation - no more hash digest comparisons and meaningless conflicts!
Both of these subsystems make more sense in the verification package than in gps itself.
This encompasses the first pass at the new, more abstracted diffing system, and the DeltaWriter implementation on top of it. Tests are needed, but cursory testing indicates that we successfully capture all types of diffs and regenerate only the subset of projects that actually need to be touched.
Also convert the SafeWriter to use LockDelta.
Add output to all of the information we assemble when checking if the Lock satisfies the current input set. Also some refactoring of the ctx.LoadProject() process to have fewer partial states.
sdboyer
force-pushed
the
verify-vendor
branch
3 times, most recently
from
b17ee5f to
2478e99
Compare
Tests are now almost completely working, after updating all the outputs to the new lock format. There is also an assortment of other fixes in here, mostly related to fixing nil pointer panics, that were uncovered by fixing up these tests.
There was no real need to delineate between Lock and LockWithImports. The old Lock had the InputsDigest concept, which was even less feasible for theoretical implementations of Lock to have, so this can't possibly be more harmful.
Also a bunch of docs for the verify package.
Some tests were complaining of a data race when writing to ctx.Out on a vendor verification failure. It's not clear why those tests were failing, but the complaint became a forcing function to refactor the previously-sloppy use of a channel and os.Exit(1) within a goroutine to encapsulation in a method on dep.Project.
|
OK, in we go! |
This was referenced Jul 11, 2018
This was referenced Jul 11, 2018
Closed
5 tasks
This was referenced Jul 23, 2018
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this do / why do we need it?
This makes automated vendor verification a reality. There's a lot that goes along with that, but everything in this PR falls under that heading. The list is:
Gopkg.lock[[project]]indicating the pruning rules that were applied for that projectinputs-digestis completely gone, replaced by a system that directly computes whether the current lock satisfies the inputs, rather than relying on a coarse-grained, merge conflict-inducing hash digestinput-importswas added to[solve-meta]; it is a list of allimportandrequiredfrom the input projectIt's annoying that we have to have
input-imports. Without an explicit list, though, we can't determine if there are were any imports removed from the current project without having to fully explore the import graph for the whole project, in order to definitively find orphaned/unimported packages. That's costly - in particular, it would mean that in CI, we'd have to have a populated cache in order to perform basic sync checks. This is by far the better option, as it means that we can fully check sync between all three states (manifest/code, lock, vendor) without needing anything other than the states themselves. It's all self-contained.Remaining TODOs:
verify.LockSatisfaction; these will be written out as an explanation for why dep is choosing to solveenv var to allow using the old safewriter behavior, in the event of bugs(This is going to be prohibitively costly)Couple notes about followup:
dep checkin with this release, too, as it's almost trivial to implement overtop the foundation ofverify.LockSatisfaction. Separate PR, though. Will also roll dry run output into this.Gopkg.tomlto allow skipping vendor verification on a per-project basis. Follow-up PR.What should your reviewer look out for in this PR?
Couldn't point to anything in particular; this is a long time coming, and there's a lot to it. If i had time to break it down further, i would, but we'll need to let there be a bit of a bumpy ride...
Which issue(s) does this PR fix?
fixes #121
fixes #1276
fixes #1496