Support updating a list of dependencies #235

carolynvs · 2017-02-10T18:34:43Z

Fixes #202.

Update a list of dependencies with dep ensure -update github.com/pkg/foo github.com/pkg/bar.
This command updates the listed package(s) to the latest version that meets the constraints in the manifest, and writes any changes to the lock file.

⚠️ The last commit changes the behavior of dep ensure -update to not modify the manifest. This seems like the direction things are headed based on #213 and #233. Honestly I was surprised that an update would have modified it to begin with, as it rewrote a constraint to mean something different, changing it from ~0.1.0 to ^0.1.0. Let me know if this is premature, or should be moved off into another issue.

Notes:

A space separated list of packages may be passed to dep ensure -update. The flag is not repeatable like the -override flag.
This command is similar to dep ensure github.com/pkg/foo@^0.8.0 but addresses a different use case. This command is intended to update a dependency to the latest version, while that other command is intended to assist with modifying the manifest, and then runs dep ensure.

Specifically, this command updates the local copy of a dependency to the latest version, and only modifies the lock file. While that other command modifies manifest, verifies the local copy of the dependency, and only replaces the local copy of the dependency when it no longer meets the constraints defined in the manifest.

googlebot · 2017-02-10T18:34:46Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please let us know the company's name.

carolynvs · 2017-02-10T18:40:34Z

I signed it!

googlebot · 2017-02-10T18:40:35Z

CLAs look good, thanks!

carolynvs · 2017-02-11T14:12:43Z

@sdboyer This is ready for some 👀 . The AppVeyor build is failing due what I think is test cleanup flakiness. If you think that it's failing from my PR, I'll dig in further on a Windows env.

tro3 · 2017-02-11T17:04:09Z

I think you're seeing #214.

sdboyer

In general, I think this is looking pretty good! Just a couple notes. And:

The last commit changes the behavior of dep ensure -update to not modify the manifest.

This is definitely right 👍👍👍. Old behavior was an oversight/bug.

sdboyer · 2017-02-13T05:08:07Z

cmd/dep/ensure.go

+		return
+	}
+
+	// Allow any of specified project versions to change, regardless of the lock file


I think we discussed checking for the presence of the named project in the lock file, and erroring out if it wasn't present. At least, that seems to make the most sense to me now, when looking at an implementation.

Turns out gps is checking and returning an error in that scenario already. Although I can handle the error and wrap with a different message perhaps? Here's what it looks like now when updating something that isn't already in the lock:

$ dep ensure -update github.com/sillyperson/fakepackage ensure Prepare: cannot update github.com/sillyperson/fakepackage as it is not in the lock

Yeah, I was pretty sure I'd guarded against that case :)

If you have an immediate improvement for the text in mind, OK. If not, though, for now I'd say just leave the error as-is. Tackling failures that come back from gps is a major task in itself, and we'll probably want a bit of a framework for it. I'd rather we make a concerted effort around that than have it organically grow through one-offs.

I spent a minute thinking about what would be a good error message and came back with "Exactly what gps is already returning, along with a recommended command to run to get back on track". I'd prefer to tackle stuff like this (presenting a user-friendly version of the gps error message) across the board.

Yeah, that sounds spot-on to me. And it reflects the division of responsibility between gps and dep well - gps says what's wrong with the abstract inputs, and dep knows about what the user input looked like that generated it (and how it might be modified).

Given the change of direction wrt CLI UI described in #213, these possibilities are likely to change and narrow anyway.

sdboyer · 2017-02-13T05:08:58Z

cmd/dep/ensure.go

-			}
-		}
+		// Ignore the lockfile for this dependency and allow its version to change
+		params.ToChange = append(params.ToChange, pc.Ident.ProjectRoot)


I think it's probably better not to tip the flag in this case. If the constraint has changed in a way that prohibits the version in the lock, it'll work itself out, anyway. Otherwise, the user didn't say -update, so...

Sure thing, I'll go back to what it was doing before.

Would you help me understand what the original code was doing? I had thought that it was tweaking the in-memory representation of the lock to allow the package version to change, and that switching to ToChange was equivalent. Sounds like I am misunderstanding that however. 😊

I checked back through blame, and it looks like the original intent was indeed doing what you're doing here. It was work done back in early Dec, and we were flying pretty seat-of-pants at that point; I just hadn't explained the nuance of using params.ToChange instead of just pulling it out of the lock.

sdboyer · 2017-02-13T05:10:51Z

cmd/dep/ensure.go

-			}
-		}
+		// Ignore the lockfile for this dependency and allow its version to change
+		params.ToChange = append(params.ToChange, pc.Ident.ProjectRoot)


Same deal as above, I don't think we want to mark for upgrade in this case.

sdboyer · 2017-02-13T05:12:32Z

cmd/dep/ensure_test.go

