make importers error tolerant #1474

sudo-suhas · 2017-12-22T04:07:38Z

What does this do / why do we need it?

This makes the import process much more error tolerant. In case of a catastrophic failure, a warning is logged and the root analyzer gives up on importing the external config but only for that directory. On the other hand, for less serious errors such as failure to load glide.lock etc., no error is returned and only a warning is logged.

What should your reviewer look out for in this PR?

Is the approach in the correct direction? There's still work left to be done but I would like to get some early feedback.

Do you need help or clarification on anything?

While this behavior of bad config tolerance is a good default, I think there should be a way to turn it off if required. Do you agree with this and if so, should it be a part of this PR or a separate one? It might make sense to club this with the possibility of having a limit on the number of recoverable errors before erroring out.
Any tips for writing the tests? There are a lot of scenarios from what I can tell.

Which issue(s) does this PR fix?

fixes #1315

Code changelog

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.

tests:

Updated existing importer tests.
Added 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)

TODO:

Add and update tests
Address feedback
Update changelog

carolynvs

This is exactly what I was thinking! 👍

You are doing the right thing for testing the individual imports, we just need tests in the base importer for any warnings/skips we are doing there too.

If we can add a test for the root analyzer to handle a missing/bad config file (just pick 1 like glide), then that is adequate. If that isn't easy/straightforward, let's add that to the integration tests in https://github.com/golang/dep/tree/master/cmd/dep/testdata/harness_tests/init/glide to make sure that dep falls back to doing a standard import when the config is not loadable.

Don't worry about a "allowed number of errors" check or treating warnings as errors for now. I'd like to see this in action first, and we can consider those separately. I'm not sure either will be necessary yet.

carolynvs · 2017-12-23T23:51:18Z

internal/importers/base/importer.go

@@ -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: Cannot determine the project root for %s: %s\n",


Let's make it clear that we are skipping this project.

" Warning: Skipping project. Cannot determine ..."

carolynvs · 2017-12-24T00:02:43Z

internal/importers/glide/importer.go

@@ -147,7 +152,8 @@ 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: Invalid glide configuration: Name is required")


Let's make it clear that we are skipping (though yeah there's not much to show):

" Warning: Skipping package. Invalid glide configuration..."

carolynvs · 2017-12-24T00:03:19Z

internal/importers/glide/importer.go

@@ -172,7 +178,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: Invalid glide lock: Name is required")


Same comment about skipping as above.

carolynvs · 2017-12-24T00:03:57Z

internal/importers/glide/importer_test.go

