Note diffs b/w imported and solved lock #1475

zmackie · 2017-12-22T16:47:36Z

What does this do / why do we need it?

This is pass at #908 , which intends to inform users when changes have been made from their imported revisions.

In addition to giving the user feedback, this PR changes the implementation of:

Line 167 in a926781

    
           func (a *rootAnalyzer) FinalizeRootManifestAndLock(m *dep.Manifest, l *dep.Lock, ol dep.Lock) {

to use a map of the projects in the old lock in order to:
a. Find projects that exist in the new and old locks for diffing
b. Find projects that are only in the new lock for NewLockedProjectFeedback

What should your reviewer look out for in this PR?

Am I even on the right track? I may be misunderstanding the differences b/w locks, constraints, and manifest (the terms are a bit fuzzy as yet) or what I'm actually to compare to see a change.

Do you need help or clarification on anything?

Am I looking at the right data to check if a pertinent change has happened?
Is the Feedback pattern the right way to propagate this information? If so, does the necessitate a new kind of feedback eg NewDiffFeedback? Right now I'm just logging to the console which is, obviously, not a final method of relaying this feedback.
As a corollary, if I'm going to use a LockedProjectDiff for the feedback, I'm considering teaching that type String(). Any thoughts on that?

Which issue(s) does this PR fix?

Fixes #908

carolynvs

You are on the right track! 👍

carolynvs · 2017-12-24T03:15:59Z

cmd/dep/root_analyzer.go

+		op, inOl := olLookup[pr]
+		if inOl {
+			if diff := gps.DiffProjects(y, op); diff != nil {
+				fmt.Printf("Some changes: \nImported: %v\nRevised: %v\nDiff: %+v\n", y, op, diff)


Let's go with your suggestion and add a new method to the feedback package like NewBrokenImportFeedback which prints something like this:

Warning: Unable to preserve imported lock v1.2.0 (abc123) for github.com/example/A. Locking in v1.2.1 (def456).

When you add a new feedback function, lets make sure to also add a unit test for it. There should be other tests in feedback_test that you can use as an example.

carolynvs · 2017-12-24T03:20:17Z

cmd/dep/root_analyzer.go

+	//
+	// IDEA: Make a lookup map of ProjectRoot ->projects in the old lock
+	// use that data structure to simplify these loops
+	olLookup := make(map[gps.ProjectRoot]gps.LockedProject)


This currently doesn't identify projects which were removed from the original/imported lock. I see that you used DiffProjects from gps and that's a great idea! I hadn't thought of that but it should save us from re-implementing the diff logic.

How about we use gps.DiffLocks which will tell us what's been added, removed and modified from the entire locks. Then we don't need to do any looping over the projects, and we can use the resulting LockDiff to print feedback on what has been added, and warn about modifies/deletes.

carolynvs · 2017-12-24T03:31:07Z

As a corollary, if I'm going to use a LockedProjectDiff for the feedback, I'm considering teaching that type String(). Any thoughts on that?

If you'd like to improve the feedback functions, I suggest that you tackle that in a separate follow-on pull request.

- Creates a new feedback type to alert users of broken imports - Unit tests for above - Utillize new functionality in FinalizeRootManifestAndLock - Teach brokenImport String()

zmackie · 2017-12-28T17:50:06Z

@carolynvs Made some updates based on your feedback.

zmackie · 2017-12-28T17:51:14Z

internal/feedback/feedback.go

+// NewBrokenImportFeedback builds a feedback entry for problems with imports from a diff of the pre- and post- solved locks
+func NewBrokenImportFeedback(ld *gps.LockDiff) *BrokenImportFeedback {
+	bi := &BrokenImportFeedback{}
+	for _, lpd := range ld.Modify {


I wasn't sure if we also needed to cover Add and Remove here. As of now, this is only taking note of Modify from the LockDiff.

We also want to warn when the final lock has removed a project. Maybe something like:

Warning: Unable to preserve imported lock v1.1.4 (bc29b4f) for github.com/foo/bar. The project was removed from the lock because it is not used.

For example, I had a project in my glide.lock that was imported, but was then removed by dep. It may help me realize that I need to manually add a required entry for that project to my manifest, to force it to be included.

We could look for adds, but that is beyond the scope of broken locks, and doesn't belong in this PR. That is what the existing functionality is doing in the else block of FinalizeManifestAndLock.

zmackie · 2017-12-28T17:52:15Z

internal/feedback/feedback.go

+	projectPath string
+	version     *gps.StringDiff
+	revision    *gps.StringDiff
+}


This struct is only tracking versions and revisions for feedback, but that may not be sufficient. Not sure...

We want to warn when one of the following properties of a locked project changes:

source

branch

version

revision

zmackie · 2017-12-28T17:52:47Z

internal/feedback/feedback.go

@@ -115,3 +164,14 @@ func GetLockingFeedback(version, revision, depType, projectPath string) string {
 	}
 	return fmt.Sprintf("Locking in %s (%s) for %s %s", version, revision, depType, projectPath)
 }
+
+func trimSHA(revision string) string {


Added this convenience method, which we could leverage above at https://github.com/golang/dep/pull/1475/files#diff-ca9bf3aa3bbb62e361e500352f67474bR152

carolynvs

This is looking good! Just needs a few tweaks to include the other ways that an imported lock could be broken.

carolynvs · 2017-12-29T15:38:06Z

internal/feedback/feedback.go

+// NewBrokenImportFeedback builds a feedback entry for problems with imports from a diff of the pre- and post- solved locks
+func NewBrokenImportFeedback(ld *gps.LockDiff) *BrokenImportFeedback {
+	bi := &BrokenImportFeedback{}
+	for _, lpd := range ld.Modify {


We also want to warn when the final lock has removed a project. Maybe something like:

Warning: Unable to preserve imported lock v1.1.4 (bc29b4f) for github.com/foo/bar. The project was removed from the lock because it is not used.

For example, I had a project in my glide.lock that was imported, but was then removed by dep. It may help me realize that I need to manually add a required entry for that project to my manifest, to force it to be included.

We could look for adds, but that is beyond the scope of broken locks, and doesn't belong in this PR. That is what the existing functionality is doing in the else block of FinalizeManifestAndLock.

carolynvs · 2017-12-29T15:39:36Z

internal/feedback/feedback.go

+	projectPath string
+	version     *gps.StringDiff
+	revision    *gps.StringDiff
+}


We want to warn when one of the following properties of a locked project changes:

source

branch

version

revision

carolynvs · 2017-12-29T15:45:11Z

internal/feedback/feedback.go

@@ -115,3 +164,14 @@ func GetLockingFeedback(version, revision, depType, projectPath string) string {
 	}
 	return fmt.Sprintf("Locking in %s (%s) for %s %s", version, revision, depType, projectPath)
 }
+
+func trimSHA(revision string) string {


carolynvs · 2017-12-29T15:51:38Z

internal/feedback/feedback.go

+		cr = fmt.Sprintf("(%s)", trimSHA(bi.revision.Current))
+	}
+
+	return fmt.Sprintf("%v %s for %s. Locking in %v %s",


With the inclusion of branch and source, maybe something like this would do:

Warning: Unable to preserve imported lock VERSION/BRANCH (REV) for PROJECT. Locking in VERSION/BRANCH (REV)

Where for VERSION/BRANCH we display whichever is not an empty string.

If the source diff is not nil:

Warning: Unable to preserve imported lock VERSION/BRANCH (REV) for PROJECT(SOURCE). Locking in VERSION/BRANCH (REV) for PROJECT(SOURCE)

Basically, don't print the source unless it's changed.

…urce for projects

- Cover: - Removed project - Changed source - Changed Branch

zmackie · 2018-01-02T17:30:06Z

@carolynvs Made some updates Per your comments. Let me know what you think.

carolynvs

Yay! Thank you for adding this, it's been loooooooong overdue. 👍 💖

googlebot · 2018-01-11T16:18:15Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this State. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

carolynvs · 2018-01-11T16:46:06Z

Oops, I've angered the CLA BOT gods by "helpfully" fixing a lint problem on the PR. I gotta remember that doesn't work on this repo... 😂

zmackie · 2018-01-11T22:33:58Z

Googlebot: I consent to the lint fixes LOL

carolynvs · 2018-01-11T23:10:05Z

No worries, when our fearless leader @sdboyer is available, he will manually merge the mess I made of your PR. 😀

sdboyer

thanks for your patience on the review. i'm trying to get v0.4.0 out the door, and it's kinda all-consuming.

i haven't actually absorbed the logic here, as i got stopped by the naming of things. Maybe i'm missing some key context, but it seems like the namings here need to be adjusted for the longer term and bigger picture.

sdboyer · 2018-01-18T00:43:30Z

internal/feedback/feedback.go

+	brokenImports []brokenImport
+}
+
+// NewBrokenImportFeedback builds a feedback entry for problems with imports from a diff of the pre- and post- solved locks


nit: let's add a line break here, and a complete sentence (final period).

Also, we can only categorically say that everything we might log here is a "problem" if we're imagining the sole use case for this feedback is on init. That's the case in this PR, but as this comment notes, this is a more generic algorithm for reporting on diffs in locks. And it's definitely important that we be able to give this kind of feedback outside of dep init, where it is not necessarily true that a change in the lock is a "problem," because the goal in all cases may or may not be fidelity to the original lock.

This doesn't require changing implementation, but rewriting these docs to focus on the functional relationship being handled by the type should happen in order to minimize future confusion about this type's purpose.

note - i could be convinced that this particular feedback mechanism is specific to the dep init case - but even if it is, it doesn't seem like the underlying types that compose the slice should be so tightly coupled.

sdboyer · 2018-01-18T00:44:37Z

internal/feedback/feedback.go

+	}
+
+	// Warning: Unable to preserve imported lock VERSION/BRANCH (REV) for PROJECT(SOURCE). Locking in VERSION/BRANCH (REV) for PROJECT(SOURCE)
+	return fmt.Sprintf("%v %s for %s. The project was removed from the lock because it is not used.",


Vars all on the same line, please - unless this is a practice from stdlib/the toolchain with which i'm unfamiliar.

sdboyer · 2018-01-18T00:58:46Z

internal/feedback/feedback.go

+	)
+}
+
+// BrokenImportFeedback holds problematic lock feedback data


This needs to be a complete sentence, but also an informative one. Per the discussion in the other comment about "problematic", that term is both uninformative, and its connotation may or may not apply.

sdboyer · 2018-01-18T01:01:31Z

internal/feedback/feedback.go

+}
+
+// BrokenImportFeedback holds problematic lock feedback data
+type BrokenImportFeedback struct {


Naming again: i think that neither "broken" nor "import" are the right terms here. i believe something like ChangedLockFeedback might be more appropriate.

sdboyer · 2018-01-18T01:02:19Z

internal/feedback/feedback.go

+func NewBrokenImportFeedback(ld *gps.LockDiff) *BrokenImportFeedback {
+	bi := &BrokenImportFeedback{}
+	for _, lpd := range ld.Modify {
+		// Ignore diffs where it's just a modified package set


Why ignore this case?

The importers do not set packages, so it's guaranteed that every lock set by an importer will have a different set of packages than what was solved.

sdboyer · 2018-01-18T01:03:30Z

internal/feedback/feedback.go

@@ -82,6 +82,127 @@ func (cf ConstraintFeedback) LogFeedback(logger *log.Logger) {
 	}
 }

+type brokenImport interface {


Naming: "Import" is not the noun here. "Dependency" is slightly murky in our terminology, but is probably the best noun to use. And, prefer "changed" to "broken," per other comments.

carolynvs · 2018-01-18T19:29:51Z

@sdboyer This feature is intended for detecting when configuration imported by init was altered by the solve, breaking any imported locks. It's so that when someone runs dep init, they can trust that their glide/godep/etc config was imported properly.

My plan is to add a flag like dep init --strict which would cause the init to return a non-zero exit code and a stronger error message after the config files are written.

carolynvs · 2018-01-18T19:36:56Z

@sdboyer Would you be okay with the formatting/doc nits being fixed in this PR, and then tackle making this generic for identifying lock changes (and not specific to init) in a follow-on pull request? I'd be happy to do that and collab with you to figure out where else we'd like to hook this in to the other commands.

zmackie · 2018-01-20T21:56:19Z

I made the formatting and sentence changes to at least get that out of the way before we decide if we want to handle making this new feedback more generic in this PR or as a followup.

I definitely see your point @sdboyer regarding the broader use of this code for alerting users to changes in locks. Basically the use case seems to be: Give some feedback on how a lock changed after something was done to it (solve being the only "something" I know of at this point). AlteredLockFeedback or ChangedLockedFeedback seem like a good names in this case.

That said, I also agree with @carolynvs that this PR is solving a particular problem resulting from changes in a post-init solve, and the issue filed had to do with a dep user wanting to know that this had happened. I personally err on the side of coding to the specific problem in front of me until more general cases present themselves. But really, I'm fine either way you folks want to go.

carolynvs · 2018-02-03T22:17:24Z

@sdboyer Can you please weigh in on if this must be made more generic than the issue called for in this PR or can be addressed in a follow-up issue? I'd really like to have feature, it's incredibly useful, so if we need to do more work on this PR in order to get it merged, it would help to know.

stepping back

sdboyer · 2018-02-21T15:36:08Z

@carolynvs i'm good with kicking to a follow-up if you are. merge at your discretion :)

zmackie requested a review from carolynvs as a code owner December 22, 2017 16:47

googlebot added the cla: yes label Dec 22, 2017

carolynvs suggested changes Dec 24, 2017

View reviewed changes

zmackie force-pushed the imported-lock-revs-not-changed branch from c7ae7f2 to 7b2fcae Compare December 28, 2017 16:01

Create BrokenImportFeedback and use in FinalizeRootManifest

c92b728

- Creates a new feedback type to alert users of broken imports - Unit tests for above - Utillize new functionality in FinalizeRootManifestAndLock - Teach brokenImport String()

zmackie force-pushed the imported-lock-revs-not-changed branch from 2dc2a1c to c92b728 Compare December 28, 2017 17:29

Add comments for new methods

6d1ebdc

zmackie commented Dec 28, 2017

View reviewed changes

carolynvs suggested changes Dec 29, 2017

View reviewed changes

zmackie added 4 commits December 29, 2017 11:50

Use trimSHA in GetUsingFeedback

14f1ae8

BrokenImportFeedback track modifications to locks and branches and so…

fd6ef50

…urce for projects

Update BrokenImportFeedback tests for easier parameterizing

c2c59f1

Add test cases for BrokenImportFeedback

ce07d2e

- Cover: - Removed project - Changed source - Changed Branch

zmackie changed the title ~~[WIP] Note diffs b/w imported and solved lock~~ Note diffs b/w imported and solved lock Jan 2, 2018

carolynvs approved these changes Jan 11, 2018

View reviewed changes

Make lint happy

502b4a3

googlebot added cla: no and removed cla: yes labels Jan 11, 2018

Don't warn about import diffs when it's only packages

16c38f4

sdboyer suggested changes Jan 18, 2018

View reviewed changes

zmackie added 2 commits January 20, 2018 16:42

Sprintf variables on same line

9075212

BrokenImportFeedback docs complete sentences

e910026

carolynvs mentioned this pull request Jan 22, 2018

Large Number of Diffs After init-ing from gvt #1536

Closed

carolynvs mentioned this pull request Feb 9, 2018

Add flag to init to tweak how revision constraints are imported #1667

Closed

sdboyer previously approved these changes Feb 21, 2018

View reviewed changes

sdboyer merged commit 2ecf531 into golang:master Feb 21, 2018

Note diffs b/w imported and solved lock #1475

Note diffs b/w imported and solved lock #1475

Conversation

zmackie commented Dec 22, 2017 • edited Loading

What does this do / why do we need it?

What should your reviewer look out for in this PR?

Do you need help or clarification on anything?

Which issue(s) does this PR fix?

carolynvs left a comment • 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

carolynvs commented Dec 24, 2017

zmackie commented Dec 28, 2017

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 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

zmackie commented Jan 2, 2018

carolynvs left a comment

Choose a reason for hiding this comment

googlebot commented Jan 11, 2018

carolynvs commented Jan 11, 2018

zmackie commented Jan 11, 2018

carolynvs commented Jan 11, 2018

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

carolynvs commented Jan 18, 2018

carolynvs commented Jan 18, 2018

zmackie commented Jan 20, 2018

carolynvs commented Feb 3, 2018

sdboyer commented Feb 21, 2018

zmackie commented Dec 22, 2017 •

edited

Loading

carolynvs left a comment •

edited

Loading