Respect dry run flag during dep ensure #256

carolynvs · 2017-02-21T15:09:16Z

This fixes #145.

When I first looked at this, turning on -v by default when -n is present, seemed like an easy win. However the verbose output from the gps solver wasn't really what I was looking for in a dry-run, I wanted to know what ensure would do, not what it evaluated. So I added output which clearly shows what will be written to the filesystem when the command is run without -n.

$ dep ensure -n
Would have written the following manifest.json:
{
    "dependencies": {
        "github.com/Masterminds/semver": {
            "branch": "2.x"
        },
        "github.com/Masterminds/vcs": {
            "version": "^1.8.0"
        },
        "github.com/pkg/errors": {
            "version": ">=0.8.0, <1.0.0"
        },
        "github.com/sdboyer/gps": {
            "branch": "master"
        }
    }
}

Would have written the following lock.json:
{
    "memo": "48f086eeb4e1871e10660af8a9ca7a46f72abea12df8843e62c3a1db46960d5f",
    "projects": [
        {
            "name": "github.com/Masterminds/semver",
            "branch": "2.x",
            "revision": "94ad6eaf8457cf85a68c9b53fa42e9b1b8683783",
            "packages": [
                "."
            ]
        },
        {
            "name": "github.com/Masterminds/vcs",
            "version": "v1.8.0",
            "revision": "fbe9fb6ad5b5f35b3e82a7c21123cfc526cbf895",
            "packages": [
                "."
            ]
        },
        {
            "name": "github.com/armon/go-radix",
            "branch": "master",
            "revision": "4239b77079c7b5d1243b7b4736304ce8ddb6f0f2",
            "packages": [
                "."
            ]
        },
        {
            "name": "github.com/pkg/errors",
            "version": "v0.8.0",
            "revision": "645ef00459ed84a119197bfb8d8205042c6df63d",
            "packages": [
                "."
            ]
        },
        {
            "name": "github.com/sdboyer/gps",
            "branch": "master",
            "revision": "7fb04c97d99da000c0b8f4ff153936d2354ae828",
            "packages": [
                "."
            ]
        }
    ]
}

Would have written the following projects to the vendor directory:
github.com/Masterminds/semver@94ad6eaf8457cf85a68c9b53fa42e9b1b8683783
github.com/armon/go-radix@4239b77079c7b5d1243b7b4736304ce8ddb6f0f2
github.com/Masterminds/vcs@fbe9fb6ad5b5f35b3e82a7c21123cfc526cbf895
github.com/sdboyer/gps@7fb04c97d99da000c0b8f4ff153936d2354ae828
github.com/pkg/errors@645ef00459ed84a119197bfb8d8205042c6df63d

sdboyer

When I first looked at this, turning on -v by default when -n is present, seemed like an easy win. However the verbose output from the gps solver wasn't really what I was looking for in a dry-run, I wanted to know what ensure would do, not what it evaluated.

Yeah, the trace output is a whole different beast.

So I added output which clearly shows what will be written to the filesystem when the command is run without -n.

I'm starting to think this maybe isn't the best approach. Chewing on this a bit, it's seeming to me like this PR should maybe do a few different things:

Break up SafeWriter.WriteAllSafe() into a function that takes a variable argument list similar to SafeWriter's fields right now, but instead returns some type of object where the decisions have all been made about which states are being written, and exposes that information - and THEN you call a method on that return value to do the writes. This would let you avoid having to reimplement the detection logic in the printing func.
Instead of indicating the entirety of the lock that would be written, I think it would be more valuable to simply indicate what would change. That means a fair bit more work, though - we'll need a sister function to locksAreEquivalent(), say diffLocks(), that shows a diff of the gps.LockedProjects in two locks. (TBH, that might even be better to do as a func in gps, as it would be entirely generic).
Being that we're moving towards not programatically writing to the manifest anymore, I don't think there's any need to worry about a diff output for that - we can keep with the straight dump, and then it'll just fall into disuse.

sdboyer · 2017-02-22T02:08:28Z

cmd/dep/ensure.go

 	return errors.Wrap(sw.WriteAllSafe(writeV), "grouped write of manifest, lock and vendor")
 }

