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

cmd/dep: fix vndrImporter's rev->constraint logic #1058

Merged
merged 5 commits into from
Aug 29, 2017
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
2 changes: 2 additions & 0 deletions cmd/dep/glide_importer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ func TestGlideConfig_Convert(t *testing.T) {
wantLockCount: 1,
wantConstraint: "^1.0.0",
wantVersion: "v1.0.0",
wantRevision: "ff2948a2ac8f538c4ecd55962e919d1e13e74baf",
},
},
"revision only": {
Expand All @@ -110,6 +111,7 @@ func TestGlideConfig_Convert(t *testing.T) {
projectRoot: "github.com/sdboyer/deptest",
wantLockCount: 1,
wantRevision: gps.Revision("ff2948a2ac8f538c4ecd55962e919d1e13e74baf"),
wantVersion: "v1.0.0",
},
},
"with ignored package": {
Expand Down
2 changes: 2 additions & 0 deletions cmd/dep/godep_importer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ func TestGodepConfig_Convert(t *testing.T) {
wantConstraint: "^1.12.0-12-g2fd980e",
wantLockCount: 1,
wantVersion: "v1.0.0",
wantRevision: "ff2948a2ac8f538c4ecd55962e919d1e13e74baf",
},
},
"empty comment": {
Expand Down Expand Up @@ -116,6 +117,7 @@ func TestGodepConfig_Convert(t *testing.T) {
wantLockCount: 1,
wantConstraint: "^1.0.0",
wantVersion: "v1.0.0",
wantRevision: "ff2948a2ac8f538c4ecd55962e919d1e13e74baf",
},
},
}
Expand Down
16 changes: 16 additions & 0 deletions cmd/dep/root_analyzer.go
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,22 @@ func (a *rootAnalyzer) Info() gps.ProjectAnalyzerInfo {
}
}

// isVersion determines if the specified value is a version/tag in the project.
func isVersion(pi gps.ProjectIdentifier, value string, sm gps.SourceManager) (bool, gps.Version, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's add a test for this function alone?

versions, err := sm.ListVersions(pi)
if err != nil {
return false, nil, errors.Wrapf(err, "list versions for %s(%s)", pi.ProjectRoot, pi.Source) // means repo does not exist
}

for _, version := range versions {
if value == version.String() {
return true, version, nil
}
}

return false, nil, nil
}

// lookupVersionForLockedProject figures out the appropriate version for a locked
// project based on the locked revision and the constraint from the manifest.
// First try matching the revision to a version, then try the constraint from the
Expand Down
37 changes: 19 additions & 18 deletions cmd/dep/root_analyzer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -229,26 +229,27 @@ func validateConvertTestCase(testCase *convertTestCase, manifest *dep.Manifest,
return errors.Errorf("Expected locked source to be %s, got '%s'", testCase.wantSourceRepo, p.Ident().Source)
}

if testCase.wantVersion != "" {
ver := p.Version().String()
if ver != testCase.wantVersion {
return errors.Errorf("Expected locked version to be '%s', got %s", testCase.wantVersion, ver)
}
// Break down the locked "version" into a version (optional) and revision
var gotVersion string
var gotRevision gps.Revision
if lpv, ok := p.Version().(gps.PairedVersion); ok {
gotVersion = lpv.String()
gotRevision = lpv.Revision()
} else if lr, ok := p.Version().(gps.Revision); ok {
gotRevision = lr
} else {
return errors.New("could not determine the type of the locked version")
}

if testCase.wantRevision != "" {
lv := p.Version()
lpv, ok := lv.(gps.PairedVersion)
if !ok {
return errors.Errorf("Expected locked version to be PairedVersion but got %T", lv)
}

rev := lpv.Revision()
if rev != testCase.wantRevision {
return errors.Errorf("Expected locked revision to be '%s', got %s",
testCase.wantRevision,
rev)
}
if gotRevision != testCase.wantRevision {
return errors.Errorf("Expected locked revision to be '%s', got %s",
testCase.wantRevision,
gotRevision)
}
if gotVersion != testCase.wantVersion {
return errors.Errorf("Expected locked version to be '%s', got %s",
testCase.wantVersion,
gotVersion)
}
}
return nil
Expand Down
4 changes: 2 additions & 2 deletions cmd/dep/testdata/vndr/golden.txt
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
Detected vndr configuration file...
Converting from vendor.conf...
Using 3f4c3bea144e112a69bbe5d8d01c1b09a544253f as initial hint for imported dep github.com/sdboyer/deptest
Using ^0.8.1 as initial constraint for imported dep github.com/sdboyer/deptest
Trying v0.8.1 (3f4c3be) as initial lock for imported dep github.com/sdboyer/deptest
Using ^2.0.0 as initial constraint for imported dep github.com/sdboyer/deptestdos
Trying * (v2.0.0) as initial lock for imported dep github.com/sdboyer/deptestdos
Trying v2.0.0 (5c60720) as initial lock for imported dep github.com/sdboyer/deptestdos
53 changes: 39 additions & 14 deletions cmd/dep/vndr_importer.go
Original file line number Diff line number Diff line change
Expand Up @@ -88,13 +88,28 @@ func (v *vndrImporter) convert(pr gps.ProjectRoot) (*dep.Manifest, *dep.Lock, er
var (
manifest = dep.NewManifest()
lock = &dep.Lock{}
err error
)

for _, pkg := range v.packages {
// ImportPath must not be empty
if pkg.importPath == "" {
err := errors.New("Invalid vndr configuration, missing import path")
err := errors.New("Invalid vndr configuration, import path is required")
return nil, nil, err
}

// Obtain ProjectRoot. Required for avoiding sub-package imports.
ip, err := v.sm.DeduceProjectRoot(pkg.importPath)
if err != nil {
return nil, nil, err
}
pkg.importPath = string(ip)

// Check if it already existing in locked projects
if projectExistsInLock(lock, ip) {
continue
}

if pkg.reference == "" {
err := errors.New("Invalid vndr configuration, revision is required")
return nil, nil, err
}

Expand All @@ -103,10 +118,27 @@ func (v *vndrImporter) convert(pr gps.ProjectRoot) (*dep.Manifest, *dep.Lock, er
ProjectRoot: gps.ProjectRoot(pkg.importPath),
Source: pkg.repository,
},
Constraint: gps.Any(),
}
pc.Constraint, err = v.sm.InferConstraint(pkg.revision, pc.Ident)

isVersion, version, err := isVersion(pc.Ident, pkg.reference, v.sm)
Copy link
Collaborator

@darkowlzz darkowlzz Aug 25, 2017

Choose a reason for hiding this comment

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

A comment here would be helpful. A reference could be a version or revision. So, we need to first check if the reference is a version.
I got confused for the need of this check 😅 then looked up an example vendor.conf file.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, the naming is confusing. I didn't even realize that a version could be stored in that field, which is why I renamed it to reference. I'll comment it up. 😀

if err != nil {
return nil, nil, errors.Wrapf(err, "Unable to interpret revision specifier '%s' for package %s", pkg.importPath, pkg.revision)
return nil, nil, err
}

// Check if the revision was tagged with a version
if !isVersion {
revision := gps.Revision(pkg.reference)
version, err = lookupVersionForLockedProject(pc.Ident, nil, revision, v.sm)
if err != nil {
v.logger.Println(err.Error())
}
}

// Try to build a constraint from the version
pp := getProjectPropertiesFromVersion(version)
if pp.Constraint != nil {
pc.Constraint = pp.Constraint
}

manifest.Constraints[pc.Ident.ProjectRoot] = gps.ProjectProperties{
Expand All @@ -115,14 +147,7 @@ func (v *vndrImporter) convert(pr gps.ProjectRoot) (*dep.Manifest, *dep.Lock, er
}
fb.NewConstraintFeedback(pc, fb.DepTypeImported).LogFeedback(v.logger)

revision := gps.Revision(pkg.revision)
version, err := lookupVersionForLockedProject(pc.Ident, pc.Constraint, revision, v.sm)
if err != nil {
v.logger.Println(err.Error())
}

lp := gps.NewLockedProject(pc.Ident, version, nil)

lock.P = append(lock.P, lp)
fb.NewLockedProjectFeedback(lp, fb.DepTypeImported).LogFeedback(v.logger)
}
Expand All @@ -132,7 +157,7 @@ func (v *vndrImporter) convert(pr gps.ProjectRoot) (*dep.Manifest, *dep.Lock, er

type vndrPackage struct {
importPath string
revision string
reference string
repository string
}

Expand All @@ -155,7 +180,7 @@ func parseVndrLine(line string) (*vndrPackage, error) {

pkg := &vndrPackage{
importPath: parts[0],
revision: parts[1],
reference: parts[1],
}
if len(parts) == 3 {
pkg.repository = parts[2]
Expand Down
Loading