Standardize the importers #1100

carolynvs · 2017-08-30T22:51:40Z

What does this do / why do we need it?

Each importer has slightly different behavior, and currently anyone who wants to contribute an importer needs to have some pretty significant dep knowledge (or I need to really be on my game during the code review!).

This makes it so that importers are only responsible for discovering and parsing config files. They then provide 3 pieces of information to a common function which handles the rest of the import logic: import path, text which possibly contains lock info, text which possibly contains constraint info. (Some tools know about ignores and are still responsible for adding those too).

The new base importer handles

Consolidating multiple imports for the same project, or sub-projects.
Figuring out what is in the lock and constraint hints, for example a branch, semver, tag, range constraint, revision or unusable text.
Adding constraints and locked projects to the manifest and lock.
Conditionally defaulting a constraint based on the lock information. This is something that some importers need (because they only track revisions). Also supporting external config during dep ensure, will require turning off the "constraint guessing".

I have also completely redone the tests against a single repository that contains all the testdata necessary for the import scenarios (github.com/carolynvs/deptest-importers). The tests now run in parallel too! 🎉

Apologies for the big PR! Unfortunately, we've gotten to where we are now thanks to piecemeal, incremental implementations and I want to make sure this works as a whole before anything lands in master.

What should your reviewer look out for in this PR?

I'd like extra eyes on the import rules (see the inline doc on baseImporter.importProjects for the rules).

Do you need help or clarification on anything?

Nope

Which issue(s) does this PR fix?

Fixes #939

Notes:

When this is complete, I will follow up with the pending importer PRs: add Gomfile importer #746, Add importer for govendor #815, and gb project importer #818.
I think baseImporter.importProjects is ripe for concurrency. I didn't want to tackle that on top of everything else though, as there were enough big changes in that same area in this PR. But it's on my radar after this PR merges and looks stable.
After this, I'm going to tackle the importer harness tests.

darkowlzz · 2017-09-01T13:32:22Z

cmd/dep/base_importer.go

+	}
+}
+
+// isVersion determines if the specified value is a tag (plain or semver).


darkowlzz · 2017-09-01T13:53:19Z

cmd/dep/base_importer.go

+	return orderedProjects, nil
+}
+
+// importProject loads imported projects into the manifest and lock.


importPackages loads imported packages ?

The importers are now only responsible for reading config files and feeding unvalidated lock and constraint hints into importPackages.

* Don’t throw away a locked version match due to a conflicting constraint unless there is another match that _does_ match * Ignore pinned constraints * Ignore conflicting constraints

github.com/carolynvs/deptest-importers contains the entire matrix of testdata needed by the importers

* The setup and validation for all the importer testcases are the same, they just need to handle calling the importer properly. * Flag importer tests to run in parallel now that they've been cleaned up.

carolynvs · 2017-09-05T15:46:30Z

This is finally ready for review! 🎉

I would really love to get this in by the end of the week. So if I don't hear anyone screaming at me to not merge by then.... 😇

jmank88 · 2017-09-05T16:22:06Z

cmd/dep/base_importer.go

+		case gps.IsRevision:
+			return true
+		case gps.IsVersion:
+			return true


nit:

case gps.IsRevision, gps.IsVersion: return true

Thanks! I knew there was a better way of doing that! ❤️

jmank88 · 2017-09-05T16:24:59Z

cmd/dep/base_importer.go

