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

Immediately handle error returned from importers #748

Merged
merged 1 commit into from
Jun 13, 2017

Conversation

darkowlzz
Copy link
Collaborator

What does this do / why do we need it?

This handles the returned error from the importer tools. Leaving this unhandled causes panic when importer fails.

@darkowlzz
Copy link
Collaborator Author

darkowlzz commented Jun 13, 2017

More context for proper solution.

So, I ran dep init in github.com/kubernetes/kubernetes/ and got a panic. The importer returned error due to invalid version.

null-5 is not a valid version for the package bitbucket.org/ww/goautoneg()

On digging further, the error was coming from the deduceConstraint() in godep_importer'sbuildProjectConstraint.

Just removing the null-5 from Godeps.json fixes the issue and the conversion happens without any more importer errors.

Question: When there's an invalid version (maybe cause of deleted tag or branch), how do we handle it? Let the user handle it by telling them about the invalid version or running another scan on the repository for any other tag/branch with the same revision?

Copy link
Collaborator

@carolynvs carolynvs left a comment

Choose a reason for hiding this comment

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

Doh! 😊

@carolynvs
Copy link
Collaborator

carolynvs commented Jun 13, 2017

Question: When there's an invalid version (maybe cause of deleted tag or branch), how do we handle it?

So far dep has avoided stopping execution and prompting the user for input, instead favoring printing a message telling the user what to do, e.g. "Do X and rerun dep init".

Scenario: Oops I locked to a deleted thing

  1. Long ago, in a galaxy far away, they used another tool, constraining a dep to the "vNext" branch and locking to revision "abc123".
  2. For reasons such as they haven't tried a clean build in a long time, they commited vendor/ and don't use the tool regularly anymore, or whatever, the bad config wasn't a problem for them.
  3. Now they are trying out dep, and suddenly this bad config matters. dep discovers that "vNext" doesn't exist anymore, and possibly that "abc123" doesn't exist either*.

My preference for handling this is:

  1. The importer warns that we had to ignore a constraint, and only locks to the revision. (I'm doing something similar in another importer-related PR)
  2. The rootAnalyzer checks after solving whether the locked revisions changed, and return an error if we can't replicate the same set of dependencies that the imported tool had in its lock. We don't do this currently, but as a user I'd be upset if I ran an import and it changed which versions I am locked to. In the error, recommend either rerunning with -skip-tools or manually editing the config files to address the missing revision.

* I think that in this case, since there is no constraint, that the solver wouldn't return the dreaded "reference is not a tree" error, and would instead try to pick another version.

@carolynvs
Copy link
Collaborator

Merging this fix and we can hash out the "proper solution" in a separate PR.

@carolynvs
Copy link
Collaborator

Okay I finally got around to making the follow-up issues: #907, #908 and #909. Go team!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants