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

fix error valid target in image-layout #579

Merged
merged 1 commit into from
Mar 10, 2017
Merged

fix error valid target in image-layout #579

merged 1 commit into from
Mar 10, 2017

Conversation

xiekeyang
Copy link
Contributor

In layout.md, The validated target should be image layout itself, not only image index.

I'd like to extract and land this firstly, while #452 is in WIP.

Signed-off-by: xiekeyang xiekeyang@huawei.com

In layout.md, The validated target should be image layout itself, not
image index.

Signed-off-by: xiekeyang <xiekeyang@huawei.com>
@@ -134,7 +134,7 @@ The `imageLayoutVersion` value will align with the OCI Image Specification versi

### oci-layout Example

```json
```json,title=OCI%20Layout&mediatype=oci%2Dlayout
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 still rather have “oci-layout file” (or similar) as the title and a real media type.

Copy link
Contributor Author

@xiekeyang xiekeyang Feb 23, 2017

Choose a reason for hiding this comment

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

I'd still rather have “oci-layout file” (or similar) as the title and a real media type.

it likely make no much sense. Refer to manifest.md or so.

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 still rather have “oci-layout file” (or similar) as the title and a real media type.

it likely make no much sense. Refer to manifest.md or so.

manifest.md has title=Manifest, which makes sense, because the JSON it's titling is a manifest. Here, the JSON we're titling is an oci-layout file, so I think we want title=oci-layout%20file.

Copy link
Contributor

Choose a reason for hiding this comment

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

How is mediatype=oci%2Dlayout a valid mediatype?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@stevvooe
Sorry for late response. This is because of https://github.com/opencontainers/image-spec/blob/master/schema/spec_test.go#L70. It means that only those who have mediatype JSON key will be validated. Any arbitrary string here will not influence it to be joined to spec-test function. I use oci-layout, as to close to description in image-layout-schema.json.
If we need a nicer name here?

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. We should probably note somewhere that this is not a mediatype.

@xiekeyang
Copy link
Contributor Author

@opencontainers/image-spec-maintainers This might be ready to merged, PTAL.

@vbatts
Copy link
Member

vbatts commented Mar 8, 2017

I'm not opposed, though oci-layout doesn't have a mediaType yet. Perhaps we need this for validating them and this example as well.

@vbatts
Copy link
Member

vbatts commented Mar 8, 2017

oh we do! I was searching for "oci-layout" and missed ./schema/image-layout-shema.json. I'll add more to that description.

LGTM

Approved with PullApprove

vbatts added a commit to vbatts/oci-image-spec that referenced this pull request Mar 8, 2017
opencontainers#579 (comment)

Signed-off-by: Vincent Batts <vbatts@hashbangbash.com>
vbatts added a commit to vbatts/oci-image-spec that referenced this pull request Mar 8, 2017
opencontainers#579 (comment)

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

jonboulle commented Mar 10, 2017

lgtm, will let @stevvooe ack/nack tho

Approved with PullApprove

@stevvooe
Copy link
Contributor

stevvooe commented Mar 10, 2017

LGTM

Approved with PullApprove

@stevvooe
Copy link
Contributor

@xiekeyang Follow this up with a note somewhere that oci-layout is not a mediatype.

@stevvooe stevvooe merged commit fa4c9fb into opencontainers:master Mar 10, 2017
@xiekeyang
Copy link
Contributor Author

@stevvooe got it. i would present another patch for this.

@xiekeyang xiekeyang deleted the bugfix-oci-layout branch March 13, 2017 03:21
@vbatts vbatts mentioned this pull request May 19, 2017
dattgoswami9lk5g added a commit to dattgoswami9lk5g/bremlinr that referenced this pull request Jun 6, 2022
7c00d pushed a commit to 7c00d/J1nHyeockKim that referenced this pull request Jun 6, 2022
7c00d added a commit to 7c00d/J1nHyeockKim that referenced this pull request Jun 6, 2022
laventuraw added a commit to laventuraw/Kihara-tony0 that referenced this pull request Jun 6, 2022
tomalopbsr0tt added a commit to tomalopbsr0tt/fabiojosej that referenced this pull request Oct 6, 2022
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.

5 participants