make importers error tolerant (#1474)

* make importers error tolerant

- root_analyzer.go: Log a warning on encountering an unrecoverable error during
  import of external config and proceed with the import for other packages.
  Do not return error from private func `importManifestAndLock`.

internal/importers:
- base/importer.go:
  - `loadPackages`: Do not return error. If `SourceManager.DeduceProjectRoot`
    fails to determine the project root, log a warning and continue with the
    rest of the imported packages.
  - `ImportPackages`: Do not return error. When constraint resolution from the
    lock hint fails, only log a warning.
- Importer implementations:
  - Return an error from `Import` only for catastrophic failures(ex: yaml
    parsing failed).
  - `load`: Make it more error tolerant. Log warnings only for any of the
    following scenarios:
    - When and if a lock file, separate from the dependency file is present,
      like, in the case of glide, and parsing fails. Continue with the import
      as if the lock file was not present.
    - If import packages are parsed line by line like in the case of glock,
      and one of the line could not be parsed.
  - `convert`: Do not return an error. Log warnings only for any of the
    following scenarios:
    - Expected field, such as `package` for an entry in `glide.yaml>imports`
      is not present.
    - Package was specified but the contraint could not be found.

* update importer tests

* improve importer warning msgs

Address feedback from @carolynvs

* add tests for importer failure scenarios

- internal/importers/base/importer_test.go:
  - Check for warning when an invalid project is present whose project root
    cannot be parsed.
  - Check for warning when lock hint cannot be resolved correctly and the
    constraint cannot be applied.
- Integration test for malformed external config(glide.yaml)

* tweak importer warnings

- {package => project}
- Improve warning message when no constraint is found for the package being
  imported.

* update changelog

CHANGELOG.md

@@ -28,6 +28,7 @@ IMPROVEMENTS:
 * Skip empty constraints during import ([#1414](https://github.com/golang/dep/pull/1349))
 * Handle errors when writing status output ([#1420](https://github.com/golang/dep/pull/1420))
 * Add constraint for locked projects in `status`. (#962)
+* Make external config importers error tolerant. ([#1315](https://github.com/golang/dep/pull/1315))
 
 # v0.3.2
 

cmd/dep/root_analyzer.go

@@ -40,10 +40,7 @@ func newRootAnalyzer(skipTools bool, ctx *dep.Ctx, directDeps map[string]bool, s
 
 func (a *rootAnalyzer) InitializeRootManifestAndLock(dir string, pr gps.ProjectRoot) (rootM *dep.Manifest, rootL *dep.Lock, err error) {
 	if !a.skipTools {
-		rootM, rootL, err = a.importManifestAndLock(dir, pr, false)
-		if err != nil {
-			return
-		}
+		rootM, rootL = a.importManifestAndLock(dir, pr, false)
 	}
 
 	if rootM == nil {
@@ -106,7 +103,7 @@ func (a *rootAnalyzer) cacheDeps(pr gps.ProjectRoot) error {
 	return nil
 }
 
-func (a *rootAnalyzer) importManifestAndLock(dir string, pr gps.ProjectRoot, suppressLogs bool) (*dep.Manifest, *dep.Lock, error) {
+func (a *rootAnalyzer) importManifestAndLock(dir string, pr gps.ProjectRoot, suppressLogs bool) (*dep.Manifest, *dep.Lock) {
 	logger := a.ctx.Err
 	if suppressLogs {
 		logger = log.New(ioutil.Discard, "", 0)
@@ -117,16 +114,20 @@ func (a *rootAnalyzer) importManifestAndLock(dir string, pr gps.ProjectRoot, sup
 			a.ctx.Err.Printf("Importing configuration from %s. These are only initial constraints, and are further refined during the solve process.", i.Name())
 			m, l, err := i.Import(dir, pr)
 			if err != nil {
-				return nil, nil, err
+				a.ctx.Err.Printf(
+					"Warning: Encountered an unrecoverable error while trying to import %s config from %q: %s",
+					i.Name(), dir, err,
+				)
+				break
 			}
 			a.removeTransitiveDependencies(m)
-			return m, l, err
+			return m, l
 		}
 	}
 
 	var emptyManifest = dep.NewManifest()
 
-	return emptyManifest, nil, nil
+	return emptyManifest, nil
 }
 
 func (a *rootAnalyzer) removeTransitiveDependencies(m *dep.Manifest) {
@@ -151,14 +152,14 @@ func (a *rootAnalyzer) DeriveManifestAndLock(dir string, pr gps.ProjectRoot) (gp
 		// The assignment back to an interface prevents interface-based nil checks from failing later
 		var manifest gps.Manifest = gps.SimpleManifest{}
 		var lock gps.Lock
-		im, il, err := a.importManifestAndLock(dir, pr, true)
+		im, il := a.importManifestAndLock(dir, pr, true)
 		if im != nil {
 			manifest = im
 		}
 		if il != nil {
 			lock = il
 		}
-		return manifest, lock, err
+		return manifest, lock, nil
 	}
 
 	return gps.SimpleManifest{}, nil, nil

cmd/dep/testdata/harness_tests/init/glide/case4/README.md

@@ -0,0 +1 @@
+Ignore glide config if glide.yaml is malformed and cannot be parsed correctly.

cmd/dep/testdata/harness_tests/init/glide/case4/final/Gopkg.toml

@@ -0,0 +1,4 @@
+
+[[constraint]]
+  name = "github.com/sdboyer/deptestdos"
+  version = "2.0.0"

cmd/dep/testdata/harness_tests/init/glide/case4/initial/glide.yaml

@@ -0,0 +1,20 @@
+'package: github.com/golang/notexist
+homepage: http://example.com
+license: MIT
+owners:
+- name: Sam Boyer
+  email: [email protected]
+  homepage: http://sdboyer.io
+ignore:
+- github.com/sdboyer/dep-test
+excludeDirs:
+- samples
+import:
+- package: github.com/sdboyer/deptest # This is a transitive dep and will be ignored
+  repo: https://github.com/sdboyer/deptest.git
+  vcs: git
+  version: v1.0.0
+- package: github.com/sdboyer/deptestdos
+  version: v2.0.0
+testImport:
+- package: github.com/golang/lint

cmd/dep/testdata/harness_tests/init/glide/case4/initial/main.go

@@ -0,0 +1,16 @@
+// Copyright 2017 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package main
+
+import (
+	"fmt"
+
+	"github.com/sdboyer/deptestdos"
+)
+
+func main() {
+	var x deptestdos.Bar
+	fmt.Println(x)
+}

cmd/dep/testdata/harness_tests/init/glide/case4/testcase.json

@@ -0,0 +1,14 @@
+{
+  "commands": [
+    ["init", "-no-examples"]
+  ],
+  "error-expected": "",
+  "gopath-initial": {
+    "github.com/sdboyer/deptest": "3f4c3bea144e112a69bbe5d8d01c1b09a544253f",
+    "github.com/sdboyer/deptestdos": "5c607206be5decd28e6263ffffdcee067266015e"
+  },
+  "vendor-final": [
+    "github.com/sdboyer/deptest",
+    "github.com/sdboyer/deptestdos"
+  ]
+}

internal/importers/base/importer.go

@@ -123,7 +123,7 @@ type importedProject struct {
 }
 
 // loadPackages consolidates all package references into a set of project roots.
-func (i *Importer) loadPackages(packages []ImportedPackage) ([]importedProject, error) {
+func (i *Importer) loadPackages(packages []ImportedPackage) []importedProject {
 	// preserve the original order of the packages so that messages that
 	// are printed as they are processed are in a consistent order.
 	orderedProjects := make([]importedProject, 0, len(packages))
@@ -132,7 +132,11 @@ func (i *Importer) loadPackages(packages []ImportedPackage) ([]importedProject,
 	for _, pkg := range packages {
 		pr, err := i.SourceManager.DeduceProjectRoot(pkg.Name)
 		if err != nil {
-			return nil, errors.Wrapf(err, "Cannot determine the project root for %s", pkg.Name)
+			i.Logger.Printf(
+				"  Warning: Skipping project. Cannot determine the project root for %s: %s\n",
+				pkg.Name, err,
+			)
+			continue
 		}
 		pkg.Name = string(pr)
 
@@ -159,7 +163,7 @@ func (i *Importer) loadPackages(packages []ImportedPackage) ([]importedProject,
 		}
 	}
 
-	return orderedProjects, nil
+	return orderedProjects
 }
 
 // ImportPackages loads imported packages into the manifest and lock.
@@ -173,11 +177,8 @@ func (i *Importer) loadPackages(packages []ImportedPackage) ([]importedProject,
 // * Revision constraints are ignored.
 // * Versions that don't satisfy the constraint, drop the constraint.
 // * Untagged revisions ignore non-branch constraint hints.
-func (i *Importer) ImportPackages(packages []ImportedPackage, defaultConstraintFromLock bool) (err error) {
-	projects, err := i.loadPackages(packages)
-	if err != nil {
-		return err
-	}
+func (i *Importer) ImportPackages(packages []ImportedPackage, defaultConstraintFromLock bool) {
+	projects := i.loadPackages(packages)
 
 	for _, prj := range projects {
 		source := prj.Source
@@ -201,6 +202,7 @@ func (i *Importer) ImportPackages(packages []ImportedPackage, defaultConstraintF
 			},
 		}
 
+		var err error
 		pc.Constraint, err = i.SourceManager.InferConstraint(prj.ConstraintHint, pc.Ident)
 		if err != nil {
 			pc.Constraint = gps.Any()
@@ -212,9 +214,12 @@ func (i *Importer) ImportPackages(packages []ImportedPackage, defaultConstraintF
 			// Determine if the lock hint is a revision or tag
 			isTag, version, err = i.isTag(pc.Ident, prj.LockHint)
 			if err != nil {
-				return err
+				i.Logger.Printf(
+					"  Warning: Skipping project. Unable to apply constraint %q for %v: %s\n",
+					prj.LockHint, pc.Ident, err,
+				)
+				continue
 			}
-
 			// If the hint is a revision, check if it is tagged
 			if !isTag {
 				revision := gps.Revision(prj.LockHint)
@@ -270,8 +275,6 @@ func (i *Importer) ImportPackages(packages []ImportedPackage, defaultConstraintF
 			fb.NewLockedProjectFeedback(lp, fb.DepTypeImported).LogFeedback(i.Logger)
 		}
 	}
-
-	return nil
 }
 
 // isConstraintPinned returns if a constraint is pinned to a specific revision.

internal/importers/base/importer_test.go

@@ -5,6 +5,7 @@
 package base
 
 import (
+	"fmt"
 	"log"
 	"testing"
 
@@ -399,16 +400,42 @@ func TestBaseImporter_ImportProjects(t *testing.T) {
 				},
 			},
 		},
+		"invalid project root": {
+			importertest.TestCase{
+				WantSourceRepo: "",
+				WantWarning:    "Warning: Skipping project. Cannot determine the project root for invalid-project",
+			},
+			[]ImportedPackage{
+				{
+					Name: "invalid-project",
+				},
+			},
+		},
+		"nonexistent project": {
+			importertest.TestCase{
+				WantSourceRepo: "",
+				WantWarning: fmt.Sprintf(
+					"Warning: Skipping project. Unable to apply constraint %q for %s",
+					importertest.V1Tag, importertest.NonexistentPrj,
+				),
+			},
+			[]ImportedPackage{
+				{
+					Name:     importertest.NonexistentPrj,
+					LockHint: importertest.V1Tag,
+				},
+			},
+		},
 	}
 
 	for name, tc := range testcases {
 		name := name
 		tc := tc
 		t.Run(name, func(t *testing.T) {
-			err := tc.Execute(t, func(logger *log.Logger, sm gps.SourceManager) (*dep.Manifest, *dep.Lock, error) {
+			err := tc.Execute(t, func(logger *log.Logger, sm gps.SourceManager) (*dep.Manifest, *dep.Lock) {
 				i := NewImporter(logger, true, sm)
-				convertErr := i.ImportPackages(tc.projects, tc.DefaultConstraintFromLock)
-				return i.Manifest, i.Lock, convertErr
+				i.ImportPackages(tc.projects, tc.DefaultConstraintFromLock)
+				return i.Manifest, i.Lock
 			})
 			if err != nil {
 				t.Fatalf("%#v", err)

internal/importers/glide/importer.go

@@ -89,10 +89,13 @@ func (g *Importer) Import(dir string, pr gps.ProjectRoot) (*dep.Manifest, *dep.L
 		return nil, nil, err
 	}
 
-	return g.convert(pr)
+	m, l := g.convert(pr)
+	return m, l, nil
 }
 
-// load the glide configuration files.
+// load the glide configuration files. Failure to load `glide.yaml` is considered
+// unrecoverable and an error is returned for it. But if there is any error while trying
+// to load the lock file, only a warning is logged.
 func (g *Importer) load(projectDir string) error {
 	g.Logger.Println("Detected glide configuration files...")
 	y := filepath.Join(projectDir, glideYamlName)
@@ -113,24 +116,26 @@ func (g *Importer) load(projectDir string) error {
 		if g.Verbose {
 			g.Logger.Printf("  Loading %s", l)
 		}
-		g.lockFound = true
 		lb, err := ioutil.ReadFile(l)
 		if err != nil {
-			return errors.Wrapf(err, "unable to read %s", l)
+			g.Logger.Printf("  Warning: Ignoring lock file. Unable to read %s: %s\n", l, err)
+			return nil
 		}
 		lock := glideLock{}
 		err = yaml.Unmarshal(lb, &lock)
 		if err != nil {
-			return errors.Wrapf(err, "unable to parse %s", l)
+			g.Logger.Printf("  Warning: Ignoring lock file. Unable to parse %s: %s\n", l, err)
+			return nil
 		}
+		g.lockFound = true
 		g.glideLock = lock
 	}
 
 	return nil
 }
 
 // convert the glide configuration files into dep configuration files.
-func (g *Importer) convert(pr gps.ProjectRoot) (*dep.Manifest, *dep.Lock, error) {
+func (g *Importer) convert(pr gps.ProjectRoot) (*dep.Manifest, *dep.Lock) {
 	projectName := string(pr)
 
 	task := bytes.NewBufferString("Converting from glide.yaml")
@@ -147,7 +152,10 @@ func (g *Importer) convert(pr gps.ProjectRoot) (*dep.Manifest, *dep.Lock, error)
 	for _, pkg := range append(g.glideConfig.Imports, g.glideConfig.TestImports...) {
 		// Validate
 		if pkg.Name == "" {
-			return nil, nil, errors.New("invalid glide configuration: Name is required")
+			g.Logger.Println(
+				"  Warning: Skipping project. Invalid glide configuration, Name is required",
+			)
+			continue
 		}
 
 		// Warn
@@ -172,7 +180,8 @@ func (g *Importer) convert(pr gps.ProjectRoot) (*dep.Manifest, *dep.Lock, error)
 	for _, pkg := range append(g.glideLock.Imports, g.glideLock.TestImports...) {
 		// Validate
 		if pkg.Name == "" {
-			return nil, nil, errors.New("invalid glide lock: Name is required")
+			g.Logger.Println("  Warning: Skipping project. Invalid glide lock, Name is required")
+			continue
 		}
 
 		ip := base.ImportedPackage{
@@ -183,10 +192,7 @@ func (g *Importer) convert(pr gps.ProjectRoot) (*dep.Manifest, *dep.Lock, error)
 		packages = append(packages, ip)
 	}
 
-	err := g.ImportPackages(packages, false)
-	if err != nil {
-		return nil, nil, errors.Wrap(err, "invalid glide configuration")
-	}
+	g.ImportPackages(packages, false)
 
 	// Ignores
 	g.Manifest.Ignored = append(g.Manifest.Ignored, g.glideConfig.Ignores...)
@@ -201,5 +207,5 @@ func (g *Importer) convert(pr gps.ProjectRoot) (*dep.Manifest, *dep.Lock, error)
 		}
 	}
 
-	return g.Manifest, g.Lock, nil
+	return g.Manifest, g.Lock
 }

internal/importers/glide/importer_test.go

@@ -125,7 +125,7 @@ func TestGlideConfig_Convert(t *testing.T) {
 			},
 			glideLock{},
 			importertest.TestCase{
-				WantConvertErr: true,
+				WantWarning: "Warning: Skipping project. Invalid glide configuration, Name is required",
 			},
 		},
 		"warn unused os field": {
@@ -160,7 +160,7 @@ func TestGlideConfig_Convert(t *testing.T) {
 		name := name
 		testCase := testCase
 		t.Run(name, func(t *testing.T) {
-			err := testCase.Execute(t, func(logger *log.Logger, sm gps.SourceManager) (*dep.Manifest, *dep.Lock, error) {
+			err := testCase.Execute(t, func(logger *log.Logger, sm gps.SourceManager) (*dep.Manifest, *dep.Lock) {
 				g := NewImporter(logger, true, sm)
 				g.glideConfig = testCase.yaml
 				g.glideLock = testCase.lock