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

remove unit test for compatability matrix #368

Closed
wants to merge 1 commit into from
Closed

remove unit test for compatability matrix #368

wants to merge 1 commit into from

Conversation

xiekeyang
Copy link
Contributor

From
#304

and
#320 (comment)

We seems agree not to support Docker media types.
If so, this unit test for Docker media support should be removed.
And we should add constraint only for OCI types.
cc @wking

Signed-off-by: xiekeyang [email protected]

@wking
Copy link
Contributor

wking commented Oct 4, 2016

fb63aee rolls back #145. I'm fine leaving it in to compare the
current spec against Docker JSON that has been caught in the wild.

I don't see Docker-compat as a requirement for external
implementations (it is explicitly not a requirement with #304), and I
don't think we need to facilitate Docker-compat in external
implementations (which is why I don't think we need #320). But I
don't have a problem with a spec constraint that image-spec v1
accept any Docker v2 content after a media-type change (which is what
these tests are about). On the other hand, I wouldn't mind making the
change more involved either (#224). So I don't mind either way on
this PR, but think #320 should stay closed.

@xiekeyang
Copy link
Contributor Author

@wking
Agree. I'd close #320.
Hopefully, this PR can be accepted, because this is required by #341.
#341 resolve the descendants validation in manifest. But UT can not pass for beyond OCI cases. So this PR should be stand firstly.

@wking
Copy link
Contributor

wking commented Oct 5, 2016

On Tue, Oct 04, 2016 at 09:33:13PM -0700, xiekeyang wrote:

Hopefully, this PR can be accepted, because this is required by
#341. #341 resolve the descendants validation in manifest. But UT
can not pass for beyond OCI cases. So this PR should be stand
firstly.

I'm not entirely sure I understand you here, but while
application/vnd.docker.distribution.manifest.v2+json is not an OCI
type, a compliant application/vnd.oci.image.manifest.list.v1+json MAY
reference one. #304 only requires OCI unpackers to understand the OCI
types, and an OCI manifest-list pointing at a Docker manifest should
get a “this may not be portable” warning from the image-validator but
should not raise a validation error. The same applies to other Docker
types. So I don't see a need to remove these compatibility tests (but
I wouldn't mind if we did).

On the other hand, #341 would error instead of printing a “may not be
portable” warning, so I agree that we don't want to handle this
portion of the validation in JSON Schema 1.

@xiekeyang xiekeyang closed this Oct 7, 2016
@xiekeyang xiekeyang deleted the med-ut branch November 4, 2016 06:44
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.

2 participants