+
+	m := `package main
+
+import (


I just merged #215 (sorry), so please rebase this branch and switch these fixtures to using the golden file pattern.

Note that only the unit tests have the golden-file pattern right now. Integration test conversions are waiting on the CLI checks to pass dep remove.

@tro3 So far I have the update test (not the others in ensure_test.go) working with golden files. Do you want me to hold off on that or finish up the rest of the file?

However you like. I have my intial conversions parked on a local branch, and I haven't gotten to ensure_test.go yet. You can finish up the file, or leave it to me - I'm good either way.

@tro3 I'll give it a go!

* A list of packages may be passed to -update. The flag is not repeatable like the -override flag. * Remove restriction on passing arguments to -update.

Instead of munging the lock to allow some projects to be updated, make use of the ToChange parameter.

Replace use of logrus with a dependency that doesn't have transient dependencies so that we don't need to lookup the latest commit, and instead can hard-code the hash in the golden file.

carolynvs · 2017-02-13T22:03:35Z

@sdboyer @tro3 I've updated the PR with the requested changes:

Remove ToChange or lock manipulation for -override and bare ensure.
Update all ensure tests to use golden files. In tests which didn't have an existing lock file (e.g. dep init && dep ensure), I swapped the Logrus dependency for a test project, so that we could hard-code a hash in the golden lock files.

tro3 · 2017-02-14T01:36:04Z

Nice. @sdboyer is that change from logrus to go-dep-test something you want me to duplicate in the other tests? Would it make sense to set up such a test project in the golang org, to contain dependencies?

carolynvs · 2017-02-14T02:05:33Z

My vote is for go-dep-test and the other test repos to live under the golang org.

…

On Feb 13, 2017, at 7:36 PM, tro3 ***@***.***> wrote: Nice. @sdboyer is that change from logrus to go-dep-test something you want me to duplicate in the other tests? Would it make sense to set up such a test project in the golang org, to contain dependencies? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

sdboyer · 2017-02-14T03:42:05Z

I have some organizational nits, but I'm going to put them in a separate issue, because I want to treat them a bit more holistically. Apart from those, I think this put us in pretty good shape, so I'm going to merge 🎊 🎊

Nice. @sdboyer is that change from logrus to go-dep-test something you want me to duplicate in the other tests?

Yes, for most tests, I'd strongly prefer that we avoid relying on real code that can change, and instead use fixture repositories. My strong preference is that we consolidate down to only using a few of these repos, just to minimize our exposure.

My REAL goal is that we don't go over the net at all for these fixture repos. But that's another issue :)

My vote is for go-dep-test and the other test repos to live under the golang org.

dep itself is already in a bit of a weird spot with respect to living under github.com/golang (being that we're not really truly "official" yet). So, I'm not sure that's the best place for test repos to live.

googlebot added the cla: no label Feb 10, 2017

googlebot added cla: yes and removed cla: no labels Feb 10, 2017

carolynvs force-pushed the ensure-update-flag branch 3 times, most recently from 50f0007 to 526090e Compare February 11, 2017 14:00

carolynvs changed the title ~~[WIP] Support updating a list of dependencies~~ Support updating a list of dependencies Feb 11, 2017

sdboyer suggested changes Feb 13, 2017

View reviewed changes

carolynvs force-pushed the ensure-update-flag branch from 526090e to 03de731 Compare February 13, 2017 15:23

carolynvs added 6 commits February 13, 2017 11:29

Document passing args to dep ensure -update

a092cfb

* A list of packages may be passed to -update. The flag is not repeatable like the -override flag. * Remove restriction on passing arguments to -update.

Add failing test for dep ensure -update github.com/foo/bar

1e1a610

Populate the list of dependencies to update from the command arguments

42109a7

Instead of munging the lock to allow some projects to be updated, make use of the ToChange parameter.

Tidy up the logic for -update vs. a bare ensure

4ad24d0

Do not modify the manifest during dep ensure -update

cd7da74

Only allow a version to change when using -update

088af81

carolynvs force-pushed the ensure-update-flag branch 5 times, most recently from 38d79de to c20a1e9 Compare February 13, 2017 21:54

Use golden test files for the ensure tests

4e573de

Replace use of logrus with a dependency that doesn't have transient dependencies so that we don't need to lookup the latest commit, and instead can hard-code the hash in the golden file.

carolynvs force-pushed the ensure-update-flag branch from c20a1e9 to 4e573de Compare February 13, 2017 21:58

sdboyer approved these changes Feb 14, 2017

View reviewed changes

sdboyer merged commit 82f1514 into golang:master Feb 14, 2017

carolynvs mentioned this pull request Mar 5, 2017

Possible ensure bug #284

Closed

carolynvs mentioned this pull request Apr 20, 2017

cmd/dep: clarify ensure help text #404

Closed

carolynvs deleted the ensure-update-flag branch June 11, 2017 15:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support updating a list of dependencies #235

Support updating a list of dependencies #235

carolynvs commented Feb 10, 2017 •

edited

Loading

googlebot commented Feb 10, 2017

carolynvs commented Feb 10, 2017

googlebot commented Feb 10, 2017

carolynvs commented Feb 11, 2017

tro3 commented Feb 11, 2017

sdboyer left a comment

sdboyer Feb 13, 2017

carolynvs Feb 13, 2017

sdboyer Feb 13, 2017

carolynvs Feb 13, 2017

sdboyer Feb 13, 2017

sdboyer Feb 13, 2017

carolynvs Feb 13, 2017 •

edited

Loading

sdboyer Feb 13, 2017

sdboyer Feb 13, 2017

sdboyer Feb 13, 2017

tro3 Feb 13, 2017

carolynvs Feb 13, 2017 •

edited

Loading

tro3 Feb 13, 2017

carolynvs Feb 13, 2017

carolynvs commented Feb 13, 2017

tro3 commented Feb 14, 2017

carolynvs commented Feb 14, 2017 via email

sdboyer commented Feb 14, 2017

Support updating a list of dependencies #235

Support updating a list of dependencies #235

Conversation

carolynvs commented Feb 10, 2017 • edited Loading

googlebot commented Feb 10, 2017

carolynvs commented Feb 10, 2017

googlebot commented Feb 10, 2017

carolynvs commented Feb 11, 2017

tro3 commented Feb 11, 2017

sdboyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carolynvs Feb 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carolynvs Feb 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carolynvs commented Feb 13, 2017

tro3 commented Feb 14, 2017

carolynvs commented Feb 14, 2017 via email

sdboyer commented Feb 14, 2017

carolynvs commented Feb 10, 2017 •

edited

Loading

carolynvs Feb 13, 2017 •

edited

Loading

carolynvs Feb 13, 2017 •

edited

Loading