Merge pull request #1058 from carolynvs/vndr-val-subtests

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

Author: darkowlzz

cmd/dep/glide_importer_test.go

@@ -88,6 +88,7 @@ func TestGlideConfig_Convert(t *testing.T) {
 				wantLockCount:  1,
 				wantConstraint: "^1.0.0",
 				wantVersion:    "v1.0.0",
+				wantRevision:   "ff2948a2ac8f538c4ecd55962e919d1e13e74baf",
 			},
 		},
 		"revision only": {
@@ -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": {

cmd/dep/godep_importer_test.go

@@ -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": {
@@ -116,6 +117,7 @@ func TestGodepConfig_Convert(t *testing.T) {
 				wantLockCount:  1,
 				wantConstraint: "^1.0.0",
 				wantVersion:    "v1.0.0",
+				wantRevision:   "ff2948a2ac8f538c4ecd55962e919d1e13e74baf",
 			},
 		},
 	}

cmd/dep/root_analyzer.go

@@ -173,6 +173,26 @@ 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) {
+	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 version.Type() != gps.IsVersion && version.Type() != gps.IsSemver {
+			continue
+		}
+
+		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

cmd/dep/root_analyzer_test.go

@@ -154,6 +154,41 @@ func TestProjectExistsInLock(t *testing.T) {
 	}
 }
 
+func TestIsVersion(t *testing.T) {
+	testcases := map[string]struct {
+		wantIsVersion bool
+		wantVersion   gps.Version
+	}{
+		"v1.0.0": {wantIsVersion: true, wantVersion: gps.NewVersion("v1.0.0").Pair("ff2948a2ac8f538c4ecd55962e919d1e13e74baf")},
+		"3f4c3bea144e112a69bbe5d8d01c1b09a544253f": {wantIsVersion: false},
+		"master": {wantIsVersion: false},
+	}
+
+	pi := gps.ProjectIdentifier{ProjectRoot: gps.ProjectRoot("github.com/sdboyer/deptest")}
+	h := test.NewHelper(t)
+	defer h.Cleanup()
+
+	ctx := newTestContext(h)
+	sm, err := ctx.SourceManager()
+	h.Must(err)
+	defer sm.Release()
+
+	for value, testcase := range testcases {
+		t.Run(value, func(t *testing.T) {
+			gotIsVersion, gotVersion, err := isVersion(pi, value, sm)
+			h.Must(err)
+
+			if testcase.wantIsVersion != gotIsVersion {
+				t.Fatalf("unexpected isVersion result for %s: \n\t(GOT) %v \n\t(WNT) %v", value, gotIsVersion, testcase.wantIsVersion)
+			}
+
+			if testcase.wantVersion != gotVersion {
+				t.Fatalf("unexpected version for %s: \n\t(GOT) %v \n\t(WNT) %v", value, testcase.wantVersion, gotVersion)
+			}
+		})
+	}
+}
+
 // convertTestCase is a common set of validations applied to the result
 // of an importer converting from an external config format to dep's.
 type convertTestCase struct {
@@ -229,26 +264,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("unexpected locked revision : \n\t(GOT) %v \n\t(WNT) %v",
+				testCase.wantRevision,
+				gotRevision)
+		}
+		if gotVersion != testCase.wantVersion {
+			return errors.Errorf("unexpected locked version: \n\t(GOT) %v \n\t(WNT) %v",
+				testCase.wantVersion,
+				gotVersion)
 		}
 	}
 	return nil

cmd/dep/testdata/vndr/golden.txt

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

cmd/dep/vndr_importer.go

@@ -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
 		}
 
@@ -103,10 +118,28 @@ 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)
+
+		// A vndr entry could contain either a version or a revision
+		isVersion, version, err := isVersion(pc.Ident, pkg.reference, v.sm)
 		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
+		}
+
+		// If the reference is a revision, check if it is 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{
@@ -115,14 +148,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)
 	}
@@ -132,7 +158,7 @@ func (v *vndrImporter) convert(pr gps.ProjectRoot) (*dep.Manifest, *dep.Lock, er
 
 type vndrPackage struct {
 	importPath string
-	revision   string
+	reference  string
 	repository string
 }
 
@@ -155,7 +181,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]