Skip to content
This repository was archived by the owner on Sep 9, 2020. It is now read-only.

Note diffs b/w imported and solved lock #1475

Merged
merged 10 commits into from
Feb 21, 2018
Merged

Note diffs b/w imported and solved lock #1475

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
c92b728
Create BrokenImportFeedback and use in FinalizeRootManifest
zmackie Dec 28, 2017
6d1ebdc
Add comments for new methods
zmackie Dec 28, 2017
14f1ae8
Use trimSHA in GetUsingFeedback
zmackie Dec 29, 2017
fd6ef50
BrokenImportFeedback track modifications to locks and branches and so…
zmackie Jan 2, 2018
c2c59f1
Update BrokenImportFeedback tests for easier parameterizing
zmackie Jan 2, 2018
ce07d2e
Add test cases for BrokenImportFeedback
zmackie Jan 2, 2018
502b4a3
Make lint happy
carolynvs Jan 11, 2018
16c38f4
Don't warn about import diffs when it's only packages
carolynvs-msft Jan 17, 2018
9075212
Sprintf variables on same line
zmackie Jan 20, 2018
e910026
BrokenImportFeedback docs complete sentences
zmackie Jan 20, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions cmd/dep/root_analyzer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,9 @@ func (a *rootAnalyzer) DeriveManifestAndLock(dir string, pr gps.ProjectRoot) (gp
func (a *rootAnalyzer) FinalizeRootManifestAndLock(m *dep.Manifest, l *dep.Lock, ol dep.Lock) {
// Iterate through the new projects in solved lock and add them to manifest
// if they are direct deps and log feedback for all the new projects.
diff := gps.DiffLocks(&ol, l)
bi := fb.NewBrokenImportFeedback(diff)
bi.LogFeedback(a.ctx.Err)
for _, y := range l.Projects() {
var f *fb.ConstraintFeedback
pr := y.Ident().ProjectRoot
Expand Down
137 changes: 130 additions & 7 deletions internal/feedback/feedback.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,123 @@ func (cf ConstraintFeedback) LogFeedback(logger *log.Logger) {
}
}

type brokenImport interface {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

String() string
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  • source
  • branch
  • version
  • revision


type modifiedImport struct {
source, branch, revision, version *gps.StringDiff
projectPath string
}

func (mi modifiedImport) String() string {
var pv string
var pr string
pp := mi.projectPath

var cr string
var cv string
cp := ""

if mi.revision != nil {
pr = fmt.Sprintf("(%s)", trimSHA(mi.revision.Previous))
cr = fmt.Sprintf("(%s)", trimSHA(mi.revision.Current))
}

if mi.version != nil {
pv = mi.version.Previous
cv = mi.version.Current
} else if mi.branch != nil {
pv = mi.branch.Previous
cv = mi.branch.Current
}

if mi.source != nil {
pp = fmt.Sprintf("%s(%s)", mi.projectPath, mi.source.Previous)
cp = fmt.Sprintf(" for %s(%s)", mi.projectPath, mi.source.Current)
}

// 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. Locking in %v %s%s",
pv,
pr,
pp,
cv,
cr,
cp,
)
}

type removedImport struct {
source, branch, revision, version *gps.StringDiff
projectPath string
}

func (ri removedImport) String() string {
var pr string
var pv string
pp := ri.projectPath

if ri.revision != nil {
pr = fmt.Sprintf("(%s)", trimSHA(ri.revision.Previous))
}

if ri.version != nil {
pv = ri.version.Previous
} else if ri.branch != nil {
pv = ri.branch.Previous
}

if ri.source != nil {
pp = fmt.Sprintf("%s(%s)", ri.projectPath, ri.source.Previous)
}

// 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.",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

pv,
pr,
pp,
)
}

// BrokenImportFeedback holds problematic lock feedback data
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

type BrokenImportFeedback struct {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

brokenImports []brokenImport
}

// NewBrokenImportFeedback builds a feedback entry for problems with imports from a diff of the pre- and post- solved locks
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

func NewBrokenImportFeedback(ld *gps.LockDiff) *BrokenImportFeedback {
bi := &BrokenImportFeedback{}
for _, lpd := range ld.Modify {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

bi.brokenImports = append(bi.brokenImports, modifiedImport{
projectPath: string(lpd.Name),
source: lpd.Source,
branch: lpd.Branch,
revision: lpd.Revision,
version: lpd.Version,
})
}

for _, lpd := range ld.Remove {
bi.brokenImports = append(bi.brokenImports, removedImport{
projectPath: string(lpd.Name),
source: lpd.Source,
branch: lpd.Branch,
revision: lpd.Revision,
version: lpd.Version,
})
}

return bi
}

// LogFeedback logs a warning for all changes between the initially imported and post- solve locks
func (b BrokenImportFeedback) LogFeedback(logger *log.Logger) {
for _, bi := range b.brokenImports {
logger.Printf("Warning: Unable to preserve imported lock %v\n", bi)
}
}

// GetUsingFeedback returns a dependency "using" feedback message. For example:
//
// Using ^1.0.0 as constraint for direct dep github.com/foo/bar
Expand All @@ -99,13 +216,7 @@ func GetUsingFeedback(version, consType, depType, projectPath string) string {
// Locking in v1.1.4 (bc29b4f) for direct dep github.com/foo/bar
// Locking in master (436f39d) for transitive dep github.com/baz/qux
func GetLockingFeedback(version, revision, depType, projectPath string) string {
// Check if it's a valid SHA1 digest and trim to 7 characters.
if len(revision) == 40 {
if _, err := hex.DecodeString(revision); err == nil {
// Valid SHA1 digest
revision = revision[0:7]
}
}
revision = trimSHA(revision)

if depType == DepTypeImported {
if version == "" {
Expand All @@ -115,3 +226,15 @@ func GetLockingFeedback(version, revision, depType, projectPath string) string {
}
return fmt.Sprintf("Locking in %s (%s) for %s %s", version, revision, depType, projectPath)
}

// trimSHA checks if revision is a valid SHA1 digest and trims to 7 characters.
func trimSHA(revision string) string {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

if len(revision) == 40 {
if _, err := hex.DecodeString(revision); err == nil {
// Valid SHA1 digest
revision = revision[0:7]
}
}

return revision
}
67 changes: 67 additions & 0 deletions internal/feedback/feedback_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"strings"
"testing"

"github.com/golang/dep"
"github.com/golang/dep/gps"
)

Expand Down Expand Up @@ -92,3 +93,69 @@ func TestFeedback_LockedProject(t *testing.T) {
}
}
}

func TestFeedback_BrokenImport(t *testing.T) {
pi := gps.ProjectIdentifier{ProjectRoot: gps.ProjectRoot("github.com/foo/bar")}

cases := []struct {
diff *gps.LockDiff
oldVersion gps.Version
currentVersion gps.Version
pID gps.ProjectIdentifier
altPID gps.ProjectIdentifier
want string
name string
}{
{
oldVersion: gps.NewVersion("v1.1.4").Pair("bc29b4f"),
currentVersion: gps.NewVersion("v1.2.0").Pair("ia3da28"),
pID: pi,
altPID: pi,
want: "Warning: Unable to preserve imported lock v1.1.4 (bc29b4f) for github.com/foo/bar. Locking in v1.2.0 (ia3da28)",
name: "Basic broken import",
},
{
oldVersion: gps.NewBranch("master").Pair("bc29b4f"),
currentVersion: gps.NewBranch("dev").Pair("ia3da28"),
pID: pi,
altPID: pi,
want: "Warning: Unable to preserve imported lock master (bc29b4f) for github.com/foo/bar. Locking in dev (ia3da28)",
name: "Branches",
},
{
oldVersion: gps.NewBranch("master").Pair("bc29b4f"),
currentVersion: gps.NewBranch("dev").Pair("ia3da28"),
pID: pi,
altPID: gps.ProjectIdentifier{ProjectRoot: gps.ProjectRoot("github.com/foo/boo")},
want: "Warning: Unable to preserve imported lock master (bc29b4f) for github.com/foo/bar. The project was removed from the lock because it is not used.",
name: "Branches",
},
{
oldVersion: gps.NewBranch("master").Pair("bc29b4f"),
currentVersion: gps.NewBranch("dev").Pair("ia3da28"),
pID: gps.ProjectIdentifier{ProjectRoot: gps.ProjectRoot("github.com/foo/boo"), Source: "github.com/das/foo"},
altPID: gps.ProjectIdentifier{ProjectRoot: gps.ProjectRoot("github.com/foo/boo"), Source: "github.com/das/bar"},
want: "Warning: Unable to preserve imported lock master (bc29b4f) for github.com/foo/boo(github.com/das/foo). Locking in dev (ia3da28) for github.com/foo/boo(github.com/das/bar)",
name: "With a source",
},
}

for _, c := range cases {
t.Run(c.name, func(t *testing.T) {
buf := &bytes.Buffer{}
ol := dep.Lock{
P: []gps.LockedProject{gps.NewLockedProject(c.pID, c.oldVersion, nil)},
}
l := dep.Lock{
P: []gps.LockedProject{gps.NewLockedProject(c.altPID, c.currentVersion, nil)},
}
log := log2.New(buf, "", 0)
feedback := NewBrokenImportFeedback(gps.DiffLocks(&ol, &l))
feedback.LogFeedback(log)
got := strings.TrimSpace(buf.String())
if c.want != got {
t.Errorf("Feedbacks are not expected: \n\t(GOT) '%s'\n\t(WNT) '%s'", got, c.want)
}
})
}
}