Registry support #1172

romangurevitch · 2017-09-14T13:30:38Z

What does this do / why do we need it?

A go dep registry will host packaged versions of go "projects" in self-contained format.
Developers will be able to publish new project versions to the registry and resolve them back from the registry as project dependencies.

The primary use case for a registry is to support development isolation against an in-house repository that serves immutable project dependencies. These dependencies can be external dependencies or internal projects used in-house as 3rd party dependencies.
The registry will support authentication via user tokens.

This pull request adds the functionality of downloading dependencies from a registry.
We will soon submit an additional pull request for publishing projects to a registry.

googlebot · 2017-09-14T13:30:41Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

romangurevitch · 2017-09-14T14:10:28Z

I signed it!

googlebot · 2017-09-14T14:10:30Z

CLAs look good, thanks!

romangurevitch · 2017-09-14T16:33:27Z

The build is failing because we added a new dependency but as a result when running dep ensure there are ~900 modified/new files in vendor folder.
I didn't commit those files so the PR would be easier to read.
Please let me know what is the correct approach according to your guidelines.

Thank you, looking forward for your feedback.

sdboyer · 2017-09-14T17:53:36Z

yep! larger review is in progress.

please commit everything into vendor, then run dep prune.

romangurevitch · 2017-09-14T19:43:05Z

I have added the vendor dependencies and the build passes now as well as all the tests.

sdboyer

ok, there's a lot in here, and it's awesome to see the fruits of all the discussions we've had.

at the same time, we've got a ways to go, as there are some critical problems with the approach.

sdboyer · 2017-09-14T14:33:12Z

internal/gps/registry.go

+	"io"
+	"os"
+	"compress/gzip"
+	"github.com/golang/dep/internal/fs"


nit: gofmt please, it'll sort the imports.

sdboyer · 2017-09-14T14:33:51Z

internal/gps/registry.go

