-
Notifications
You must be signed in to change notification settings - Fork 2.7k
WIP: Namespace package #344
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
241c413 to
a16f5cd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Think we could move this main() to the top, so it's the first thing after imports?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed, should be on top
a16f5cd to
780a659
Compare
|
what is lacking to get this merged? |
namespace/namespace.go
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of passing in a base set of Entries and a Discoverer, a Discoverer should be able to be composed of multiple discoverers. The base entries should be able to be converted to a discoverer from a static set of entries and composed with any other methods.
|
There are still a few more changes I want to make before removing the WIP flag
|
7aa8d6f to
b3b1f5b
Compare
|
Made the updates from my previous comment. I need to do one more pass at code cleanup and unit tests then I will remove the WIP tag. I am happy with the current interface although if someone can offer a better name for Entry I would be grateful. I also still think |
b3b1f5b to
9abfd7a
Compare
4fda71c to
59c74f8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This struct only exists to ensure the underlying entry array is sorted. This keeps inserts and lookups fast. We could get rid of this struct and use functions which mutate and return the Entry slice, but we will lose that guarantee. This entries slice is never actually sorted since inserts are in order.
d1174c3 to
97fda35
Compare
Signed-off-by: Stephen J Day <stephen.day@docker.com>
Move namespace logic into separate package. Reduce namespace interface and make use of it from command. Signed-off-by: Derek McGowan <derek@mcgstyle.net> (github: dmcgowan)
Contains logic was not taking into account the end boundary of the last scope component, allowing for bad matches. Signed-off-by: Derek McGowan <derek@mcgstyle.net> (github: dmcgowan)
97fda35 to
0083e99
Compare
|
@stevvooe I think I agree that from the namespace resolver perspective, entries should have a scope, action, and arguments. The only entry which would attempt to be resolved is the "namespace" action and only by a particular resolver such as the MultiResolver or a RecursiveResolver. This is relevant to the http resolver contribution from @minimnar |
|
@miminar I took a peak at the suggested changes. I'm confused as to why you'd add a block command when you can capture and blackhole the namespace: In the above example, "some.company/" is has no configuration so it cannot be pushed or pulled. Following from that, I find blocking the entire "endpoint" to be odd when you can simply not include it in the configuration. These details may not be clear in the specification. While the confirmation feature seems like a dubious feature, it would probably be better expressed as a modifier for namespace directive: Albeit, it seems odd to express UI behavior at this level. It might be better to have the concept of namespace-less targets, which could be used to push/pull (note the lack of a namespace declaration): Then, such and endpoint would require an explicit call out for push: |
|
@stevvooe You're right about the blocking feature. It wasn't clear enough to me while I wrote those notes. I like the idea of namespace-less targets. I'll try to come up with more elaborated proposal based on it later. |
|
@miminar Sounds good. We probably need to make these use cases clear in the namespace package docs. |
|
We are reevaluating the approach here. Closing this for now. |
Creates a namespace package and namespace tool. The interface to the namespace package is primarily focused on managing sets of namespace entries which can be used to resolve a name into entries and initialize a registry client.