descriptor.md: add descriptions for the extended field #649
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
AkihiroSuda
reviewed
Apr 24, 2017
descriptor.md
Outdated
| @@ -43,6 +43,8 @@ The following fields contain the primary properties that constitute a Descriptor | |||
|
|
|||
| This OPTIONAL property contains arbitrary metadata for this descriptor. | |||
| This OPTIONAL property MUST use the [annotation rules](annotations.md#rules). | |||
|
|
|||
| Descriptors pointing to [`application/vnd.oci.image.manifest.v1+json`](manifest.md) SHOULD include the extended field `platform`, see [Image Index Property Descriptions](https://github.com/qianzhangxa/image-spec/blob/master/image-index.md#image-index-property-descriptions) for details. | |||
There was a problem hiding this comment.
The link target needs to be image-index.md#image-index-property-descriptions.
Also, if this is going to be merged, I think "This property (platform) SHOULD be present if its target is platform-specific." in image-index.md should be updated to "This property SHOULD be present"
There was a problem hiding this comment.
Thanks @AkihiroSuda. I have updated the code.
And I think we still need "if its target is platform-specific." in image-index.md since the manifest in a image index can be of other media type (not just application/vnd.oci.image.manifest.v1+json) in which case platform can be absent.
Signed-off-by: Qian Zhang <zhq527725@gmail.com>
|
I guess this is fine, but we already have a SHOULD for this IIRC |
|
On Tue, Apr 25, 2017 at 10:33:55AM -0700, v1.0.0.batts wrote:
I guess this is fine, but we already have a SHOULD for this IIRC
The existing SHOULD is in [1]. That SHOULD is tied to the descriptor
location (all descriptors in an index's ‘manifests’, regardless of
whether they link a manifest), while this SHOULD is tied to the
descriptor target (all descriptors linking a manifest, regardless of
whether the descriptor is in an index or not). So they're similar,
but not quite the same.
Personally, I think the SHOULD proposed here makes more sense if the
platform docs move over to descriptor.md (a Markdown version of #620),
and the current SHOULD makes more sense with the current spec (where
‘platform’ is unspecified outside of the index ‘manifests’ context).
I don't think we want the low-level descriptor.md referencing the very
high-level image-index.md to define descriptor properties.
[1]: https://github.com/opencontainers/image-spec/blame/a82c09852aba4978e3d16bb0cd583d5e2f4a63c9/image-index.md#L44
|
|
LGTM |
1 similar comment
|
LGTM |
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Signed-off-by: Qian Zhang zhq527725@gmail.com