+
+var errNotFound = errors.New(http.StatusText(http.StatusNotFound))
+
+type Registry interface {


Need docs on exported types

sdboyer · 2017-09-14T14:36:54Z

internal/gps/registry.go

+}
+
+func (s *registrySource) listPackages(ctx context.Context, pr ProjectRoot, r Revision) (ptree pkgtree.PackageTree, err error) {
+	resp, err := s.execDownloadDependency(pr, r)


downloading on the fly may be OK as a first step, but eventually we're going to want to keep these downloaded tarballs locally.

sdboyer · 2017-09-14T14:38:34Z

internal/gps/registry.go

+	return &versionsResp, err
+}
+
+func (s *registrySource) execDownloadDependency(pr ProjectRoot, r Revision) (*http.Response, error) {


context.Context needs to be plumbed through this function in order to facilitate cancellation.

sdboyer · 2017-09-14T14:40:45Z

internal/test/test.go

@@ -21,6 +21,7 @@ import (
 	"testing"

 	"github.com/pkg/errors"
+	"github.com/golang/dep/cmd/dep/testdata/registry"


testdata dirs are for data, not for source code. this package needs to be relocated - i'll have thoughts about the right place to put it once i grok the pieces a bit better

sdboyer · 2017-09-21T02:51:30Z

cmd/dep/login.go

@@ -0,0 +1,150 @@
+package main


we need to hide this from normal builds until we're actually ready to ship the registry. feature-flag style, dep login needs to not even be recognized as a command until we flip the switch.

maybe we do this with build tags, or maybe something else, but we can't put new commands in front of our users until it's mostly baked.

sdboyer · 2017-09-21T02:53:09Z

cmd/dep/publish.go

@@ -0,0 +1,308 @@
+package main
+
+import (


gofmt, or better, goimports

sdboyer · 2017-09-21T02:53:27Z

cmd/dep/publish.go

@@ -0,0 +1,308 @@
+package main


as with login.go, this needs to be unreachable

sdboyer · 2017-09-21T02:55:47Z

cmd/dep/testdata/harness_tests/init/registry/case1/final/Gopkg.lock

@@ -0,0 +1,21 @@
+# This file is autogenerated, do not edit; changes may be undone by the next 'dep ensure'.


"case1"

where possible, it's preferred that the name of the test fixture be descriptive, rather than just incrementing a number.

sdboyer · 2017-09-21T03:06:02Z

internal/gps/deduce.go

@@ -588,6 +590,8 @@ func (dc *deductionCoordinator) deduceRootPath(ctx context.Context, path string)
 		switch d := data.(type) {
 		case maybeSource:
 			return pathDeduction{root: prefix, mb: d}, nil


this is probably a much better place to think about hooking in: instead of messing with the deduction process itself, which is really not where the registry belongs (per other comments), the thing to be taking over is the maybeSources returned in the pathDeductions. create a new type (the maybeRegistrySource in the current iteration of the PR isn't structured for this at all) that takes other maybeSources, and figures out how to transparently wrap them.

side note with that - it'll be very important that the path on disk used by registry-wrapped ones are entirely different from what would've been used by the underlying wrapped instance.

sdboyer · 2017-09-21T03:15:35Z

also, a more general note - seeing the results of opting for publishing into the registry, rather than transparently proxying it in, has really left a poor taste in my mouth. Yoav's point on the spec doc about this was that manual publishing of versions presents a much, much lower barrier of entry for the registry implementor than does proxying, which i understand, but the idea that we need to add a publish command to dep itself in order to have registries functioning in even a basic way feels like a non-starter to me.

i need to ponder on this more, but my feeling is that transparent proxying needn't be nearly so hard for us because the SourceManager already provides so many facilities for doing just that (even though we'll need to move gps out of internal for that to be helpful). it also helps stave off even the possibility of "local" vs. "upstream" releases/versions, and having to differentiate between the two, which still just feels to me like a nightmare.

yoav · 2017-09-22T22:10:10Z

@sdboyer with regards to publish - this functionality can be left out, and indeed dep already has a lot of the logic to proxy-in dependencies via SourceManager, so as long as a registry supports transparent proxying dep can stay clean of publishing.

My main concerns were:

Applying the same logic server-side is typically more complicated, being state-aware, especially when having to deal with cleaning bad state (real world problems).
It shortens the path to allow straight-forward dep registries, such as https://github.com/deejross/dep-registry, to be fully functional from the get-go.
This PR didn't seem like a good fit for making the code in gps public ;)
The concept of publishing your own packages exists in many package ecosystems without introducing ambiguity (npm, crates, etc.), and is needed when your registry does not have access to the vcs (for example, projects within the same organization sharing only specific published versions without opening the whole source tree).

In any case, since this impacts the registry implementation we would need to provide clarity :)

sdboyer · 2017-10-07T01:47:35Z

i think we should set a time for another face-to-face about this - maybe late next week? - so that we can review and regroup. that way i can also set aside time to re-review this and the spec doc, as my mind currently feels like it's been sandblasted by the last couple weeks and i can scarcely remember any details here 😢

but...!

This PR didn't seem like a good fit for making the code in gps public ;)

yeah, i think maybe i just need to up and do this. it's not hard - gomvpkg seems to have 💯 worked when i ran it locally just now - and we're protected by the pre-1.0.0 rules, so it's fine for us to continue in our API-breaking ways for a while yet.

Applying the same logic server-side is typically more complicated, being state-aware, especially when having to deal with cleaning bad state (real world problems).

no question. for example, i am utterly unconfident that today's SourceManager would do well as a reliable backend to a long-lived server - there are some possible hacks i noted in #659, but they are haaaaaaaacky hacky hacks, and in no way guaranteed to work.

The concept of publishing your own packages exists in many package ecosystems without introducing ambiguity (npm, crates, etc.), and is needed when your registry does not have access to the vcs (for example, projects within the same organization sharing only specific published versions without opening the whole source tree).

for sure - i've got no qualms about the importance of being able to publish projects. but it's that key difference between publishing projects to a canonical location (and thereby, import path), vs. publishing to fill an intermediate proxy where the essential purpose is to enforce policy. really, i think it goes all the way back up to the word meanings for me - i don't think "publish" is even the right verb to use there. yes, both are the action of pushing a version into a registry, and functionally are probably indistinguishable, and this works fine for other communities. but context is everything: if we accept the idea that there are canonical locations for projects, then "publishing" seems misleading - "mirroring" isn't quite right easier, but the idea is closer. the words we choose significantly impact users' understanding, and if we can find ones that tend to put peoples' expectations in the right ballpark, IMO it's a big win.

yoav · 2017-10-08T21:34:50Z

i think we should set a time for another face-to-face about this - maybe late next week?

That'd probably be the best thing to do. I'll ping you in slack to set something up.

my mind currently feels like it's been sandblasted by the last couple weeks

yep. I know just the feeling 😜

romangurevitch · 2017-11-16T09:54:46Z

I'm closing this pull request and soon will submit a new pull request to avoid merging issues and add your requests.

Registry support - download

24fd8e5

romangurevitch requested review from carolynvs, ibrasho and sdboyer as code owners September 14, 2017 13:30

googlebot added the cla: no label Sep 14, 2017

googlebot added cla: yes and removed cla: no labels Sep 14, 2017

Registry support - download

df985d1

Registry support - no-registry flag for ensure cmd

1afea6b

romangurevitch requested a review from darkowlzz as a code owner September 17, 2017 11:35

Registry support - publish

33d05a1

romangurevitch changed the title ~~Registry support - download~~ Registry support Sep 17, 2017

Registry support - publish

9d4e869

sdboyer suggested changes Sep 21, 2017

View reviewed changes

sdboyer mentioned this pull request Sep 25, 2017

Integrating with Linux package management systems #1214

Closed

romangurevitch added 2 commits October 2, 2017 11:22

Registry support - gofmt and other review fixes

7926fd5

Registry support - gofmt and other review fixes

a79554a

carolynvs removed their request for review October 24, 2017 19:19

Registry support - few comment fixes

b2ccf08

romangurevitch closed this Nov 16, 2017

romangurevitch mentioned this pull request Nov 16, 2017

Registry support #1381

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Registry support #1172

Registry support #1172

romangurevitch commented Sep 14, 2017

googlebot commented Sep 14, 2017

romangurevitch commented Sep 14, 2017

googlebot commented Sep 14, 2017

romangurevitch commented Sep 14, 2017 •

edited

Loading

sdboyer commented Sep 14, 2017

romangurevitch commented Sep 14, 2017

sdboyer left a comment

sdboyer Sep 14, 2017

sdboyer Sep 14, 2017

sdboyer Sep 14, 2017

sdboyer Sep 14, 2017

sdboyer Sep 14, 2017

sdboyer Sep 21, 2017

sdboyer Sep 21, 2017

romangurevitch Oct 8, 2017

sdboyer Sep 21, 2017

sdboyer Sep 21, 2017

sdboyer Sep 21, 2017

sdboyer commented Sep 21, 2017

yoav commented Sep 22, 2017

sdboyer commented Oct 7, 2017

yoav commented Oct 8, 2017

romangurevitch commented Nov 16, 2017


		var errNotFound = errors.New(http.StatusText(http.StatusNotFound))

		type Registry interface {

		@@ -0,0 +1,21 @@
		# This file is autogenerated, do not edit; changes may be undone by the next 'dep ensure'.

Registry support #1172

Registry support #1172

Conversation

romangurevitch commented Sep 14, 2017

What does this do / why do we need it?

googlebot commented Sep 14, 2017

romangurevitch commented Sep 14, 2017

googlebot commented Sep 14, 2017

romangurevitch commented Sep 14, 2017 • edited Loading

sdboyer commented Sep 14, 2017

romangurevitch commented Sep 14, 2017

sdboyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdboyer commented Sep 21, 2017

yoav commented Sep 22, 2017

sdboyer commented Oct 7, 2017

yoav commented Oct 8, 2017

romangurevitch commented Nov 16, 2017

romangurevitch commented Sep 14, 2017 •

edited

Loading