Skip to content
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

clarification about nested index #716

Merged
merged 1 commit into from
Apr 11, 2018

Conversation

AkihiroSuda
Copy link
Member

@AkihiroSuda AkihiroSuda commented Jul 28, 2017

Signed-off-by: Akihiro Suda [email protected]

@jonjohnsonjr
Copy link
Contributor

What is meant by required? For whom?

There is pretty specific language here:
https://github.com/opencontainers/image-spec/blob/master/image-index.md

I don't see any reason to forbid an image index referencing another image index, but I'm curious what the use case would be.

@AkihiroSuda
Copy link
Member Author

@jonjohnsonjr
Copy link
Contributor

Ah, thanks! My apologies, please ignore me :)

I guess I don't quite understand what is meant by "image providers" here:

While the use of an image index is OPTIONAL for image providers, image consumers SHOULD be prepared to process them.

Does that just refer to distribution in general? (I don't mean to hijack your PR but it seems like the <<optional>> bit traces back to this sentence, so it might be worth clarifying/removing as part of this PR?)

@jonboulle
Copy link
Contributor

jonboulle commented Jul 31, 2017

I also don't understand the use case for an image index to reference another image index. Notably this is not called out here in the actual index specification: https://github.com/opencontainers/image-spec/blame/master/image-index.md#L35

https://github.com/opencontainers/image-spec/blame/9e2ed30b411505b510b4ab080c4035d71bd56b44/image-layout.md#L148

I'm wondering if this line snuck in accidentally as the original context is very different:
9bb56d8#diff-99bfcb610f238b942638d49d1ff2eaf4R46

Here it refers to blobs in the ref directory, not the descriptors in the index file.

@AkihiroSuda
Copy link
Member Author

@jonboulle

Yes, I'm +1 for removing nested index if it was introduced by accident.
WDYT about necessity of index?

@jonboulle
Copy link
Contributor

I don't see a need for it but could be proven wrong.

@AkihiroSuda
Copy link
Member Author

opened #718 and #719

@AkihiroSuda AkihiroSuda closed this Aug 2, 2017
@AkihiroSuda AkihiroSuda reopened this Aug 3, 2017
@AkihiroSuda AkihiroSuda changed the title dot: index is required and can be referred from another index clarification about index Aug 3, 2017
@AkihiroSuda
Copy link
Member Author

#718 and #719 were rejected.

Let me reopen this PR with updated docs.

@AkihiroSuda
Copy link
Member Author

image-index.md Outdated
@@ -2,6 +2,7 @@

The image index is a higher-level manifest which points to specific [image manifests](manifest.md), ideal for one or more platforms.
While the use of an image index is OPTIONAL for image providers, image consumers SHOULD be prepared to process them.
Also, the top-level index (`index.json`) is a REQUIRED file in [the image layout](image-layout.md).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather not have this line. This file/section is about defining the application/vnd.oci.image.index.v1+json media type, and should not be about where that media type is used.

I'd also encourage dropping the previous line for the same reason. Instead, requirements about what providers may/should/must supply and what consumers may/should/must consume should be handled in the referencing location. For example, you can see that application/vnd.oci.image.index.v1+json consumers MUST handle application/vnd.oci.image.manifest.v1+json in the context of manifests. And you'd see that consumers of image-layout v1.0.0 MUST handle application/vnd.oci.image.index.v1+json in the context of index.json around here (what I think you should be adding in this PR). And you'd see that consumers need not handle v1.0.0 image layouts here (which I think you should also be adding in this PR).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this doesn't belong in this file.

image-layout.md Outdated
@@ -149,6 +149,7 @@ This index provides an established path (`/index.json`) to have an entry point f
* Future versions of the spec MAY use a different mediatype (i.e. a new versioned format).
* An encountered `mediaType` that is unknown SHOULD be safely ignored.

Note that while `index.json` is a REQUIRED file, the use of an image index is OPTIONAL for image providers.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd shift this line up to the opening section and rephrase it to something like:

Consumers MAY support OCI Image Layout and MAY also support other mechanisms for distributing image content.

The optionality of image-layout support is important enough that I'd rather not bury it 150 lines into the file ;).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already state this in a clear manner in another doc.

image-layout.md Outdated

The first named reference (`"stable-release"`) points to another index that might contain multiple references with several platform specification and annotations.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Backticks and quotes seems like overkill. Can we use stable-release here (and v1.0 below)? Those are the names; the quotes are just part of representing strings in JSON.

Also probably “several platform sepecification” → “distinct platforms” or some such.

image-layout.md Outdated

The first named reference (`"stable-release"`) points to another index that might contain multiple references with several platform specification and annotations.
Note that the `org.opencontainers.image.ref.name` annotation SHOULD only be considered valid when on descriptors on `index.json`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe link to annotations.md for this so folks can see that we're referencing a requirement defined there.

@stevvooe
Copy link
Contributor

stevvooe commented Aug 4, 2017

@AkihiroSuda It would be good if you could file an issue stating the problem before filing PRs. I am not sure that these are correct and they seem redundant with other statements that are already present.

What is the problem that you are trying to address here?

@AkihiroSuda AkihiroSuda changed the title clarification about index clarification about nested index Aug 4, 2017
@AkihiroSuda
Copy link
Member Author

AkihiroSuda commented Aug 4, 2017

Opened #720 for the problem: The fact that "Image Layout is OPTIONAL" is not explicitly documented and causing confusion about necessity of index.json.

I modified this PR to limit its scope to nested index examples. PTAL.


The first named reference (`stable-release`) points to another index that might contain multiple references with distinct platforms and annotations.
Note that the [`org.opencontainers.image.ref.name` annotation](annotations.md) SHOULD only be considered valid when on descriptors on `index.json`.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: This redundant sentence is intentionally added for better human understandability.

image-layout.md Outdated
The first named reference (`stable-release`) points to another index that might contain multiple references with distinct platforms and annotations.
Note that the [`org.opencontainers.image.ref.name` annotation](annotations.md) SHOULD only be considered valid when on descriptors on `index.json`.

The second named reference ("`v1.0`") points to a manifest that is specific to the linux/ppc64le platform.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You still have both backticks and quotes here.

@AkihiroSuda
Copy link
Member Author

Fixed quotation

@@ -6,6 +6,7 @@ digraph G {
layer [shape=note, label="Layer tar archive\napplication/vnd.oci.image.layer.v1.tar\napplication/vnd.oci.image.layer.v1.tar+gzip\napplication/vnd.oci.image.layer.nondistributable.v1.tar\napplication/vnd.oci.image.layer.nondistributable.v1.tar+gzip"]
}

imageIndex -> imageIndex [label="1..*"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're going to draw this loop in here, I think we also want to SHOULD support for indexes here (for both providers and consumers), at least for the index in index.json.

@AkihiroSuda
Copy link
Member Author

Updated, anyone PTAL?

@AkihiroSuda
Copy link
Member Author

lint failure on travis is unrelated: #735

@stevvooe
Copy link
Contributor

stevvooe commented Nov 21, 2017

LGTM

The main use case I see is an archive that has multiple multi-platform images.

Approved with PullApprove

@AkihiroSuda
Copy link
Member Author

ping @opencontainers/image-spec-maintainers

@cyphar
Copy link
Member

cyphar commented Mar 11, 2018

LGTM.

/cc @opencontainers/image-spec-maintainers

Approved with PullApprove

@vbatts
Copy link
Member

vbatts commented Apr 11, 2018

LGTM

Approved with PullApprove

@vbatts vbatts merged commit e562b04 into opencontainers:master Apr 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants