Godep importer #742
Godep importer #742
Conversation
cmd/dep/godep_importer.go
Outdated
| if err != nil { | ||
| return nil, nil, err | ||
| } | ||
| manifest.Constraints[pc.Ident.ProjectRoot] = gps.ProjectProperties{Source: pc.Ident.Source, Constraint: pc.Constraint} |
There was a problem hiding this comment.
Does godep allow specifying an alternative source? I didn't see anything handling that in buildProjectConstraint. If not, we can skip setting the Source field on this line since it will always be "".
There was a problem hiding this comment.
I couldn't find any way to set alternative source in godep. There's an open issue for the same tools/godep#366 .
cmd/dep/godep_importer.go
Outdated
| } | ||
|
|
||
| // Rev must not be empty | ||
| if pkg.Rev == "" { |
There was a problem hiding this comment.
Let's add a unit test to verify error handling when rev is empty too.
cmd/dep/godep_importer.go
Outdated
| } | ||
|
|
||
| type godepJSON struct { | ||
| Name string `json:"ImportPath"` |
There was a problem hiding this comment.
What do you think about removing this field since it's not used for any of the conversion logic?
There was a problem hiding this comment.
yeah, doesn't seem to be useful until we decide to make it mandatory for a valid config file.
Removing :)
| Detected godep configuration files... | ||
| Converting from Godeps.json ... | ||
| Using ^0.8.1 as initial constraint for imported dep github.com/sdboyer/deptest | ||
| Trying ^0.8.1 (3f4c3be) as initial lock for imported dep github.com/sdboyer/deptest |
There was a problem hiding this comment.
This is just an FYI. In #715 I am working on fixing the version set in the lock, and once that goes in the godep importer should get that fix automatically. I'll make sure to verify that this test case is updated, as we shouldn't be locking to a version with an operator, i.e. it should be Trying v0.8.1 instead of Trying ^0.8.1 in the feedback. That was my mistake but should be fixed soon!
There was a problem hiding this comment.
@carolynvs #715 didn't change this?
There was a problem hiding this comment.
@darkowlzz You just saved my 🥓 ! I missed a spot in godep's importer. Fixing...
|
Removed Also, added a few checks to:
|
darkowlzz
force-pushed
the
godep-importer
branch
from
8f58ee6
to
0ac12ad
Compare
| } | ||
|
|
||
| // Obtain ProjectRoot. Required for avoiding sub-package imports. | ||
| ip, err := g.sm.DeduceProjectRoot(pkg.ImportPath) |
There was a problem hiding this comment.
What prompted you to check for sub-package imports? I don't remember hitting this with the glide importer and am wondering if this is a general concern for all importers or is godep specific?
There was a problem hiding this comment.
Ah okay after scrolling down to the test, it makes sense. Glide lists unique packages and then lists the subpackages used separately which explains why I didn't have to check for this. 😁 Carry on!
- Adds skipping duplicate and sub-package imports using projectExistsInLock(). - Adds tests for missing Rev in godep import package list. - Removes `Name` field from godepImporter.
darkowlzz
force-pushed
the
godep-importer
branch
from
0ac12ad
to
d2109d3
Compare
|
Seems to be ready 😊 |
|
🎉💃🏻🚀 |
|
woohoo!! 🎉 🚀 🎊 |
What does this do / why do we need it?
Adds godep config importer support to init.
What should your reviewer look out for in this PR?
Importer logic and test cases.
Which issue(s) does this PR fix?
fixes #577