Registry support #1381
Registry support #1381
Conversation
romangurevitch
requested review from
carolynvs,
ibrasho and
sdboyer
as code owners
|
This PR is based on #1172 with review fixes. |
gps/registry.go
Outdated
| return nil, err | ||
| } | ||
|
|
||
| req.Header.Set("Authorization", "BEARER "+s.token) |
There was a problem hiding this comment.
It's usually spelled "Bearer" with only the B capitalized.
cmd/dep/main.go
Outdated
| @@ -19,6 +19,8 @@ import ( | |||
|
|
|||
| "github.com/golang/dep" | |||
| "github.com/golang/dep/internal/fs" | |||
| "github.com/golang/dep/gps" | |||
| "net/url" | |||
There was a problem hiding this comment.
please run gofmt or goimports to keep imports sorted
There was a problem hiding this comment.
huh, interesting, i wasn't even looking at the ""github.com/golang/dep/internal/fs" import - i was looking at "net/url". that should be in the first block of stdlib imports.
cmd/dep/main.go
Outdated
| registryURL := getEnv(c.Env, "DEPREGISTRYURL") | ||
| token := getEnv(c.Env, "DEPREGISTRYTOKEN") | ||
|
|
||
| // check if url is not define do not create registry |
There was a problem hiding this comment.
Complete sentences for inline comments, please
| dc.mut.Unlock() | ||
|
|
||
| // Trigger the HTTP-backed deduction process for this requestor. | ||
| return rd.deduce(ctx, path) |
There was a problem hiding this comment.
This entails at least one HTTP request for each root path, including the known path case (which was removed above, compared with deductionCoordinator.deduceRootPath()). This will be a notable performance hit for the user vs. the current behavior.
More importantly, though, as long as we are guaranteeing that any proxy registry can access and passthrough-cache any upstream VCS source that a dep client would be able to access itself without an intermediate registry, then, then i see no compelling use case for having this extra request. In fact, all i see is a vector for confusion and errors, as we become reliant on registries to respect naming invariants that are presently universal, by virtue of being encoded in the tool. It seems like it should be possible - at least as a first pass approach - to just rely on the existing static deduction subsystem, then perform some structured transformations on the pathDeductions that they return.
Is there a crucial use case that's serviced by making this request to the registry?
There was a problem hiding this comment.
Thanks. Yes, it would be preferable to deduct the root project path without HTTP request. I added the known paths deduction to address this issue.
However, for imports that are not using well known URLs, passthrough upstream VCS source is not always possible. For instance, in a closed network environment that work exclusively through a private registry deducing the project root by querying the VCS repository for the goget metadata cannot work (leaving aside the fact that most of the benefits of having a local private registry are lost by bypassing and accessing the sources directly).
Therefore, there is a valid use case IMO for having the registry know about the project roots it hosts. Once, we allowed static deduction to kick in first I don't think the performance impact (a single HEAD call) would be an issue.
There was a problem hiding this comment.
Yes, it would be preferable to deduct the root project path without HTTP request.
great 😄
However, for imports that are not using well known URLs,
for sure, i'm entirely in agreement that anything requiring normal go-get HTTP metadata deduction can be forwarded to the registry.
Once, we allowed static deduction to kick in first I don't think the performance impact (a single HEAD call) would be an issue.
agreed, that's no net increase in the number of HTTP requests, so no harm done.
There was a problem hiding this comment.
Great. Is it ready to be merged?
|
no. this is just a preliminary review.
…On January 4, 2018 5:56:58 AM EST, Roman Gurevitch ***@***.***> wrote:
romangurevitch commented on this pull request.
> + // access to the rootxt map.
+ returnFunc: func(pd pathDeduction) {
+ dc.mut.Lock()
+ dc.rootxt.Insert(pd.root, pd.mb)
+ dc.mut.Unlock()
+ },
+ }
+
+ // Save the rd in the rootxt so that calls checking on similar
+ // paths made while the request is in flight can be folded together.
+ dc.mut.Lock()
+ dc.rootxt.Insert(path, rd)
+ dc.mut.Unlock()
+
+ // Trigger the HTTP-backed deduction process for this requestor.
+ return rd.deduce(ctx, path)
Great. Is it ready to be merged?
--
You are receiving this because your review was requested.
Reply to this email directly or view it on GitHub:
#1381 (comment)
|
|
Out of curiosity what were you using as a registry? |
|
Dep was officially deprecated earlier this year, and the proposal to archive this repository was accepted. As such, I'm closing outstanding issues before archiving the repository. For any further comments, please use the proposal thread on the Go issue tracker. Thanks! |
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.
Set the following environment variables to use registry:
DEPREGISTRYURL
DEPREGISTRYTOKEN