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

schema: Update the json file #638

Closed
wants to merge 2 commits into from

Conversation

zhouhao3
Copy link

@zhouhao3 zhouhao3 commented Apr 5, 2017

Update the corresponding json file and spec file according to #620

Signed-off-by: zhouhao zhouhao@cn.fujitsu.com

Fixes #618

Signed-off-by: zhouhao <zhouhao@cn.fujitsu.com>
Copy link
Contributor

@jonboulle jonboulle left a comment

Choose a reason for hiding this comment

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

On thinking about this further I am not sure this makes things clearer, given that we still need to retain the qualification (here) in the image-index description. Perhaps it would make more sense to leave platform where it is, and instead simply augment the descriptor description here to mention that "descriptors pointing to certain types may include extended fields" or similar ..?

@@ -43,32 +43,6 @@ For the media type(s) that this document is compatible with, see the [matrix][ma
This OPTIONAL property describes the platform which the image in the manifest runs on.
Copy link
Contributor

Choose a reason for hiding this comment

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

line 28 should be changed from "with the following additional properties and restrictions" to "with the following requirements and restrictions" or similar

Copy link
Author

@zhouhao3 zhouhao3 Apr 6, 2017

Choose a reason for hiding this comment

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

updated, what do you think of 3e7b8d0.

@stevvooe
Copy link
Contributor

stevvooe commented Apr 5, 2017

Platform shouldn't be valid on descriptors outside usage on the index. While we've changed the Go datatype to make processing easier, the specification still reads correctly.

@stevvooe
Copy link
Contributor

stevvooe commented Apr 5, 2017

I missed #638 (review).

Perhaps it would make more sense to leave platform where it is, and instead simply augment the descriptor description here to mention that "descriptors pointing to certain types may include extended fields" or similar ..?

This is probably the correct approach here.

@qianzhangxa
Copy link
Contributor

Agree, that should be the correct approach.

Signed-off-by: zhouhao <zhouhao@cn.fujitsu.com>
@stevvooe
Copy link
Contributor

stevvooe commented Apr 6, 2017

I'm going to go ahead and close this favor of @jonboulle's suggestion in #638 (review).

@stevvooe stevvooe closed this Apr 6, 2017
@qianzhangxa
Copy link
Contributor

But based on @jonboulle's comment, we need to augment the descriptor description, so we still need another PR to update descriptor.md?

@stevvooe
Copy link
Contributor

stevvooe commented Apr 7, 2017

@qianzhangxa Yes, but this PR needs to be closed.

@zhouhao3 zhouhao3 deleted the plat-form branch May 26, 2017 06:12
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.

4 participants