Write vendor, regardless of lock, if force is true #325

carolynvs · 2017-03-13T14:53:28Z

I wasn't taking forcevendor into account properly when the diffs were the same (which is exactly what that flag was for...) 😞

This fixes my misunderstanding about when lock and newlock should be set in the SafeWriter tests. Previously, I thought I should only pass in newlock when the lock had changed, or was brand new, when actually it should be passed anytime there was an existing lock file. Due to the bad test, it was missing that a bare ensure with an existing lock file (and no vendor) should result in vendor being written.

This fixes #323.

Fix misunderstanding about when lock and newlock should be set in the SafeWriter tests.

jstemmer · 2017-03-13T19:26:05Z

txn_writer.go

 	}

 	if newLock != nil {
 		if lock == nil {
 			sw.Payload.Lock = newLock
-			sw.Payload.ForceWriteVendor = true
+			sw.Payload.WriteVendor = true
 		} else {
 			diff := diffLocks(lock, newLock)


Running dep ensure -n without a lock.json file does not print a diff even though dep ensure creates a new lock file. I don't think that's WAI?

With the other changes you made here this entire block in newLock != nil could then be simplified to:

if newLock != nil { sw.Payload.Lock = newLock sw.Payload.LockDiff = diffLocks(lock, newLock) sw.Payload.WriteVendor = true } ...

which solves the problem. However, you'd probably have to fix the nil check in the diffLocks function for this to work when lock is nil.

Thanks! I'll play with that simplification and add a test for a bare ensure without an existing lock file.

carolynvs · 2017-03-15T16:22:40Z

I've fixed ensure dry-run to print the entire lock file when it is brand new, only showing the diff when there was an existing lock.
I've simplified SafeWriter.Prepare.

* Always write the manifest when provided. * Write the lock only when modified or when writing vendor/. * Clarify what should be passed for the two locks args: old lock and new lock. Updated `dep init` to match this expectation. * Add test case for writing a new lock

carolynvs · 2017-03-15T16:27:45Z

This is ready for review

jstemmer

One thing I noticed while testing this was that the lock diff is not printed if LockDiff only contains a HashDiff. Probably not worth delaying this PR for since it fixes #323, but something to keep in mind.

Other than that LGTM.

jstemmer · 2017-03-17T18:13:54Z

txn_writer.go

@@ -160,26 +157,23 @@ func (diff StringDiff) MarshalJSON() ([]byte, error) {
 //   and the vendor directory in the same way
 // - If the forceVendor param is true, then vendor/ will be unconditionally
 //   written out based on newLock if present, else lock, else error.
-func (sw *SafeWriter) Prepare(manifest *Manifest, lock *Lock, newLock *Lock, forceVendor bool) {
+func (sw *SafeWriter) Prepare(manifest *Manifest, oldLock *Lock, newLock *Lock, forceVendor bool) {


nit: The function comments still refer to lock

Also, s/oldLock *Lock, newLock *Lock/oldLock, newLock *Lock)

Doh! 😊 Fixing...

sdboyer · 2017-03-17T19:27:37Z

This is looking pretty good! I want to do a more thorough pass a little later, but aside from that nit, it seems like this should be able to go in presently. Thanks!

carolynvs · 2017-03-17T21:17:36Z

I'm reviewing the original doc on that function (from before I ever touched it) and am a bit unsure about this scenario:

https://github.com/golang/dep/blob/0acfd38/txn_writer.go#L40
If sw.Lock is provided without an sw.NewLock, it will be written to the standard lock file name in the root dir, but vendor will NOT be written

I believe the only usage of this function where the original lock is nil and the new lock is specified is during dep init but that is always passing forceVendor=true. Is this still a valid scenario (no existing lock, brand new lock, don't write to vendor)? If so, then I need to add a test for that and fix my implementation.

Based on my understanding, this is what I think the implementation (and doc) should be for SafeWriter.Prepare

// Prepare to write a set of config yaml, lock and vendor tree.
//
// - If manifest is provided, it will be written to the standard manifest file
//   name beneath root.
// - If only a newLock is provided, or the locks are not equivalent,
//   the newLock will be written to the standard lock file name beneath root,
//   and a vendor tree will be written to the vendor directory.
// - If the forceVendor param is true, then vendor/ will be unconditionally
//   written out based on newLock.

carolynvs · 2017-03-17T21:55:18Z

I've updated the lock diff to include the memo if it has changed, e.g. Memo: abc123 -> def546.

sdboyer · 2017-03-17T22:53:26Z

I think the case I was aiming for with that original doc was a bit of forward-thinking - that we'd want to dep ensure -no-vendor, where we ensure the correct lock is written, but skip writing out vendor.

We're not quite ready for that now, I think, but I'd like to see the safe writer preserve support for such a case when we get to it.

carolynvs · 2017-03-17T23:00:08Z

Gotcha! I'll get that fixed tonight.

* If manifest is provided, write it. * If newLock is provided, write it. * Explicit vendor behavior: VendorOnChanged, VendorAlways, VendorNever. * Write vendor/ using newLock.

carolynvs · 2017-03-19T16:56:00Z

Apologies for the delay. It was really bothering me how much logic was being "encoded" in the combination of function arguments and I wanted to make this more explicit/maintainable for the next person.

If manifest is provided, write it.
If newLock is provided, write it.
Control whether vendor/ is written using: VendorOnChanged, VendorAlways, VendorNever.
Write vendor/ using newLock.

So for the future scenario of dep ensure --no-vendor, the caller passes VendorNever instead of varying whether oldLock or newLock is set.

sdboyer · 2017-03-20T16:12:55Z

This looks to be in pretty good shape, so I'm gonna merge - thanks!

Write vendor, regardless of lock, if force is true

Write vendor, regardless of lock, if force is true

2287888

Fix misunderstanding about when lock and newlock should be set in the SafeWriter tests.

googlebot added the cla: yes label Mar 13, 2017

jstemmer reviewed Mar 13, 2017

View reviewed changes

sdboyer mentioned this pull request Mar 14, 2017

Ensure lock issue on docker #332

Closed

carolynvs mentioned this pull request Mar 15, 2017

dep ensure: must provide a lock in order to write out vendor #323

Closed

Print new lock file contents during dry-run

f5fbfe4

carolynvs force-pushed the fix-ensure-unmodified-lock branch from 42f2cb1 to e565a9f Compare March 15, 2017 16:24

jstemmer reviewed Mar 17, 2017

View reviewed changes

Include memo in lock diff

008ff04

Further simplify ensure payload logic

a5ab339

* If manifest is provided, write it. * If newLock is provided, write it. * Explicit vendor behavior: VendorOnChanged, VendorAlways, VendorNever. * Write vendor/ using newLock.

sdboyer merged commit aaf8a8b into golang:master Mar 20, 2017

carolynvs deleted the fix-ensure-unmodified-lock branch March 20, 2017 16:42

ibrasho pushed a commit to ibrasho-forks/dep that referenced this pull request May 10, 2017

Merge pull request golang#325 from carolynvs/fix-ensure-unmodified-lock

0aa070a

Write vendor, regardless of lock, if force is true

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Write vendor, regardless of lock, if force is true #325

Write vendor, regardless of lock, if force is true #325

Uh oh!

carolynvs commented Mar 13, 2017 •

edited

Loading

Uh oh!

jstemmer Mar 13, 2017

Uh oh!

carolynvs Mar 13, 2017

Uh oh!

carolynvs commented Mar 15, 2017

Uh oh!

carolynvs commented Mar 15, 2017

Uh oh!

jstemmer left a comment

Uh oh!

jstemmer Mar 17, 2017

Uh oh!

sdboyer Mar 17, 2017

Uh oh!

carolynvs Mar 17, 2017

Uh oh!

sdboyer commented Mar 17, 2017

Uh oh!

carolynvs commented Mar 17, 2017 •

edited

Loading

Uh oh!

carolynvs commented Mar 17, 2017 •

edited

Loading

Uh oh!

sdboyer commented Mar 17, 2017

Uh oh!

carolynvs commented Mar 17, 2017

Uh oh!

carolynvs commented Mar 19, 2017

Uh oh!

sdboyer commented Mar 20, 2017

Uh oh!

Uh oh!

Write vendor, regardless of lock, if force is true #325

Write vendor, regardless of lock, if force is true #325

Uh oh!

Conversation

carolynvs commented Mar 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jstemmer Mar 13, 2017

Choose a reason for hiding this comment

Uh oh!

carolynvs Mar 13, 2017

Choose a reason for hiding this comment

Uh oh!

carolynvs commented Mar 15, 2017

Uh oh!

carolynvs commented Mar 15, 2017

Uh oh!

jstemmer left a comment

Choose a reason for hiding this comment

Uh oh!

jstemmer Mar 17, 2017

Choose a reason for hiding this comment

Uh oh!

sdboyer Mar 17, 2017

Choose a reason for hiding this comment

Uh oh!

carolynvs Mar 17, 2017

Choose a reason for hiding this comment

Uh oh!

sdboyer commented Mar 17, 2017

Uh oh!

carolynvs commented Mar 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

carolynvs commented Mar 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sdboyer commented Mar 17, 2017

Uh oh!

carolynvs commented Mar 17, 2017

Uh oh!

carolynvs commented Mar 19, 2017

Uh oh!

sdboyer commented Mar 20, 2017

Uh oh!

Uh oh!

carolynvs commented Mar 13, 2017 •

edited

Loading

carolynvs commented Mar 17, 2017 •

edited

Loading

carolynvs commented Mar 17, 2017 •

edited

Loading