@@ -125,7 +125,7 @@ func TestGlideConfig_Convert(t *testing.T) {
 			},
 			glideLock{},
 			importertest.TestCase{
-				WantConvertErr: true,
+				WantWarning: "Warning: Invalid glide configuration: Name is required",


This is exactly how we should test these changes. 👍

carolynvs · 2017-12-24T00:04:24Z

internal/importers/glock/importer.go

@@ -79,14 +80,19 @@ func (g *Importer) load(projectDir string) error {
 	for scanner.Scan() {
 		pkg, err := parseGlockLine(scanner.Text())
 		if err != nil {
-			return err
+			g.Logger.Printf("  Warning: Unable to parse line: %s\n", err)


Include the word "Skipping" in this somewhere.

carolynvs · 2017-12-24T00:05:35Z

internal/importers/glock/importer.go

 		}
 		if pkg == nil {
 			continue
 		}
 		g.packages = append(g.packages, *pkg)
 	}

+	if err := scanner.Err(); err != nil {


Is this something that you introduced? Or am I just missing where it was moved from?

I saw that it was present in the importer for vndr which uses a scanner as well:

dep/internal/importers/vndr/importer.go

Lines 78 to 80 in d9ce240

if scanner.Err() != nil {

return errors.Wrapf(err, "unable to read %s", vndrFile(dir))

}

I figured that either it should be present in both locations or neither. And logging a warning for an error seemed better.

Sounds good! I was just curious where it came from but now I see it in vndr. 👍

carolynvs · 2017-12-24T00:07:14Z

internal/importers/glock/importer.go

 		}

 		if pkg.revision == "" {
-			return nil, nil, errors.New("invalid glock configuration, revision is required")
+			g.Logger.Printf(


Since glock only provides revisions, if it's empty we should skip and continue to the next project.

I don't quite understand. If the revision isn't present we would just not have the LockHint. Isn't it better to still add the package? I would also need to do these changes in a few other places where I continue with importing the package even though the contraint/lock hint could not be resolved.

glock is unique because it doesn't provide a ConstraintHint or Source, just the LockHint. So if the imported config have a LockHint, then there's no point it continuing. Dep doesn't add "empty constraints" to the manifest during import, and the normal solve step will add the project to the lock if needed, so when the LockHint, ConstraintHint and Source are empty, it's a no-op.

The other tools may still have a ConstraintHint or Source, which is why we don't stop as soon as we know that the LockHint is empty.

Okay. I looked through the rest of the importers and the same scenario applies to govend as well:

dep/internal/importers/govend/importer.go

Lines 100 to 109 in c77daf6

if pkg.Revision == "" {

g.Logger.Printf(

" Warning: Invalid govend configuration, rev not found for Path %q\n", pkg.Path,

)

}

ip := base.ImportedPackage{

Name: pkg.Path,

LockHint: pkg.Revision,

}

Additionally, I do have a concern regarding this because the warning might be misleading. Like me, the user might also be confused by Skipping package... How about changing the warning to something like Warning: Skipping import with empty constraints. The solve step will add the dependency to the lock if needed.? too long?

carolynvs · 2017-12-24T00:09:44Z

internal/importers/vndr/importer.go

 	}
 	defer f.Close()

 	scanner := bufio.NewScanner(f)
 	for scanner.Scan() {
 		pkg, err := parseVndrLine(scanner.Text())
 		if err != nil {
-			return errors.Wrapf(err, "unable to parse line")
+			v.Logger.Printf("  Warning: Unable to parse line: %s\n", err)


Include the word "skipping" in the warning.

carolynvs · 2017-12-24T00:10:51Z

internal/importers/vndr/importer.go

@@ -75,25 +78,27 @@ func (v *Importer) loadVndrFile(dir string) error {
 		v.packages = append(v.packages, *pkg)
 	}

-	if scanner.Err() != nil {
-		return errors.Wrapf(err, "unable to read %s", vndrFile(dir))
+	if err := scanner.Err(); err != nil {


Let's make it clear that we are ignoring the errors found.

" Warning: Ignoring errors found while parsing %s..."

sudo-suhas · 2017-12-24T04:31:50Z

internal/importers/base/importer.go

@@ -132,7 +132,11 @@ func (i *Importer) loadPackages(packages []ImportedPackage) ([]importedProject,
 	for _, pkg := range packages {
 		pr, err := i.SourceManager.DeduceProjectRoot(pkg.Name)


@carolynvs When does SourceManager.DeduceProjectRoot return an error? I tried passing a package path which does not exist but that did not return an error.

invalid-project works. Let me know if you want me to use something else.

sudo-suhas · 2017-12-24T05:33:12Z

internal/importers/base/importer.go

-					version = nil
-					i.Logger.Println(err)
+			if isTag, version, err = i.isTag(pc.Ident, prj.LockHint); err != nil {
+				i.Logger.Printf(


I was looking to test this and from what I can tell, an error would be returned only if the package did not exist(or network errs but we aren't really testing for that). Perhaps it might be more appropriate to skip the project on encountering an error here?

sudo-suhas · 2017-12-27T04:21:14Z

@carolynvs I have pushed some commits which should address the feedback you had given. Other than that, I have the following questions which I mentioned in code comments above:

When we skip the package because there is no constraint present in the case of glock and govend, is the warning of Skipping package misleading? Would it be better to change that message to Warning: Skipping import with empty constraints. The solve step will add the dependency to the lock if needed.?
In the base importer, if isTag returns an error, what should we do? Because as far as I can tell, that'll happen only if the package is not resolvable(private git maybe?).

carolynvs · 2017-12-29T15:27:41Z

@sudo-suhas

Would it be better to change that message to Warning: Skipping import with empty constraints. The solve step will add the dependency to the lock if needed.?

I like your warning better, go with it. 👍

In the base importer, if isTag returns an error, what should we do?

In general anywhere that we used to return an error should now skip to the next project. So let's skip if that function returns an error.

carolynvs · 2017-12-29T15:59:22Z

internal/importers/glide/importer.go

@@ -152,7 +152,9 @@ func (g *Importer) convert(pr gps.ProjectRoot) (*dep.Manifest, *dep.Lock) {
 	for _, pkg := range append(g.glideConfig.Imports, g.glideConfig.TestImports...) {
 		// Validate
 		if pkg.Name == "" {
-			g.Logger.Println("  Warning: Invalid glide configuration: Name is required")
+			g.Logger.Println(
+				"  Warning: Skipping package. Invalid glide configuration, Name is required",


These should all say "Skipping project", not package.

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

@carolynvs

Address feedback from @carolynvs

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

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

carolynvs

Perfecto! ✨

sudo-suhas · 2018-01-12T00:11:57Z

Yay 🎉 and thanks for all the help!

sudo-suhas requested a review from carolynvs as a code owner December 22, 2017 04:07

googlebot added the cla: yes label Dec 22, 2017

sudo-suhas mentioned this pull request Dec 22, 2017

Importers should not hard fail on errors #1315

Closed

sudo-suhas force-pushed the err-tolerant-importers branch 3 times, most recently from eb7f1b4 to c77daf6 Compare December 23, 2017 06:45

carolynvs suggested changes Dec 24, 2017

View reviewed changes

sudo-suhas commented Dec 24, 2017

View reviewed changes

carolynvs suggested changes Dec 29, 2017

View reviewed changes

sudo-suhas force-pushed the err-tolerant-importers branch from 5f45022 to 57a5d17 Compare December 30, 2017 04:44

sudo-suhas changed the title ~~[WIP] make importers error tolerant~~ make importers error tolerant Dec 30, 2017

sudo-suhas added 6 commits January 5, 2018 11:28

update importer tests

e5a6715

improve importer warning msgs

de1c39e

Address feedback from @carolynvs

tweak importer warnings

432e0d1

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

update changelog

4c9c06d

sudo-suhas force-pushed the err-tolerant-importers branch from 57a5d17 to 4c9c06d Compare January 5, 2018 05:59

sudo-suhas requested a review from sdboyer as a code owner January 5, 2018 05:59

carolynvs approved these changes Jan 11, 2018

View reviewed changes

carolynvs merged commit 7bf4611 into golang:master Jan 11, 2018

sudo-suhas deleted the err-tolerant-importers branch January 12, 2018 00:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make importers error tolerant #1474

make importers error tolerant #1474

sudo-suhas commented Dec 22, 2017 •

edited

Loading

carolynvs left a comment

carolynvs Dec 23, 2017

carolynvs Dec 24, 2017

carolynvs Dec 24, 2017

carolynvs Dec 24, 2017

carolynvs Dec 24, 2017

carolynvs Dec 24, 2017

sudo-suhas Dec 24, 2017

carolynvs Dec 24, 2017

carolynvs Dec 24, 2017

sudo-suhas Dec 24, 2017

carolynvs Dec 24, 2017

sudo-suhas Dec 24, 2017

carolynvs Dec 24, 2017

carolynvs Dec 24, 2017

sudo-suhas Dec 24, 2017

sudo-suhas Dec 24, 2017 •

edited

Loading

sudo-suhas Dec 24, 2017

sudo-suhas commented Dec 27, 2017

carolynvs commented Dec 29, 2017

carolynvs Dec 29, 2017

carolynvs left a comment

sudo-suhas commented Jan 12, 2018

	if scanner.Err() != nil {
	return errors.Wrapf(err, "unable to read %s", vndrFile(dir))
	}

	if pkg.Revision == "" {
	g.Logger.Printf(
	" Warning: Invalid govend configuration, rev not found for Path %q\n", pkg.Path,
	)
	}

	ip := base.ImportedPackage{
	Name: pkg.Path,
	LockHint: pkg.Revision,
	}

		@@ -132,7 +132,11 @@ func (i *Importer) loadPackages(packages []ImportedPackage) ([]importedProject,
		for _, pkg := range packages {
		pr, err := i.SourceManager.DeduceProjectRoot(pkg.Name)

make importers error tolerant #1474

make importers error tolerant #1474

Conversation

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

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

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

sudo-suhas Dec 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sudo-suhas commented Dec 27, 2017

carolynvs commented Dec 29, 2017

Choose a reason for hiding this comment

carolynvs left a comment

Choose a reason for hiding this comment

sudo-suhas commented Jan 12, 2018

sudo-suhas commented Dec 22, 2017 •

edited

Loading

sudo-suhas Dec 24, 2017 •

edited

Loading