add compatibility mediatype

[xiekeyang]

Add compatibility matrix media types like docker image.
Image tool implementation maybe need these types for validation, like #273 done.

Signed-off-by: xiekeyang [email protected]

[wking]

On Sat, Sep 17, 2016 at 11:51:17PM -0700, xiekeyang wrote:

A specs-go/compatibility/mediatype.go (29)

Having Go constants for these media types matches the existing
image-spec pattern, but I personally don't see the point. By the time
we require compatibility for anything, we should be past the point of
renames like #212. And #212 changed both the media type and the Go
const name anyway. So what do you gain by replacing a hard-coded:

"application/vnd.docker.distribution.manifest.list.v2+json"

with a hard-coded:

MediaTypeDockerImageManifestList

?

[runcom]

If this will be used in code actually, I'm +1 on this one, otherwise I don't see the point either.

[wking]

On Sun, Sep 18, 2016 at 12:21:30AM -0700, Antonio Murdaca wrote:

If this will be used in code actually…

I expect somebody (probably at least Docker) will be considering
these media types as they translate between OCI and Docker formats.
But they already have constants for them (e.g. 1).

[xiekeyang]

@wking @runcom
In #273 I validate the internal media type of config and layer, for manifest patten. In that patch we are talking about the implementation, but I feel you support me to add the media-type validation.
For that, compatibility matrix involves docker types, as this patch add. If it is OK, we need to add the validation logic code in image-tools project. But I think the const definition should follow spec-go/v1/mediatype.go to defined in image-spec project.

But they already have constants for them (e.g. [1]).
[1]: https://github.com/docker/distribution/blob/v2.5.1/manifest/manifestlist/manifestlist.go#L14

It seems that maintainers don't like to import docker package to oci projects, so I have to re-definition them here.

WDYT?

[wking]

On Sun, Sep 18, 2016 at 12:42:44AM -0700, xiekeyang wrote:

In #273 I validate the internal media type of config and layer, for
manifest patten. In that patch we are talking about the
implementation, but I feel you support me to add the media-type
validation.

Yes, I think we should be validating media types. See #304 for some
in-flight wording that I think we want before we adjust the tooling
though.

For that, compatibility matrix involves docker types, as this patch
add.