+	return false
+}
+
+func (i *baseImporter) testConstraint(c gps.Constraint, v gps.Version) bool {


carolynvs · 2017-09-05T18:16:34Z

I can't tell if the new bolt tests are flaky or if I managed to break them somehow?
https://travis-ci.org/golang/dep/jobs/272136148#L476-L480

Signal received: waiting for 1 ops to complete...

--- FAIL: TestBoltCacheTimeout (0.82s)
	source_cache_bolt_test.go:170: expected no cached versions, but got:
			[]gps.PairedVersion{gps.versionPair{v:gps.branchVersion{name:"originalbranch", isDefault:false}, r:"rev1"}, gps.versionPair{v:"originalver", r:"rev2"}}
FAIL

jmank88 · 2017-09-05T21:25:01Z

I can't tell if the new bolt tests are flaky or if I managed to break them somehow?

Seems flaky, I'm working on reproducing it. I'm hoping it's just some clock funkiness. I restarted the failed job.

The closure created by t.Run wasn't copying the testcase value, so the test was being run on the wrong values.

When a lock hint is empty, nothing should be added to the lock

carolynvs · 2017-09-06T16:21:16Z

Turns out that #1130 was already fixed by this PR. Go me. 😉 So I just added a test to verify the correct behavior.

I also realized that my parallel tests weren't working as I had thought. I'm now making local copies of the testcase variables so that they are properly included in the subtest closure.

carolynvs requested a review from sdboyer as a code owner August 30, 2017 22:51

googlebot added the cla: yes label Aug 30, 2017

carolynvs requested a review from darkowlzz August 30, 2017 22:52

carolynvs force-pushed the standarized-importer-pattern branch 2 times, most recently from 834e8e6 to 331c6fb Compare August 30, 2017 23:39

darkowlzz reviewed Sep 1, 2017

View reviewed changes

chriswhelix mentioned this pull request Sep 1, 2017

[WIP] Import transitive constraints from glide #920

Closed

5 tasks

carolynvs added 5 commits September 5, 2017 09:20

cmd/dep: Standardize import logic for importers

f6e75af

The importers are now only responsible for reading config files and feeding unvalidated lock and constraint hints into importPackages.

cmd/dep: Support new importer rules

b3dff94

* Don’t throw away a locked version match due to a conflicting constraint unless there is another match that _does_ match * Ignore pinned constraints * Ignore conflicting constraints

cmd/dep: Switch to special importer testdata set

42891a6

github.com/carolynvs/deptest-importers contains the entire matrix of testdata needed by the importers

cmd/dep: Move import test execution into testcase

2d44886

* The setup and validation for all the importer testcases are the same, they just need to handle calling the importer properly. * Flag importer tests to run in parallel now that they've been cleaned up.

cmd/dep: Validate warnings in import testcases

1407196

carolynvs force-pushed the standarized-importer-pattern branch from fc84b3e to 1407196 Compare September 5, 2017 14:28

carolynvs changed the title ~~[WIP]Standardize the importers~~ Standardize the importers Sep 5, 2017

jmank88 reviewed Sep 5, 2017

View reviewed changes

carolynvs added 2 commits September 5, 2017 11:39

cmd/dep: Document every baseImporter function

6f6b195

cmd/dep: Simplify pinned constraint check

58cbb3f

carolynvs mentioned this pull request Sep 6, 2017

dep: Panic observed on init #1130

Closed

carolynvs added 2 commits September 5, 2017 22:31

cmd/dep: fix parallel tests

9356f05

The closure created by t.Run wasn't copying the testcase value, so the test was being run on the wrong values.

cmd/dep: verify empty locks are skipped

0f33a59

When a lock hint is empty, nothing should be added to the lock

jmank88 approved these changes Sep 9, 2017

View reviewed changes

jmank88 merged commit 49910b7 into golang:master Sep 9, 2017

This was referenced Sep 10, 2017

gvt (and gb-vendor) importer #1149

Merged

"dep init" failes to import glide with an importpath to a remote subpackage #861

Closed

carolynvs mentioned this pull request Sep 26, 2017

dep init fails on bitbucket.org/ww/goautoneg fabulous-gopher/k8s-dep-e2e#1

Closed

This was referenced Sep 26, 2017

dep picked a different revision for cloud.google.com/go fabulous-gopher/k8s-dep-e2e#5

Closed

dep init fails on github.com/kr/pty fabulous-gopher/k8s-dep-e2e#2

Closed

carolynvs deleted the standarized-importer-pattern branch October 6, 2017 15:43

carolynvs mentioned this pull request Oct 6, 2017

Add importer for govendor #815

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Standardize the importers #1100

Standardize the importers #1100

carolynvs commented Aug 30, 2017 •

edited

Loading

darkowlzz Sep 1, 2017

darkowlzz Sep 1, 2017

carolynvs commented Sep 5, 2017

jmank88 Sep 5, 2017

carolynvs Sep 5, 2017

jmank88 Sep 5, 2017

carolynvs commented Sep 5, 2017

jmank88 commented Sep 5, 2017 •

edited

Loading

carolynvs commented Sep 6, 2017

Standardize the importers #1100

Standardize the importers #1100

Conversation

carolynvs commented Aug 30, 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?

darkowlzz Sep 1, 2017

Choose a reason for hiding this comment

darkowlzz Sep 1, 2017

Choose a reason for hiding this comment

carolynvs commented Sep 5, 2017

jmank88 Sep 5, 2017

Choose a reason for hiding this comment

carolynvs Sep 5, 2017

Choose a reason for hiding this comment

jmank88 Sep 5, 2017

Choose a reason for hiding this comment

carolynvs commented Sep 5, 2017

jmank88 commented Sep 5, 2017 • edited Loading

carolynvs commented Sep 6, 2017

carolynvs commented Aug 30, 2017 •

edited

Loading

jmank88 commented Sep 5, 2017 •

edited

Loading