+func printDryRunActions(sw dep.SafeWriter, writeV bool) error {
+	if sw.Manifest != nil {


hmm...this check isn't great, because it's now effectively duplicating write-detection logic that exists in the SafeWriter. They'd now need to be updated in both places at the same time, or the logic drifts out of whack.

I'm also not sure that the subsequent checks are even quite the same - e.g., we don't write the Lock even if it's passed if it's equal to an existing lock.

sdboyer · 2017-02-22T02:09:09Z

cmd/dep/ensure.go

+
+	if sw.Lock != nil {
+		fmt.Println("Would have written the following lock.json:")
+		m, err := sw.Lock.MarshalJSON()


Yeah, like here, the lock we actually write out depends on comparison of the old and existing lock.

sdboyer · 2017-02-22T03:16:27Z

I realize that's a bunch of things - please feel free to ping me in slack to plan a bit if you think it'd be easier to make a plan sync rather than async.

carolynvs · 2017-02-22T23:43:07Z

Just pushed up my WIP. I have what was in the initial PR refactored as we discussed. Next step I'll work on diffing the locks.

carolynvs · 2017-02-27T16:22:11Z

I lied, worked on improving the tests first. 😄 Today I'll work on diffing the locks.

carolynvs · 2017-02-28T21:00:19Z

I have a diff but haven't tackled what the console output should look like yet. I was thinking something like this?

When writing the manifest, dump the entire contents
For projects added to the lock, show the entire project entry
For projects removed from the lock, just show the project root, e.g. github.com/foo/bar
For modified projects, show the name plus
- Show the version with oldVersion -> newVerson, if the version changed
- Show the revision with oldRev -> newRev, if the revision changed
- If any packages changed, show a list of the added/removed packages with a + or - to indicate if it has been added or removed. Do not show a package if it wasn't added/removed.

dep ensure -update -n

  Would have written the following manifest.json:
  {
      "dependencies": {
          "github.com/Masterminds/semver": {
              "branch": "2.x"
          },
          "github.com/Masterminds/vcs": {
              "version": "^1.8.0"
          },
          "github.com/pkg/errors": {
              "version": ">=0.8.0, <1.0.0"
          },
          "github.com/sdboyer/gps": {
              "branch": "master"
          }
      }
  }

  Would have written the following changes to lock.json:
  Add: [
      {
          "name": "github.com/Masterminds/semver",
          "branch": "2.x",
          "revision": "94ad6eaf8457cf85a68c9b53fa42e9b1b8683783",
          "packages": [
              "."
          ]
      }
  ]
  Remove: [
      "github.com/Masterminds/vcs"
  ]
  Modify: [
    {
        "name": "github.com/pkg/errors",
        "version": "v0.8.0 -> v0.9.0",
        "revision": "645ef00459ed84a119197bfb8d8205042c6df63d -> abc123",
        "packages": [
            "+foo",
            "-bar",
        ]
    }
  ]

carolynvs · 2017-03-02T22:20:57Z

@sdboyer Alrighty, this is ready for review again.

I realize that this got bigger than we both originally thought. If you prefer that I tidy up the commits so that this can be split up into smaller PRs, just let me know.

ensure -n is implemented and prints stuff!
When printing what the command would have done, it diffs the lock file to print out only the changes. Manifest is dumped in its entirety. When this PR is merged, and you are okay with the diff impl, I can submit a PR to move this into gps.
Improved test failure reporting in the test helper.
Added a test project helper which made it easier to split up mega tests that were testing multiple scenarios in the same test. This test helper could be used in the other command tests, like ensure but to keep this PR from even more creep (or in case you hate the helper), I didn't touch any tests outside of the SafeWriter tests.

sdboyer

OK, I've done a first pass review on this. I didn't dig TOO deep into the revised mechanics of SafeWriter (i'll try to do so over the weekend at some point), but it seems like we're on a good path. Thus far, I've mostly got small nits.

The test harness improvements look generally good for this purpose, but let's hold off on using the improved harness more widely. There's a bunch of testing structure changes pending in #265, and I'd like to minimize conflicts. (Once we get both of these merged, I think we can step back a bit and figure out the next step of really making this testing system work well)

sdboyer · 2017-03-04T01:27:32Z

txn_writer.go

+// TODO(carolynvs) this should be moved to gps
+type LockedProjectDiff struct {
+	Name       gps.ProjectRoot `json:"name"`
+	Repository *StringDiff     `json:"repo,omitempty"`


Let's stick with calling this Source, as it is elsewhere. "repo" busts through some abstraction levels that, while we don't need them now, I suspect we'll want them later :)

@sdboyer Do you want the json name changed as well? Right now it's repo which matches what is output in the lock file.

sdboyer · 2017-03-04T01:28:17Z

txn_writer.go

+	Repository *StringDiff     `json:"repo,omitempty"`
+	Version    *StringDiff     `json:"version,omitempty"`
+	Branch     *StringDiff     `json:"branch,omitempty"`
+	Revision   *StringDiff     `json:"revision,omitempty"`


curious why these are all pointers

For the diff, I did not want to print the field if empty. For example, if only the revision is different, then that is the only field displayed in the diff. This was the only way I could figure out to get omitempty to work. When I used a struct, instead of a pointer, the other fields were output with an empty string value.

sdboyer · 2017-03-04T01:31:00Z

txn_writer.go

+	return nil
+}
+
+// diffLocks compares two locks and idenitfies the differences between them.


nit: s/idenitfies/identifies/

sdboyer · 2017-03-04T01:32:39Z

txn_writer.go

+
+// diffProjects compares two projects and identifies the differences between them.
+// Returns nil if there are no differences
+// TODO(carolynvs) this should be moved to gps and updated once the gps unexported fields are available to use.


I've actually been meaning to change gps to export those fields. The thing I was trying to do (make it logically impossible for LockedProjects to have nil Version data) by not exporting them in the first place was dumb, awkward, and didn't even accomplish the goal.

Doesn't matter too much, though - b/c yes, we'll be promoting this up to gps, anyway. 😄

sdboyer · 2017-03-04T01:36:36Z

txn_writer.go

+type LockDiff struct {
+	HashDiff *StringDiff
+	Add      []LockedProjectDiff
+	Remove   []gps.ProjectRoot


At this level, I think it might make more sense to just use []LockedProjectDiff for Remove, as well. The implementation here only prints the ProjectRoot, but if we're thinking generically, it seems worth it to me that we preserve both sides of the information. That way, we can experiment with the output later without having to tinker with the underlying implementation.

sdboyer · 2017-03-04T01:55:52Z

Oh and also 🎊 🎊 🎉 🎉 😄 🎊 holy crap what a lot of work here - thank you!

carolynvs · 2017-03-05T23:53:07Z

Glad to know I didn't give you a 💩 to review over the weekend. I know it's hard to tell someone that their "mega PR" needs more work. I'll get this updated tomorrow. Thanks for the review!

carolynvs · 2017-03-07T14:32:21Z

I had to rebase due to conflicting changes in the testdata files and while I was at it squashed my WIP commits.

@sdboyer This is ready for review again. I've made the following changes:

Fixed spelling
Renamed LockedProjectDiff.Repository to Source. It still serializes to json as repo so that it reflects the actual diff of the lock file, which uses repo.
Display all fields for a removed project in the lock diff

sdboyer · 2017-03-07T18:21:28Z

Renamed LockedProjectDiff.Repository to Source. It still serializes to json as repo so that it reflects the actual diff of the lock file, which uses repo.

This is totally a dumb oversight on my part. It needs to be renamed in the lock, and the struct.

I'm going to try to do another review tonight. Thanks for keeping with it!

carolynvs · 2017-03-07T20:02:43Z

@sdboyer I've renamed repo to source in the lock file (and the diff).

sdboyer · 2017-03-09T01:44:19Z

@carolynvs ahh, sorry, i should've been clear - while that renaming does need to happen, let's do it in a separate PR. it's going to potentially break some peoples' lock.json, and even though we shouldn't have to care about that, it'll be better to be able to point to a more atomic single issue.

So, just pull out that last commit, and we're ready to merge. woohoo!

carolynvs · 2017-03-09T15:05:33Z

@sdboyer Haha, I thought that seemed a bit aggressive. 😉 All fixed! After this is merged, I'll submit another PR for the rename.

carolynvs · 2017-03-09T15:07:11Z

@sdboyer Since it sounds like gps is going to move soon into the dep repo, when would you like a PR moving the bits flagged in this PR to move into the gps package?

sdboyer · 2017-03-09T17:53:57Z

@carolynvs i think it'd be fine to do that in gps now. i won't get around to moving gps until next week, i think, so we might as well use the time window we have :)

small conflict to resolve now that we've merged #287 - take care of that, and we're good to go!

(also, do you mind opening a follow-up issue for the s/repo/source/ change?)

Allows tests to easily read lock/manifest files from any directory, not just the testdata directory

carolynvs · 2017-03-09T19:35:15Z

@sdboyer I've rebased and updated the ensure test that I added to use the new test harness.

Once this is in I'll submit the rename PR and another to gps.

tro3 · 2017-03-09T19:39:09Z

@carolynvs, how intuitive or painful was the new test harness? Always looking for feedback

carolynvs · 2017-03-09T19:40:15Z

@tro3 I figured it out in about 1 minute. 🍰 💖

One thing I wondered about was if there was a place to put a "this is what this test case is about" type comment? e.g. in the testcase.json I wanted to put in the equivalent of "Make sure the -n (dry-run) flag doesn't actually do stuff OR things".

carolynvs · 2017-03-09T19:48:12Z

@tro3 I'm a goof. What I meant to say is "THANK YOU for making the tests more straightforward. It was intuitive to switch over and made the tests easier to grok."

tro3 · 2017-03-09T20:05:01Z

On the testcase doc, a README.md next to testcase.json would work fine. (That's what I was planning on doing anyway.) Extra files should not affect the tests in any way.

sdboyer

Mostly small things again, but we're making progress. (Cost of a big PR 😞 )

sdboyer · 2017-03-10T03:09:57Z

fs.go

@@ -25,7 +26,7 @@ func IsRegular(name string) (bool, error) {
 		return false, err
 	}
 	if fi.IsDir() {
-		return false, fmt.Errorf("%q is a directory, should be a file", name)
+		return false, errors.Errorf("%q is a directory, should be a file", name)


These errs changes aren't really related - next time, let's separate unrelated things into their own PR.

Sorry about that! I'll make sure to split these up better next time. Thank you for your patience.

sdboyer · 2017-03-10T03:12:02Z

lock.go

@@ -134,6 +118,29 @@ func (l *Lock) MarshalJSON() ([]byte, error) {
 	return buf.Bytes(), err
 }

+// TODO(carolynvs) this should be moved to gps


Let's make an issue for this. (I'm not quite fully convinced, but still, an issue is the thing to do.)

It would only need to move to gps if the lock diffing implementation was moved to gps as it's used when doing the diff. Here's an issue to discuss if we really want to move it and how. #312

sdboyer · 2017-03-10T03:20:57Z

test/test.go

+}
+
+// ShouldNotExist returns an error if path exists.
+func (h *Helper) ShouldNotExist(path string) error {


I'm not sure about this pattern - Should{Not}Exist() vs. Must{Not}Exist(). It seems to mirror the distinction between testing.T.Error() and testing.T.Fatal(), but it actually doesn't, because here it's left up to the caller to actually set an error. I think that might be misleading.

I can see why you're doing it this way - you want to be able to compose it with higher-order calls. But let's compose these differently - have the function that returns the error be a non-exported one, called by all of these helper methods.

This came from the discussion in #vendor on how to improve the tests and their output when a test fails. The suggestions which resonated with me was to not fail a test from a helper when it is a condition that the test was directing verifying, and to return an error instead. On the flip side, if the error is not something that a test is explicitly testing (e.g. some unrelated problem deep in the test setup), then failing with a stack trace is fine.

For example, consider a test that is asserting that the lock file doesn't exist:

When the helper fails the test, a more generic error message is output along with a stack trace. Then you must walk back through the stack trace to figure out which line in the test failed.

$ go test -run TestStuff --- FAIL: TestStuff (0.00s) test.go:64: lock.json should not exist github.com/golang/dep.TestStuff /Users/caro8994/src/github.com/golang/dep/txn_writer_test.go:21 testing.tRunner /usr/local/Cellar/go/1.8/libexec/src/testing/testing.go:657 runtime.goexit /usr/local/Cellar/go/1.8/libexec/src/runtime/asm_amd64.s:2197

When the helper returns an error that is checked by the test, the error message and offending line in the test is more clear.

--- FAIL: TestStuff (0.00s) txn_writer_test.go:441: The lock file should not be written during a dry-run

My preference is to export and use the functions that return an error (what I was naming Should*) and limit the use of the original Must* functions which fail the test from the helper.

Let me know if the problem is ambiguous name of the function, or if it is the testing pattern (returning errors) and I will update the PR accordingly.

Again, my apologies for putting this test refactoring in the PR. At the time I asked you about whether or not it should be split out, I think we both underestimated how much of a change it would be. 😃

Hah, yes, totally. I'd forgotten about both that conversation, and consideration, despite being there for it 😄 Sorry, yes, let's carry on as-is.

sdboyer · 2017-03-10T03:22:06Z

test_project_context_test.go

+	h.Cd(pc.Project.AbsRoot)
+	h.Setenv("GOPATH", pc.tempDir)
+
+	// Setup a Source Manager


nit: "setup" is a noun, "set up" is the verb phrase :)

sdboyer · 2017-03-10T03:29:04Z

test_project_context_test.go

+)
+
+// TestProjectContext groups together test project files and helps test them
+type TestProjectContext struct {


Seems like we have some overlap here with the new harness from #287. Could be that this is actually completely complementary to what's there, though. Could you provide some quick context?

sdboyer · 2017-03-12T03:23:38Z

OK, I think we're ready to go ahead with this. Everything else I had was a nit or a question...so, onward!

carolynvs · 2017-03-12T03:25:01Z

Wooo! 😄 Thank you for your patience and help getting this figured out!

Respect dry run flag during dep ensure

googlebot added the cla: yes label Feb 21, 2017

carolynvs force-pushed the dry-run branch 3 times, most recently from 1805a28 to 126753d Compare February 21, 2017 15:40

carolynvs changed the title ~~[WIP] Respect dry run flag during dep ensure~~ Respect dry run flag during dep ensure Feb 21, 2017

carolynvs force-pushed the dry-run branch from 126753d to 25ff41c Compare February 21, 2017 15:44

sdboyer suggested changes Feb 22, 2017

View reviewed changes

carolynvs changed the title ~~Respect dry run flag during dep ensure~~ [WIP]Respect dry run flag during dep ensure Feb 22, 2017

carolynvs force-pushed the dry-run branch from 25ff41c to 1b2e978 Compare February 22, 2017 23:42

carolynvs force-pushed the dry-run branch from 1b2e978 to 053e9c2 Compare February 27, 2017 16:19

carolynvs changed the title ~~[WIP]Respect dry run flag during dep ensure~~ Respect dry run flag during dep ensure Mar 3, 2017

sdboyer suggested changes Mar 4, 2017

View reviewed changes

carolynvs force-pushed the dry-run branch 2 times, most recently from 756aaf8 to 0d77dff Compare March 7, 2017 14:26

carolynvs mentioned this pull request Mar 7, 2017

project structure: move gps into a dep internal directory #300

Closed

carolynvs force-pushed the dry-run branch from 92a9233 to 0d77dff Compare March 9, 2017 15:04

sdboyer approved these changes Mar 9, 2017

View reviewed changes

carolynvs added 11 commits March 9, 2017 13:05

Wrap errors in the test helper that were missed the first time through

ea0a384

Add test.Helper.GetTestFile

578974f

Allows tests to easily read lock/manifest files from any directory, not just the testdata directory

Wrap fs errors to include a better message and callstack

891494b

Add TestProjectContext to manage common testing tasks

1c50c3a

Respect dry-run flag during dep ensure

e43a3b8

Split out SafeWriter bad input tests into 1 per scenario

6ac8a77

Don't leave open test file handles open

6f72258

Extract version type matching into a single function

d02eb69

Display diff of lock file during dry-run

02635cc

Rename LockedProjectDiff Repository -> Source

b237632

Display all fields for a removed project in the lock diff

7e27e80

carolynvs force-pushed the dry-run branch from 0d77dff to 7e27e80 Compare March 9, 2017 19:30

carolynvs mentioned this pull request Mar 9, 2017

Rename the "repo" field in the lock to "source" #308

Closed

sdboyer suggested changes Mar 10, 2017

View reviewed changes

carolynvs mentioned this pull request Mar 10, 2017

Use upstream gps lock diff implementation #312

Closed

sdboyer approved these changes Mar 12, 2017

View reviewed changes

sdboyer merged commit 55422bc into golang:master Mar 12, 2017

carolynvs deleted the dry-run branch March 12, 2017 03:25

sdboyer mentioned this pull request Mar 13, 2017

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

Closed

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

Merge pull request golang#256 from carolynvs/dry-run

87a65ec

Respect dry run flag during dep ensure

Respect dry run flag during dep ensure #256

Respect dry run flag during dep ensure #256

Conversation

carolynvs commented Feb 21, 2017 • edited Loading

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

sdboyer commented Feb 22, 2017

carolynvs commented Feb 22, 2017

carolynvs commented Feb 27, 2017

carolynvs commented Feb 28, 2017

carolynvs commented Mar 2, 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

Choose a reason for hiding this comment

sdboyer commented Mar 4, 2017

carolynvs commented Mar 5, 2017

carolynvs commented Mar 7, 2017

sdboyer commented Mar 7, 2017

carolynvs commented Mar 7, 2017

sdboyer commented Mar 9, 2017

carolynvs commented Mar 9, 2017

carolynvs commented Mar 9, 2017

sdboyer commented Mar 9, 2017

carolynvs commented Mar 9, 2017 • edited Loading

tro3 commented Mar 9, 2017

carolynvs commented Mar 9, 2017 • edited Loading

carolynvs commented Mar 9, 2017

tro3 commented Mar 9, 2017 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdboyer commented Mar 12, 2017

carolynvs commented Mar 12, 2017

carolynvs commented Feb 21, 2017 •

edited

Loading

carolynvs commented Mar 9, 2017 •

edited

Loading

carolynvs commented Mar 9, 2017 •

edited

Loading

tro3 commented Mar 9, 2017 •

edited

Loading