I don't think we need image-tools to support the Docker types
(although this seems to be up for discussion in the #304 thread). But
where it's easy to do so (most of the time?), I have no problem with
image-tools supporting the Docker types. My point here 1 is that
I'd rather have 2 changed from:

if err := m.Config.validate(w, []string{v1.MediaTypeImageConfig}); err != nil {

to:

err := m.Config.validate(w, []string{
"application/vnd.oci.image.config.v1+json",
})
if err != nil {

Using v1.MediaTypeImageConfig (or one of the constants you're adding
here) introduces a layer of indirection and associated mental overhead
to save you a few keystrokes, and I don't think that's a useful trade.

[runcom]

@wking so you prefer 40+ chars strings to consts (not in a war strings over const)? It is a useful trade to me to have const instead of strings, the level of indirection and metal overhead (?) are easy to fix with "go to definition" functionalities in your editor. Even if we won't rename anymore the media types, it doesn't mean we should start to hard code strings, that's personally not an argument to me.

This looks good to me if we're ever going to introduce these media types here.

[wking]

On Sun, Sep 18, 2016 at 01:07:32AM -0700, Antonio Murdaca wrote:

@wking so you prefer 40+ chars strings to consts…

That's easy to handle with the copy/paste function in your editor ;).
If you already have a canonical string representation, why coin
another one?

[xiekeyang]

@runcom @wking
I trade off proposals about supporting and validating media type in related PR and issues. We agree that validation should be done.
In my opinion, Docker types should be supported because spec involves it into Compatibility Matrix. If we don't add Docker, image tool's validation will failed to docker objects, which is conflict to spec.
And we had better to define it in image-spec project, because the Matrix is a part of image-spec, not image-tool.
It seems not important if chars strings is long or short, but we should make sure dependency and consistency between spec and tool.
Do you agree?

[wking]

On Sun, Sep 18, 2016 at 02:48:25AM -0700, xiekeyang wrote:

We agree that validation should be done.

Yup, although there is currently no spec wording around this. #304 is
trying to add some.

In my opinion, Docker types should be supported because spec
involves it into Compatibility Matrix. If we don't add Docker, image
tool's validation will failed to docker objects, which is conflict
to spec.

This is unclear. There is some discussion in #304, but I'd rather not
require tools handle Docker types in order to be OCI compliant (they'd
obviously have to handle the Docker types if they wanted to be Docker
compliant). As it stands, there are no RFC 2119 keywords in
media-types.md, which means (to me) that there are no normative
requirements in that file. But the spec is generally lacking RFC 2119
keywords, so it's possible media-types and/or the descriptor locations
touched by #304 grow some Docker requirements in the future.

And we had better to define it in image-spec project, because the
Matrix is a part of image-spec, not image-tool.

If we decide that image-spec is in the business of declaring Go
constants for media types, then yes, we should probably define
constants for all media types that impact compliance. I'm against
declaring those Go constants for reasons discussed above, but having a
consistent approach is more important to me than which approach we end
up using ;).

It seems not important if chars strings is long or short, but we
should make sure dependency and consistency between spec and tool.

If we get out of the business of declaring Go constants, than there is
no Go dependency between the spec and the tool on this point. And
regardless of whether we're declaring Go constants, we'll be declaring
media types in the Markdown. I agree that the tool will have to
consistently use whatever the spec provides.

[xiekeyang]

@wking
In opencontainers/image-tools/pull/10 I present media type validation not compliant to docker type.
Now we may keep types only compliant to oci, and decide, in future, if docker types should be declaried.
Maybe this patch can be closed.
please take a look on opencontainers/image-tools/pull/10. Thanks.

[wking]

On Sun, Sep 18, 2016 at 08:53:34PM -0700, xiekeyang wrote:

Now we may keep types only compliant to oci, and decide, in future,
if docker types should be declaried.

This sounds reasonable to me.

Maybe this patch can be closed.

It's probably worth waiting until #304 lands (or not) to see what the
consensus is on how the compatability matrix impacts compliance. If
the consensus is “OCI compliance requires support for the listed
Docker types” and there's also a consensus around “we want to provide
constants for referenced media types”, then we want this PR. I'm not
personally in favor of either, but that doesn't mean the consensus
will end up where I am now ;). And I think both are currently up in
the air.

[wking]

On Sun, Sep 18, 2016 at 09:30:17PM -0700, W. Trevor King wrote:

Maybe this patch can be closed.

It's probably worth waiting until #304 lands (or not) to see what
the consensus is on how the compatability matrix impacts compliance.

#304 landed, so I think the consensus is that we don't require support
for Docker media types and do not need the constants around them that
this PR is adding.

[xiekeyang]

OK, I'd close the PR.

[xiekeyang]

@wking

#304 landed, so I think the consensus is that we don't require support
for Docker media types and do not need the constants around them that
this PR is adding.

So, do you think that we should remove the unit test on Docker media type compatibility, as https://github.com/opencontainers/image-spec/blob/master/schema/manifest_backwards_compatibility_test.go#L27 ?

[xiekeyang]

@wking

Now I'm some confusing. I read all comments in #304. I assume that all ones agree that manifest.md doesn't go against to media-types.md #compatibility-matrix because that it is just informative.
Then, we should likely remove https://github.com/opencontainers/image-spec/blob/master/schema/manifest_backwards_compatibility_test.go#L27.
In schema package, we should add constraint only by manifest.md mentioned, right?

And then, we should remove file schema/manifest_backwards_compatibility_test.go because Docker types needn't to be supported.

Above is chain-reaction.