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: Limit schemaVersion to 2 and enumerate mediaType #404

Merged
merged 3 commits into from
Nov 17, 2016

Conversation

wking
Copy link
Contributor

@wking wking commented Oct 20, 2016

For manifests and manifest lists, to match the requirements from their Markdown definitions.

@xiekeyang
Copy link
Contributor

@wking

  1. Do you forget to refresh the generated virtual file system, to see https://github.com/opencontainers/image-spec/blob/master/HACKING.md#virtual-schema-httpfilesystem?
  2. It seems that you want to reject all media types in compatibility-matrix, that may be conflict to our previous conclusion. I worry that we fall into the confusion again that media type should be loose or strict. After read Intra-blob JSON validation correctness and image-spec release policy #406 , I still think this PR is too strict.

@wking
Copy link
Contributor Author

wking commented Oct 21, 2016

On Thu, Oct 20, 2016 at 09:48:41PM -0700, xiekeyang wrote:

  1. Do you forget to refresh the generated virtual file system…

Oops, yeah. Fixed with the just-pushed 00f177b.

  1. It seems that you want to reject all media types in compatibility-matrix…

I think there are two things going on here: what you are and what you reference. #304 is about who you reference, and the pattern is that we require implementations to support appropriate OCI types but allow images and implementations to use other types as well. This PR, on the other hand, is about declaring what you are, and there's only one good answer for that.

An application/vnd.oci.image.manifest.v1+json with application/vnd.docker.container.image.v1+json in config.mediaType is legal (although there may be compliant runtimes that don't support it). An application/vnd.oci.image.manifest.v1+json with application/vnd.docker.distribution.manifest.v2+json in mediaType is not legal.

@wking
Copy link
Contributor Author

wking commented Oct 21, 2016

I've just pushed an additional commit (801699e) fixing the convertFormats direction to make Travis happy, so this stricter checking has already been useful :).

@vbatts
Copy link
Member

vbatts commented Nov 1, 2016

needs a rebase

@wking
Copy link
Contributor Author

wking commented Nov 1, 2016

On Tue, Nov 01, 2016 at 07:51:41AM -0700, Vincent Batts wrote:

needs a rebase

Rebased around #413's schema/fs.go bump with 801699ec23165c.

@philips
Copy link
Contributor

philips commented Nov 16, 2016

LGTM

Conflict though.

Approved with PullApprove

For manifests and manifest lists, to match the requirements from their
Markdown definitions.

Signed-off-by: W. Trevor King <wking@tremily.us>
Generated with:

  $ make schema-fs

Signed-off-by: W. Trevor King <wking@tremily.us>
We want to go from the Docker type (the key) to the OCI type (the
value), not the other way around.

Signed-off-by: W. Trevor King <wking@tremily.us>
@wking
Copy link
Contributor Author

wking commented Nov 16, 2016

On Wed, Nov 16, 2016 at 02:38:58PM -0800, Brandon Philips wrote:

Conflict though.

Rebased onto master and rebuilt schema/fs.go with c23165cdad01a1.

It's probably best if you order up all the JSON-Schema-touching PRs
(maybe tag them all?), because all of them are going to conflict on
fs.go.

An alternative would be to ask contributors to not update fs.go and
have the merging maintainer rebuild fs.go after merging the change.
In this case you'd also want to add a Travis line rebuilding fs.go
before running the tests.

@philips
Copy link
Contributor

philips commented Nov 17, 2016

LGTM

ptal @opencontainers/image-spec-maintainers

Approved with PullApprove

@stevvooe
Copy link
Contributor

stevvooe commented Nov 17, 2016

LGTM

Approved with PullApprove

@stevvooe stevvooe merged commit ebee8d2 into opencontainers:master Nov 17, 2016
@wking wking deleted the schema-version-json-schema branch January 19, 2017 23:52
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.

None yet

5 participants