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

Add importer for github.com/LK4D4/vndr #978

Merged
merged 7 commits into from
Aug 11, 2017

Conversation

spenczar
Copy link
Contributor

@spenczar spenczar commented Aug 8, 2017

This was motivated by working on something that imports github.com/moby/moby, which uses github.com/LK4D4/vndr. I understand that, in the future, dep will use importers to understand the manifests of transitive dependencies to construct an appropriate dependency graph. That would be really useful when working with moby!

@carolynvs, thanks for your guidance in slack. I followed the pattern set by godep_importer.go and glide_importer.go.

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.

THIS LOOKS AWESOME! 🎉

Would you please also add an end-to-end test? I suggest copying and tweaking the godep test from here: https://github.com/golang/dep/tree/master/cmd/dep/testdata/harness_tests/init/godep/case1

To run just that single test you can use:

go test -run TestIntegration/init/vndr ./cmd/dep -v -logs

return nil, nil, errors.Wrapf(err, "unable to load vndr file")
}

manifest := &dep.Manifest{
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note to self: If #952 is merged first, we should use the constructor dep.NewManifest.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The race is on! 🏁

"github.com/pkg/errors"
)

func TestVndrConfig_Import(t *testing.T) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

My apologies for not giving a heads up that this was incoming before you started... Another PR was just merged which switched the importers to table based tests. Below is an example of how the tests should look now. I hope it's not too much trouble to update the tests here to match, let me know if they don't line up well, I see you are testing slightly differently (using deep equal).

func TestGodepConfig_Convert(t *testing.T) {
testCases := map[string]struct {
json godepJSON
wantConvertErr bool
matchPairedVersion bool
projectRoot gps.ProjectRoot
wantConstraint string
wantRevision gps.Revision
wantVersion string
wantLockCount int
}{
"convert project": {
json: godepJSON{
Imports: []godepPackage{
{
ImportPath: "github.com/sdboyer/deptest",
// This revision has 2 versions attached to it, v1.0.0 & v0.8.0.
Rev: "ff2948a2ac8f538c4ecd55962e919d1e13e74baf",
Comment: "v0.8.0",
},
},
},
matchPairedVersion: true,
projectRoot: gps.ProjectRoot("github.com/sdboyer/deptest"),
wantConstraint: "^0.8.0",
wantRevision: gps.Revision("ff2948a2ac8f538c4ecd55962e919d1e13e74baf"),
wantVersion: "v0.8.0",
wantLockCount: 1,
},

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem, this was pretty easy to do.

There's now a lot of repeated code between the three importers, though. I've got a sketch of how this could be resolved in spenczar/dep@vndr_importer...importer_test_refactor. It makes things a more abstract, which is bad, but it reduces the code duplication, which is good. If you think this is a net positive, let me know and I'll make an actual PR - up to you, I can see it either way.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@spenczar Oops! I missed this comment until just now. Want to take a look at #992? I think we both approached it about the same.

Source: pkg.repository,
},
}
pc.Constraint, err = v.sm.InferConstraint(pkg.revision, pc.Ident)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know that moby uses short git hashes, which is a bit of a problem. I think it would be easiest to have that be a separate issue, so I created #987 to address that part.

@AlekSi
Copy link
Contributor

AlekSi commented Aug 10, 2017

Poke @LK4D4

@carolynvs carolynvs merged commit 4f22124 into golang:master Aug 11, 2017
@carolynvs carolynvs mentioned this pull request Aug 11, 2017
carolynvs added a commit to carolynvs/dep that referenced this pull request Aug 11, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants