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

*: manifest-list -> image-index #546

Merged
merged 1 commit into from
Feb 16, 2017

Conversation

vbatts
Copy link
Member

@vbatts vbatts commented Feb 1, 2017

per discussion around #533

Signed-off-by: Vincent Batts vbatts@hashbangbash.com

@stevvooe
Copy link
Contributor

stevvooe commented Feb 2, 2017

LGTM

👍 👍 👍 👍 👍 👍 👍 👍

Approved with PullApprove

image-layout.md Outdated
@@ -5,7 +5,7 @@ This layout MAY be used in a variety of different transport mechanisms: archive

Given an image layout and a ref, a tool can create an [OCI Runtime Specification bundle](https://github.com/opencontainers/runtime-spec/blob/v1.0.0-rc2/bundle.md) by:

* Following the ref to find a [manifest](manifest.md#image-manifest), possibly via a [manifest list](manifest-list.md#manifest-list)
* Following the ref to find a [manifest](manifest.md#image-manifest), possibly via a [image index](image-index.md#manifest-list)
Copy link
Contributor

Choose a reason for hiding this comment

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

“a image index” → “an image index”?

And do we really need to call this an “image index”? Can't it just be an “index”? There doesn't seem to be much specific to images about an array of descriptors.

Copy link
Member Author

Choose a reason for hiding this comment

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

this is like the Image Layout, no?

Copy link
Contributor

Choose a reason for hiding this comment

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

this is like the Image Layout, no?

Yeah, we should probably rename that too ;).

The only thing particularly image-specific about image-spec is the config format, and when we have sufficient context, we refer to that as “a configuration”. The full name (for when you don't have sufficient context) is “OCI Image Configuration”. I think that's a good pattern to follow, and it would mean we'd have something like the following formats:

Current Short Long
image layout directory storage OCI Image Directory Storage
manifest list index OCI Image Index
manifest manifest OCI Image Manifest
configuration configuration OCI Image Configuration
layer (and "layer filesystem changeset”) layer OCI Image Layer

Copy link
Contributor

Choose a reason for hiding this comment

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

OCI Image Directory Storage

WTAF. The image layout is NOT directory storage. OCI image spec DOES NOT prescribe a specific directory layout for storing images.

Stop propagating your fantasy. It is confusing people and holding things up.

Copy link
Contributor

@wking wking Feb 10, 2017

Choose a reason for hiding this comment

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

The image layout is NOT directory storage.

Image-layout is not storing directories, it is using a directory tree to store blobs and references. You can wrap that image-layout tree up in a tarball (or whatever you want), but it's still a directory-based format. That format stores opaque, content-addressable blobs, and generic references to them, so I don't see anything that is specific to images besides some informative hints.

Perhaps “directory-based storage” would make that distinction more clear?

Copy link
Contributor

Choose a reason for hiding this comment

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

OCI Image Directory Storage

To everyone else in the world, this does not mean a tar.

Copy link
Member Author

Choose a reason for hiding this comment

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

@wking i'll take a look for where it may stutter a bit, but your table is a excessive . 😬

Copy link
Contributor

Choose a reason for hiding this comment

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

@vbatts @wking I had a visceral rejection to "Directory Storage".

I would be fine with this table if we called it OCI Image Archive.

It is out of scope of this issue. Let's get this one merged for the media type change.

@@ -1,12 +1,12 @@
{
"description": "OpenContainer Image Manifest List Specification",
"$schema": "http://json-schema.org/draft-04/schema#",
"id": "https://opencontainers.org/schema/image/manifest-list",
"id": "https://opencontainers.org/schema/image/image-index",
Copy link
Contributor

Choose a reason for hiding this comment

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

Here's another place where “image index” seems over specific. Can we follow your …vnd.oci.image.index… media type (which does not double up on index and use …/image/index here (and the other ids you touch)?

spec.md Outdated
@@ -50,7 +50,7 @@ The [OCI Image Media Types](media-types.md) document is a starting point to unde

The high-level components of the spec include:

* An archival format for container images, consisting of an [image manifest](manifest.md), a [manifest list](manifest-list.md) (optional), an [image layout](image-layout.md), a set of [filesystem layers](layer.md), and [image configuration](config.md) (base OCI layer)
* An archival format for container images, consisting of an [image manifest](manifest.md), a [image index](image-index.md) (optional), an [image layout](image-layout.md), a set of [filesystem layers](layer.md), and [image configuration](config.md) (base OCI layer)
Copy link
Member

Choose a reason for hiding this comment

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

nit: a image -> an image

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

@vbatts
Copy link
Member Author

vbatts commented Feb 13, 2017

Updated. PTAL.

@stevvooe
Copy link
Contributor

@vbatts CI is failing.

@vbatts
Copy link
Member Author

vbatts commented Feb 13, 2017

fixed. dot wasn't on travis, and the image needed to be updated.

@stevvooe
Copy link
Contributor

stevvooe commented Feb 13, 2017

LGTM

Approved with PullApprove

spec.md Outdated
@@ -1,7 +1,7 @@
# Open Container Initiative
## Image Format Specification

This specification defines an OCI Image, consisting of a [manifest](manifest.md), a [manifest list](manifest-list.md) (optional), a set of [filesystem layers](layer.md), and a [configuration](config.md).
This specification defines an OCI Image, consisting of a [manifest](manifest.md), a [image index](image-index.md) (optional), a set of [filesystem layers](layer.md), and a [configuration](config.md).
Copy link
Contributor

Choose a reason for hiding this comment

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

“a image” → “an image”.

manifest.md Outdated
@@ -3,15 +3,15 @@
There are three main goals of the Image Manifest Specification.
The first goal is content-addressable images, by supporting an image model where the image's configuration can be hashed to generate a unique ID for the image and its components.
The second goal is to allow multi-architecture images, through a "fat manifest" which references image manifests for platform-specific versions of an image.
In OCI, this is codified in a [Manifest List](manifest-list.md).
In OCI, this is codified in a [Image Index](image-index.md).
Copy link
Contributor

Choose a reason for hiding this comment

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

“a Image” → “an Image” (and this “a i…” → “an i…” shows up in a few other places too).

@@ -62,6 +62,6 @@ The following figure shows how the above media types reference each other:
![](img/media-types.png)

[Descriptors](descriptor.md) are used for all references.
The manifest list being a "fat manifest" references one or more image manifests per target platform. An image manifest references exactly one target configuration and possibly many layers.
The image-index being a "fat manifest" references one or more image manifests per target platform. An image manifest references exactly one target configuration and possibly many layers.
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't hyphenate “image index” in most of the other places where you mention it. I don't care if you hyphenate it or not, but it's probably worth picking one way and staying consistent.

Copy link
Contributor

Choose a reason for hiding this comment

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

prefer without

schema/fs.go Outdated
local: ".content-descriptor.json.un~",
size: 1548,
modtime: 1485467092,
"/.fs.go.un~": {
Copy link
Contributor

Choose a reason for hiding this comment

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

More local files. I think you should land #538 (I've added a comment reproducing my successfully ignoring local files like this to that PR) or remove your local editor files before rebuilding fs.go.

@wking
Copy link
Contributor

wking commented Feb 14, 2017

Also, still some dangling references to clean up:

$ git grep -i manifest.list origin/pr/546
origin/pr/546:annotations.md:This specification defines the following annotation keys, intended for but not limited to manifest list and image manifest authors:
origin/pr/546:image-index.md:The manifest list is a higher-level manifest which points to specific [image manifests](manifest.md) for one or more platforms.
origin/pr/546:image-index.md:While the use of a manifest list is OPTIONAL for image providers, image consumers SHOULD be prepared to process them.
origin/pr/546:image-index.md:    Manifest lists concerned with portability SHOULD use one of the above media types.
origin/pr/546:image-index.md:        Manifest lists SHOULD use, and implementations SHOULD understand, values [supported by runtime-spec's `platform.arch`][runtime-platform2].
origin/pr/546:image-index.md:        Manifest lists SHOULD use, and implementations SHOULD understand, values [supported by runtime-spec's `platform.os`][runtime-platform2].
origin/pr/546:media-types.md:- [application/vnd.docker.distribution.manifest.list.v2+json](https://github.com/docker/distribution/blob/master/docs/spec/manifest-v2-2.md#manifest-list) - mediaType is different
origin/pr/546:schema/image-index-schema.json:  "description": "OpenContainer Image Manifest List Specification",
origin/pr/546:schema/manifest_backwards_compatibility_test.go:  "application/vnd.docker.distribution.manifest.list.v2+json": v1.MediaTypeImageIndex,
origin/pr/546:schema/manifest_backwards_compatibility_test.go:   "mediaType": "application/vnd.docker.distribution.manifest.list.v2+json",
origin/pr/546:specs-go/v1/image_index.go:       // Annotations contains arbitrary metadata for the manifest list.

The application/vnd.docker.distribution.manifest.list.v2+json entries are fine, but the others should be updated.

Copy link
Member

@coolljt0725 coolljt0725 left a comment

Choose a reason for hiding this comment

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

And https://github.com/opencontainers/image-spec/blob/master/annotations.md#annotations should changes the line ‘This specification defines the following annotation keys, intended for but not limited to manifest list and image manifest authors’ which contains the manifest list

image-index.md Outdated
@@ -1,12 +1,12 @@
# OCI Image Manifest List Specification
# OCI Image Index Specification

The manifest list is a higher-level manifest which points to specific [image manifests](manifest.md) for one or more platforms.
Copy link
Member

Choose a reason for hiding this comment

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

should this manifest list also change to image index ?

image-index.md Outdated
@@ -1,12 +1,12 @@
# OCI Image Manifest List Specification
# OCI Image Index Specification

The manifest list is a higher-level manifest which points to specific [image manifests](manifest.md) for one or more platforms.
While the use of a manifest list is OPTIONAL for image providers, image consumers SHOULD be prepared to process them.
Copy link
Member

Choose a reason for hiding this comment

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

type ManifestList struct {
// ImageIndex references manifests for various platforms.
// This structure provides `application/vnd.oci.image.index.v1+json` mediatype when marshalled to JSON.
type ImageIndex struct {
Copy link
Member

Choose a reason for hiding this comment

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

comment of Annotations // Annotations contains arbitrary metadata for the manifest list. below should change to manifest list to image index

@coolljt0725
Copy link
Member

oh, sorry for the noise :), seems @wking has point out these reference that's need to be clean up in #546 (comment)

@jonboulle
Copy link
Contributor

seems like this needs a rebase and a few more fixes from the last couple reviews (at a glance I still see several manifest list references too)

{
"schemaVersion": 2,
"manifests": [
{
"mediaType": "application/vnd.oci.image.manifest.v1+json",
"mediaType": "application/vnd.oci.image.index.v1+json",
"size": 7143,
"digest": "sha256:0228f90e926ba6b96e4f39cf294b2586d38fbb5a1e385c05cd1ee40ea54fe7fd",
"annotations": {

Choose a reason for hiding this comment

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

Is org.opencontainers.ref.name in annotations valid for image index?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes. Just like a file ./refs/stable could be a descriptor of a manifest-list, this named reference points to a now image-index (previously manifest-list). I only changed this example to demo an image-index, rather than only a manifest.

Choose a reason for hiding this comment

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

Got it, thanks!

@vbatts vbatts added this to the v1.0.0-rc5 milestone Feb 15, 2017
@vbatts
Copy link
Member Author

vbatts commented Feb 15, 2017

rebased and fixed. PTAL

@@ -1,6 +1,6 @@
# Extensibility

Implementations that are reading/processing [manifests](manifest.md) or [manifest lists](manifest-list.md) MUST NOT generate an error if they encounter an unknown property.
Implementations that are reading/processing [manifests](manifest.md) or [image index](image-index.md) MUST NOT generate an error if they encounter an unknown property.
Copy link
Contributor

Choose a reason for hiding this comment

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

"image indexes" or "the image index"?

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

annotations.md Outdated
@@ -15,7 +15,7 @@ Consumers MUST NOT generate an error if they encounter an unknown annotation key

## Pre-Defined Annotation Keys

This specification defines the following annotation keys, intended for but not limited to manifest list and image manifest authors:
This specification defines the following annotation keys, intended for but not limited to [image-index](image-index.md) and image [manifest](manifest.md) authors:
Copy link
Contributor

Choose a reason for hiding this comment

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

Hyphenated "image-index" here (you seem to prefer the unhyphenated "image index").

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

image-index.md Outdated

The manifest list is a higher-level manifest which points to specific [image manifests](manifest.md) for one or more platforms.
While the use of a manifest list is OPTIONAL for image providers, image consumers SHOULD be prepared to process them.
The Image Index is a higher-level manifest which points to specific [image manifests](manifest.md), ideal for one or more platforms.
Copy link
Contributor

Choose a reason for hiding this comment

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

Title-case "Image Index" here (you seem to prefer the sentence-case "image index).

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

image-index.md Outdated
@@ -34,7 +34,7 @@ For the media type(s) that this document is compatible with, see the [matrix][ma

- [`application/vnd.oci.image.manifest.v1+json`](manifest.md)

Manifest lists concerned with portability SHOULD use one of the above media types.
Image Indexes are concerned with portability SHOULD use one of the above media types.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/are concerned/concerned/

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

@vbatts vbatts force-pushed the manifest-list-rename branch 2 times, most recently from 789e261 to 8f4ab26 Compare February 15, 2017 22:38
@vbatts
Copy link
Member Author

vbatts commented Feb 15, 2017

updated PTAL

@philips
Copy link
Contributor

philips commented Feb 15, 2017

LGTM

Approved with PullApprove

image-index.md Outdated

The manifest list is a higher-level manifest which points to specific [image manifests](manifest.md) for one or more platforms.
While the use of a manifest list is OPTIONAL for image providers, image consumers SHOULD be prepared to process them.
The Image index is a higher-level manifest which points to specific [image manifests](manifest.md), ideal for one or more platforms.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if we're punting copy-edits to a follow-up commit or not, but “Image” → “image” (either in this PR or in the follow-up copy-edit).

Copy link
Member Author

Choose a reason for hiding this comment

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

Well I would not mind punting copy-edits, but I've already got to rebase again.

@@ -1,12 +1,12 @@
digraph G {
{
manifestList [shape=note, label="Manifest list\n<<optional>>\napplication/vnd.oci.image.manifest.list.v1+json"]
imageIndex [shape=note, label="Image Index<<optional>>\napplication/vnd.oci.image.index.v1+json"]
Copy link
Contributor

Choose a reason for hiding this comment

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

This dropped a newline with “Manifest list\n” → “Image Index”. I don't know if that was intentional or not, although my preference would be to keep the newline.

Copy link
Member Author

Choose a reason for hiding this comment

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

will pick that up.

manifest.md Outdated
@@ -3,15 +3,15 @@
There are three main goals of the Image Manifest Specification.
The first goal is content-addressable images, by supporting an image model where the image's configuration can be hashed to generate a unique ID for the image and its components.
The second goal is to allow multi-architecture images, through a "fat manifest" which references image manifests for platform-specific versions of an image.
In OCI, this is codified in a [Manifest List](manifest-list.md).
In OCI, this is codified in an [Image Index](image-index.md).
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Earlier changes seemed to be moving towards sentence-casing these references.

Copy link
Member Author

Choose a reason for hiding this comment

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

meh. I'm two ways about it.

@@ -202,10 +202,83 @@ func _escFSMustString(useLocal bool, name string) string {

var _escData = map[string]*_escFile{

"/backwards_compatibility_test.go": {
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to add *.go entries in fs.go.

Copy link
Member Author

Choose a reason for hiding this comment

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

?

@@ -379,735 +452,9107 @@ hR+PkX5zgcX30ucJvpgCXqksmwP73wn8U2qZo81xwnEy+6yyZ6Vexsl79az+DgAA//850zAaggUAAA==

"/fs.go": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh, recursive http.FileSystem! And not new to this PR either :p. In fact, it looks like it goes all the way back to #65.

Copy link
Member Author

Choose a reason for hiding this comment

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

so, this issue is outside of 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.

Not really, since you're supposed to be ignoring *.go. Your copy of esc just seems to ignore -ignore=… :p. For example, here's my most-recently-landed fs.go rebuild, and it only contains *.json.

Copy link
Contributor

Choose a reason for hiding this comment

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

And in case it helps, here's the history of the recursive /fs.go's presence in master:

$ git log --decorate -p -G '/fs.go' origin/master -- schema/fs.go | grep '^commit \|Author\|"/fs.go"'
commit 0556a6bee5ee1514762de05fd95d7af09b38f07f (origin/pr/533)
Author: Vincent Batts <vbatts@hashbangbash.com>
+       "/fs.go": {
commit 10e6ff7101c2b0ff76e41a4a213838953e2082d8 (origin/pr/78)
Author: Jonathan Boulle <jonathanboulle@gmail.com>
-       "/fs.go": {
commit 4263cf529b30d46b99a761ca764ede70b667cdc9 (origin/pr/65)
Author: Sergiusz Urbaniak <sur@coreos.com>
+       "/fs.go": {

Since #533, only @xiekeyang has rebuilt fs.go:

$ git log --decorate 0556a6be..origin/master -- schema/fs.go | grep '^commit \|Author'
commit 5bf90dd1ed7a0607c7fdc6013e3841952d228d43 (origin/pr/564)
Author: xiekeyang <xiekeyang@huawei.com>

Who seems to have the same buggy esc behavior:

$ git cat-file -p 5bf90dd1:schema/gen.go | grep 'generate esc'
//go:generate esc -private -pkg=schema -ignore=.*go -ignore=.*swp .
$ git cat-file -p 5bf90dd1:schema/fs.go | grep '"/'
        "/config-schema.json": {
        "/config_test.go": {

My theory is still that you have an old, buggy copy of esc ;).

@@ -22,7 +22,7 @@
}
},
"annotations": {
"id": "https://opencontainers.org/schema/image/manifest-list/annotations",
"id": "https://opencontainers.org/schema/image/manifest/annotations",
Copy link
Contributor

Choose a reason for hiding this comment

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

This is also in flight with #484. I guess whichever way gets the fix in first… ;)

@@ -27,6 +27,6 @@ type Manifest struct {
// Layers is an indexed list of layers referenced by the manifest.
Layers []Descriptor `json:"layers"`

// Annotations contains arbitrary metadata for the manifest list.
// Annotations contains arbitrary metadata for the manifest.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is also in flight with #484. I guess whichever way gets the fix in first… ;)

@jbouzane
Copy link

jbouzane commented Feb 16, 2017

LGTM

Approved with PullApprove

It has come up a number of times that the manifest-list, while
intentionally suited for pointing to a list of manifests, is intended to
be a general index and entry-point. During the image-layout addition of
`/index.json`, replacing the ./refs/ directory, it was recommended that
we finally make this distinction in the manifest-list as well.

This rename does not affect the compatibility with the docker v2.s2
manifest-list, as it is primarily a semantic change.

Signed-off-by: Vincent Batts <vbatts@hashbangbash.com>
@vbatts
Copy link
Member Author

vbatts commented Feb 16, 2017

rebased and updated. PTAL

@vbatts
Copy link
Member Author

vbatts commented Feb 16, 2017 via email

@vbatts
Copy link
Member Author

vbatts commented Feb 16, 2017 via email

@stevvooe
Copy link
Contributor

stevvooe commented Feb 16, 2017

LGTM

Approved with PullApprove

1 similar comment
@philips
Copy link
Contributor

philips commented Feb 16, 2017

LGTM

Approved with PullApprove

@stevvooe stevvooe merged commit a125828 into opencontainers:master Feb 16, 2017
@vbatts vbatts deleted the manifest-list-rename branch February 17, 2017 04:10
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.

9 participants