-
Notifications
You must be signed in to change notification settings - Fork 50
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
Clean up language around tag listing and repository naming #50
Clean up language around tag listing and repository naming #50
Conversation
LGTM |
proposals/distribution.md
Outdated
@@ -103,11 +103,11 @@ The following entries should be added to the [scope table][scope]: | |||
* “Specifying a distribution method”. | |||
This entry replaces part of the previous “Creating Reference spec for optional DNS based naming & distribution” and “Standardizing on a particular Distribution method” entries. | |||
|
|||
Retrieving image indexes covers the current “tag listing” (e.g. “what named manifests are in `library/busybox`?”), because tags are entries in the image format's [`manifests` array][manifests]. | |||
Other tag-listing endpoints needed for backwards-compatibility are therefore in scope as well. | |||
Tag-listing (e.g. “what named manifests are in `library/busybox`?”) endpoints are in scope and required for backwards compatible with clients. |
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.
Nit: s/compatible/compatibility/
61ef85a
to
147b507
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.
see comments
proposals/distribution.md
Outdated
|
||
Grouping image indexes in repositories is considered part of distribution policy or content management, which are out of scope for this entry's per-image action. | ||
For example, “what images are under `library/`?” is out of scope for this project. | ||
Grouping image repository names is considered part of distribution policy or content management, which are out of scope. |
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.
What do you think about "Managing the grouping of image repository names" ?
proposals/distribution.md
Outdated
Retrieving image indexes covers the current “tag listing” (e.g. “what named manifests are in `library/busybox`?”), because tags are entries in the image format's [`manifests` array][manifests]. | ||
Other tag-listing endpoints needed for backwards-compatibility are therefore in scope as well. | ||
Tag-listing (e.g. “what named manifests are in `library/busybox`?”) endpoints are in scope and required for backwards compatibility with clients. | ||
External methods for listing tags remain compatible through fetching by content address, but are out of scope for definition. |
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.
definition is a narrow term.. I think best to just say spec.. or remove. Or did you mean to indicate that a non-definition discussion of some type may come out of the spec regarding said external methods?
/s/definition/this spec/
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.
I can remove it, what I was trying to say is that external methods will not be defined within this specification. Rather the interface external methods would need is already in place.
147b507
to
0abcef8
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.
LGTM
Listing repositories (like [`/v2/_catalog`][catalog]) is a multi-[image-index][] action, which is out of scope for this entry. | ||
Managing groups of image indexes requires multi-[image-index][] actions, which are out of scope for this entry. | ||
Listing image indexes within a group is a multi-[image-index][] action, which is out of scope for this entry. | ||
* Description: Define a protocol for creating, retrieving, updating, and deleting content-addressable objects, such as those defined in the [image specification][image-spec]. |
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.
The distribution mandate needs to be broader than CAS. I don't think this sentence is broad enough to justify including the mutable, name-addressable tag-listing and tag -> manifest endpoints, which you list as in-scope above.
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 sentence is describing the protocol which is based on content-addressable objects. Naming just maps to these objects, the objects themselves are not mutable. It is clearly listed up above that these mapping endpoints are in scope and will be supported. The protocol however can be used without this tagging.
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 sentence is describing the protocol which is based on content-addressable objects.
The protocol uses content-addressable objects with a mutable, name-addressable layer on top (like Git's refs). Of the endpoints listed here, only /v2/<name>/blobs/<digest>
is strictly content addressable. The stuff under /v2/<name>/blobs/uploads/
is how you feed data into CAS, so that's fine too. But /v2/<name>/manifests/<reference>
and /v2/<name>/tags/list
are mutable and name-addressable, so you need some other wording to justify them.
Naming just maps to these objects, the objects themselves are not mutable.
So add a sentence that includes this naming in the scope? As long as any mentions of “naming” are clearly distinct from the out-of-scope “… DNS based naming…” entry.
It is clearly listed up above that these mapping endpoints are in scope and will be supported.
Right, but that text is just informative background that won't end up in the scope table. I think we want normative language that will end up in the scope table to explain why tag listing and name-addressable references are in scope.
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.
So add a sentence that includes this naming in the scope?
Can you propose a sentence to add and we can discuss. I don't want to go back and forth on the correctness of the description. I am not sure how the scope table is generated, sounds like the previous sentences are just there for the proposal but unrelated to the scope table at https://www.opencontainers.org/about/oci-scope-table
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.
Can you propose a sentence to add and we can discuss.
My previous attempt is what you're removing here ;). I was trying to lean on image-spec indexes and org.opencontainers.image.ref.name
to show why tag-listing and tag -> manifest-descriptor resolution were in scope. It sounds like that's the basic idea, but @stevvooe wants phrasing that doesn't mention the image-spec primitives, and I don't have a rewording I like better yet.
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.
I am not sure how the scope table is generated...
I think @caniszczyk just updates WordPress based on the instructions here after the TOB approves the proposal.
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.
I was trying to lean on image-spec indexes and org.opencontainers.image.ref.name to show why tag-listing and tag -> manifest-descriptor resolution were in scope.
I get that, the problem is that image-spec indexes and that annotation have nothing to do with the distribution specification, so it was confusing. The annotation may be the same as what the registry has, but there is no relationship in the API. The reason the current tag endpoints are included are for backwards compatibility and there are discussions which need to be had about updating that interface and capabilities. However the point of this proposal is to take the specification as is and keep the scope narrowed down. The CAS part of the specification has already been widely agreed on and is good to have in the scope table, we can always broaden later as the discussion matures.
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.
The reason the current tag endpoints are included are for backwards compatibility and there are discussions which need to be had about updating that interface and capabilities.
So add a sentence saying which endpoints are needed for backwards compat? And making it clear that backwards-compat with those endpoints is in-scope while backwards-compat with /v2/_catalog
is out-of-scope?
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 is why I was asking about the format of these entries, these sentences are already there. Are you saying we should just move the sentences from before the What
to under Description
? The previous version also had in/out of scope language in two places, it is really not clear what this section is trying to accomplish with the formatting.
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 is why I was asking about the format of these entries, these sentences are already there. Are you saying we should just move the sentences from before the
What
to underDescription
? The previous version also had in/out of scope language in two places, it is really not clear what this section is trying to accomplish with the formatting.
My intention with the formatting was:
* “{What entry}”.
{Detailed informative discussion, intended for the proposal only}.
* What: {What entry}
* In/Out/Future: {Scope entry}
* Status: {Status entry}
* Description: {Normative definiton}
* Why: {High-level informative motivation}
If you want to drop the detailed-informative-discussion section entirely because you find it not-very-informative, that's fine with me. For the “Description” entry, how about something like:
* Description: Define a protocol for creating, retrieving, updating, and deleting content-addressable objects, such as those defined in the [image specification][image-spec].
Also define, as an optional layer, the `/v2/<name>/tags/list` and `/v2/<name>/manifests/<reference>` endpoints needed for backwards compatibility with existing clients.
I'll be reopening the vote for the distribution spec after we have these last issues resolved on the proposal. Please take some time to review this PR so we can move forward. Thanks! |
proposals/distribution.md
Outdated
Retrieving image indexes covers the current “tag listing” (e.g. “what named manifests are in `library/busybox`?”), because tags are entries in the image format's [`manifests` array][manifests]. | ||
Other tag-listing endpoints needed for backwards-compatibility are therefore in scope as well. | ||
Tag-listing (e.g. “what named manifests are in `library/busybox`?”) endpoints are in scope and required for backwards compatibility with clients. | ||
External methods for listing tags remain compatible through fetching by content address, but are out of scope. |
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.
I don't understand this.
Through the context of this PR, I can guess that this refers to using an image index to approximate listing tags, but on its own this sentence would be totally meaningless to me. Can we just drop it?
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.
I can guess that this refers to using an image index to approximate listing tags
What do you mean by this? I was thinking an external method would completely replace the tag listing, rather than just approximate it. There is a gap in the existing API where manifests must be uploaded with a tag, but we can easily address that especially for cases where tagging is not used at all within a registry. Since something like garbage collection does not belong in this specification, having a requirement about the relationship between tags and content is not required, allowing tags to be completely external.
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.
What do you mean by this?
This PR drops Retrieving image indexes covers the current “tag listing”
and adds External methods for listing tags
, so I assumed you replaced the former with the latter to be more general?
I don't really see the need to mention this. It seems irrelevant, but I might just not understand what you're trying to say.
There is a gap in the existing API where manifests must be uploaded with a tag
Not per the spec. IIRC, docker doesn't allow you to push by a digest (maybe DockerHub too?), but I know at least GCR allows it.
Since something like garbage collection does not belong in this specification, having a requirement about the relationship between tags and content is not required, allowing tags to be completely external.
I don't see how tags can be completely external if tag listing is part of the spec. Are you saying tags are (mostly) undefined behavior in order to give freedom to implementors?
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.
I don't really see the need to mention this. It seems irrelevant, but I might just not understand what you're trying to say.
I was mostly just trying to account for the language which was here before, it really doesn't need to be mentioned. I will remove it, the scope section below is already clear that specification supports managing content addressable objects, what clients end up doing with that doesn't need to be mentioned here.
Not per the spec
Fair enough, the spec is clear that references can be digests, the incompatibility was mostly related to the deprecated schema 1 I believe. We will clean up these examples in the specification once it is in OCI.
I don't see how tags can be completely external if tag listing is part of the spec
I am only saying that a registry should be usable as only a CAS, without tagging at all. In which case any sort of resolution of a string -> content address is the responsibility of the distribution client. This does provide more freedom to distribution clients.
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.
just trying to account for the language which was here before
Makes sense! I appreciate your patience :)
I am only saying that a registry should be usable as only a CAS, without tagging at all.
Right, I totally agree. My worry is that a registry that only implements the CAS portions of the spec will be seen as non-conforming.
resolution of a string -> content address is the responsibility of the distribution client
I think this is my ideal. Tag listing and tag resolution seem to be more about discovery/naming than distribution, and registries that wish to be compatible with older clients could choose to implement these via the old tags list endpoint.
Regardless, I think this is a good starting point. I just hope tag listing being a requirement isn't a foregone conclusion.
@@ -103,19 +103,17 @@ The following entries should be added to the [scope table][scope]: | |||
* “Specifying a distribution method”. | |||
This entry replaces part of the previous “Creating Reference spec for optional DNS based naming & distribution” and “Standardizing on a particular Distribution method” entries. | |||
|
|||
Retrieving image indexes covers the current “tag listing” (e.g. “what named manifests are in `library/busybox`?”), because tags are entries in the image format's [`manifests` array][manifests]. | |||
Other tag-listing endpoints needed for backwards-compatibility are therefore in scope as well. | |||
Tag-listing (e.g. “what named manifests are in `library/busybox`?”) endpoints are in scope and required for backwards compatibility with clients. |
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.
I'm not totally convinced that tag listing should be included in distribution.
Does "backwards compatibility with clients" here refer to just docker pull -a
?
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.
I'm not totally convinced that tag listing should be included in distribution.
Specifically tag listing or tag operations in general?
Does "backwards compatibility with clients" here refer to just docker pull -a?
Also skopeo inspect
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.
I'd argue tags in general but I doubt that would get much traction :)
Reading through the distribution spec, it seems to me that tags are an implicitly-defined mutable layer on top of the CAS with some missing features (e.g. deleting a tag). It would be nice if tags were first class. Merging distribution/distribution#2169 would help with some of my concerns, but I'm not sure what the plan is for the PRs under Related GitHub Issues.
Happy to punt on this for now if it can be improved later, but the registry is currently a mix of a DAG/CAS/CMS and I'd like to scope down 'distribution' to be as narrow as we can get away with.
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.
I agree with everything you just said 😄
There is where we are with tags right now (what we need to include so clients can continue to work just using the OCI specification) and where we want to be with tagging, which I believe is an explicit tags endpoint like you listed. Ideally the CAS and tagging elements are distinct and the registry API can be used as CAS only. I am trying to communicate that here so we remain open to extension, backwards compatible, and still support just CAS with tagging elsewhere. I think maybe this isn't fully accomplishing that, other than dropping do you have any recommendations for making the point more clear?
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.
@jonjohnsonjr The intent of "Related Github Issues" was that these would be merged as part of the specification process. I am not sure at all how my comment about "These should be included" became "Related Github Issues".
In general, for the registry to be useful, you need CAS plus name pinning, so I don't think we can go full CAS here.
Tag listing should mention the endpoints and external method relying on content addressability. Image repositories should not be referred to as indexes, but rather as image repositories. Signed-off-by: Derek McGowan <derek@mcgstyle.net>
Signed-off-by: Stephen J Day <stephen.day@docker.com>
0abcef8
to
2b4308e
Compare
LGTM |
1 similar comment
LGTM |
Are we sure we wanted to land this with a CAS-only mandate? More on that in this sub-thread. And I'd understood @stevvooe to be on board with a broader mandate based on his:
|
This commit redefines the `_catalog` endpoint as an optional operation. Background on the issue: opencontainers#22 https://groups.google.com/a/opencontainers.org/forum/#!topic/dev/rJ72OtZuhbc opencontainers/tob#35 opencontainers/tob#46 opencontainers/tob#50 Signed-off-by: Atlas Kerr <atlaskerr@gmail.com>
This commit redefines the `_catalog` endpoint as an optional operation. Background on the issue: opencontainers#22 https://groups.google.com/a/opencontainers.org/forum/#!topic/dev/rJ72OtZuhbc opencontainers/tob#35 opencontainers/tob#46 opencontainers/tob#50 Signed-off-by: Atlas Kerr <atlaskerr@gmail.com>
Tag listing should mention the endpoints and external methods relying on content addressability.
Image repositories should not be referred to as indexes, but rather as image repositories.