Dep: Importers should not hard fail on errors

[carolynvs]

When bad config is encountered, the importer should print a warning, and skip to the next package, instead of hard failing. This is nice to have as part of #909, because if the import hard fails, nothing can be written. But it is absolutely required for importing during dep ensure.

Examples of hard fails that should be warnings:

• Unparsable config files
• Bad package names
• Bad revisions
• Errors encountered from gps looking up versions, guessing constraints, converting import paths to project roots, etc.

[sudo-suhas]

I'll try to work on this.

[sudo-suhas]

Hey @carolynvs, I have been trying to look at all the linked issues and PRs to better understand the goal here. Could you please help me and answer some doubts I have?

• By 'bad config', you mean config files of tools like glide right?
• Should I be looking to modify loadPackages? Is the convert function in each importer implementation also relevant?
• W.r.t gps, could you give me a bit more guidance? I am a little lost on what exactly I should be looking at.
• My understanding was that dep imports external config only during init. How does dep ensure come into the picture?

[carolynvs]

The goal of this issue is to alter import such that it never returns an error and any problems are treated as warnings. The first step is to stop returning errors from Import all together:

https://github.com/golang/dep/blob/77447101dfecfb2d266f028fadb31a0699ead729/internal/importers/importers.go#L28

becomes

Import(path string, pr gps.ProjectRoot) (*dep.Manifest, *dep.Lock)

By 'bad config', you mean config files of tools like glide right?

Yes, if the importer encounters invalid external configuration files they should attempt to recover and continue if possible. An example of a recoverable error would be something like this:

https://github.com/golang/dep/blob/77447101dfecfb2d266f028fadb31a0699ead729/internal/importers/glide/importer.go#L149-L151

In that case instead of returning an error, a warning is printed and the importer moves on to the next package.

Should I be looking to modify loadPackages? Is the convert function in each importer implementation also relevant?

Yes, ImportPackages and loadPackages on the base importer should never return errors. Same for the convert function for each importer.

W.r.t gps, could you give me a bit more guidance? I am a little lost on what exactly I should be looking at.

No changes should be made to the gps package. I was trying to say that the base importer executes functionality in gps (either directly or via the SourceManager field), and if any of those fail, again we should warn and attempt to continue.

My understanding was that dep imports external config only during init. How does dep ensure come into the picture?

Don't worry about ensure, this issue is just a prerequisite for enabling that feature later on. Currently dep only imports external configuration found in the root of the repository during init. We are working on a feature to enable importing external configuration found in dependencies during dep init and eventually during ensure as well. (the epic for this is #1335). So if a dependency never migrates to dep (or is just slow to do so), we can still take advantage of that dependency's glide/govendor/gb/etc config.

Hopefully that helps narrow down the scope of the issue! 😀

[sudo-suhas]

Thanks @carolynvs, this is very helpful. I am now much more clear on the scope of the issue and where the changes need to be made.

The first step is to stop returning errors from Import all together:

Errors are also used to indicate whether an operation was successful. In the rare scenario that something unexpected happened(ex: yaml parsing failed), how do we handle it? Is it enough to return nil for *dep.Manifest and *dep.Lock and check that? Would it be better to change only the locations which have recoverable errors(to not immediately return)? Also, should there be something like a limit on the number of recoverable errors?

if the importer encounters invalid external configuration files they should attempt to recover and continue if possible

Would it be better to delegate this to root_analyzer.go>importManifestAndLock?

[carolynvs]

Is it enough to return nil for *dep.Manifest and *dep.Lock

Yes, in case of catastrophic failure, we should log a warning and return nil manifest and lock.

should there be something like a limit on the number of recoverable errors

Let's focus on the immediate requirements of the issue first. There have been enough questions that I'm concerned that I completely underestimated the scope of this issue. So if we feel like we need to cap recoverable errors, I'd prefer to tackle that in a separate PR.

Would it be better to delegate this to root_analyzer.go>importManifestAndLock?

That spot should work as well.

[sudo-suhas]

Let's focus on the immediate requirements of the issue first. There have been enough questions that I'm concerned that I completely underestimated the scope of this issue.

Sure @carolynvs. I'll try to make a PR over the next few days.

There have been enough questions that I'm concerned that I completely underestimated the scope of this issue.

Yea, it's a bad habit of mine, my apologies 😅.

[carolynvs]

Oh! No, please ask questions, they are all very good. I just didn't realize how much work may be lurking in this issue until you asked! 😊

[sudo-suhas]

@carolynvs I have made a PR - #1474. I'd love to get some early feedback from you.