schema: Add descriptor tests and fix mediaType pattern #448

wking · 2016-11-05T22:43:58Z

Cherry-picking the $ref fix from #444, since there was a lot going on
in that PR around the manifest list. Fixing those things is good, but
I thought it would help review to have something that just focused on
descriptor bugs.

CC @xiekeyang.

xiekeyang · 2016-11-06T03:13:21Z

schema/defs-image.json

@@ -4,7 +4,7 @@
    "mediaType": {
      "id": "https://opencontainers.org/schema/image/mediaType",
      "type": "string",
-      "pattern": "^[a-z]+/[0-9a-zA-Z.+]+$"
+      "pattern": "^[A-Za-z0-9][A-Za-z0-9!#$&-^_.+]{0,126}/[A-Za-z0-9][A-Za-z0-9!#$&-^_.+]{0,126}$"


https://tools.ietf.org/html/rfc6838#section-4.2 declare that:

Note that this syntax is somewhat more restrictive than what is
allowed by the ABNF in Section 5.1 of [RFC2045] or Section 4.2 of
[RFC4288]. Also note that while this syntax allows names of up to
127 characters, implementation limits may make such long names
problematic. For this reason, and SHOULD
be limited to 64 characters.

If we SHOULD limit the length to 64? After all, schema package is provided to customer for their implementation. We should eliminate problematic risk.

We previously didn't limit the length at all ;). I'd rather go with the most generous interpretation of the current RFC (which I think matches what I have here :p) and leave it to consumers to add additional checks if they like.

xiekeyang

unit test cases about descriptor looks good and enough. meanwhile, we should also keep test cases in manifest and manifest list files cover important definitions for descriptor, because we have been aware that they use different schema branch sometime(more checking will be done after this PR landed).

wking · 2016-11-06T06:04:38Z

On Sat, Nov 05, 2016 at 10:03:13PM -0700, xiekeyang wrote:

meanwhile, we should also keep test cases in manifest and
manifest list files cover important definitions for descriptor,
because we have been aware that they use different schema branch
sometime…

+1 for exactly this reason.

stevvooe · 2016-11-16T01:11:54Z

descriptor.md

@@ -19,7 +19,8 @@ The following fields contain the primary properties that constitute a Descriptor

 - **`mediaType`** *string*

-  This REQUIRED property contains the MIME type of the referenced content.
+  This REQUIRED property contains the media type of the referenced content.
+  Values MUST comply with [RFC 6838][rfc6838].


Restrict this MUST to the format in the rfc.

Was there a particular normative portion of RFC 6838 that you feel does not apply to mediaType values? For example, the vendor tree semantics are outside of the naming requirements section, and I think both of those apply to mediaType values.

Just reference 4.2 directly. Most of the rest of the specification is related to registration.

Most of the rest of the specification is related to registration

Doesn't all the registration stuff still apply? For example §3.2 has:

While public exposure and review of media types to be registered in the vendor tree are not required, using the [email protected] mailing list for review is encouraged, to improve the quality of those specifications.

so I think our unregistered vendor types are fine. And I don't think our spec should allow unregistered standards-tree types (violating [§3.1]) or other RFC-violating media types, even if they happen to match the §4.2 syntax.

If you'd like to draw attention to §4.2 in particular, I'd be fine with:

Values MUST comply with RFC 6838, including the naming requirements in its §4.2.

@wking That's fine.

@wking That's fine.

Done with 119ef52 → 15dbdf5.

wking · 2017-01-25T19:07:42Z

Rebased onto master with 9e597ab → 119ef52 now that #444 has landed.

Restore the MUST which was removed from the 15f9f74 version of this commit. My personal preference would be to just say: Values MUST comply with [RFC 6838][rfc6838]. But Stephen wanted a direct reference to 4.2 as well [1]. [1]: opencontainers#448 (comment) Signed-off-by: W. Trevor King <[email protected]>

stevvooe · 2017-01-25T23:25:07Z

LGTM

wking · 2017-01-26T00:27:32Z

This PR currently fails Travis [1], but the problem is unrelated (#530). [1]: https://travis-ci.org/opencontainers/image-spec/builds/195291380#L361

wking · 2017-01-26T04:50:09Z

On Wed, Jan 25, 2017 at 04:25:28PM -0800, W. Trevor King wrote: This PR currently fails Travis [1], but the problem is unrelated (#530).

Actually, since #444 picked up my JSON Schema change (15f9f74), I can drop the fs.go rebuild from this PR (it's covered by #530 and #531). I've done that with 15dbdf5 → 070cd2a, and now Travis is happy.

stevvooe · 2017-01-26T22:02:46Z

@opencontainers/image-spec-maintainers PTAL

Working these up turned up a few bugs in the old schema, which we've fixed in the earlier commits in this series. Signed-off-by: W. Trevor King <[email protected]>

wking · 2017-01-26T22:10:33Z

I just pushed 070cd2a → be553fb fixing ‘SHA256’ → ‘sha256’ for the “invalid hash characters” test. Now SHA256 only occurs in the “invalid aglorithm characters” test.

vbatts · 2017-01-31T15:28:00Z

LGTM

stevvooe · 2017-01-31T19:59:33Z

LGTM

Restore the MUST which was removed from the 15f9f741 version of this commit. My personal preference would be to just say: Values MUST comply with [RFC 6838][rfc6838]. But Stephen wanted a direct reference to 4.2 as well [1]. [1]: opencontainers/image-spec#448 (comment) Signed-off-by: W. Trevor King <[email protected]>

wking mentioned this pull request Nov 5, 2016

Bug Fix: error syntax in schema definitions #444

Merged

xiekeyang reviewed Nov 6, 2016

View reviewed changes

stevvooe reviewed Nov 16, 2016

View reviewed changes

wking mentioned this pull request Dec 1, 2016

Support different levels validation for JSON reader stream #452

Closed

wking force-pushed the descriptor-tests branch from 9e597ab to 119ef52 Compare January 25, 2017 19:07

wking force-pushed the descriptor-tests branch from 119ef52 to 15dbdf5 Compare January 25, 2017 23:22

wking force-pushed the descriptor-tests branch from 15dbdf5 to 070cd2a Compare January 26, 2017 04:43

schema/descriptor_test: Add descriptor tests

be553fb

Working these up turned up a few bugs in the old schema, which we've fixed in the earlier commits in this series. Signed-off-by: W. Trevor King <[email protected]>

wking force-pushed the descriptor-tests branch from 070cd2a to be553fb Compare January 26, 2017 22:07

stevvooe merged commit 5fccf47 into opencontainers:master Jan 31, 2017

wking deleted the descriptor-tests branch February 3, 2017 05:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

schema: Add descriptor tests and fix mediaType pattern #448

schema: Add descriptor tests and fix mediaType pattern #448

wking commented Nov 5, 2016

xiekeyang Nov 6, 2016

wking Nov 6, 2016

xiekeyang left a comment

wking commented Nov 6, 2016

stevvooe Nov 16, 2016

wking Nov 16, 2016

stevvooe Jan 25, 2017

wking Jan 25, 2017 •

edited

Loading

stevvooe Jan 25, 2017

wking Jan 25, 2017

wking commented Jan 25, 2017 •

edited

Loading

stevvooe commented Jan 25, 2017 •

edited by caniszczyk

Loading

wking commented Jan 26, 2017 via email

wking commented Jan 26, 2017 via email

stevvooe commented Jan 26, 2017

wking commented Jan 26, 2017 via email

vbatts commented Jan 31, 2017 •

edited by caniszczyk

Loading

stevvooe commented Jan 31, 2017 •

edited by caniszczyk

Loading

schema: Add descriptor tests and fix mediaType pattern #448

schema: Add descriptor tests and fix mediaType pattern #448

Conversation

wking commented Nov 5, 2016

xiekeyang Nov 6, 2016

Choose a reason for hiding this comment

wking Nov 6, 2016

Choose a reason for hiding this comment

xiekeyang left a comment

Choose a reason for hiding this comment

wking commented Nov 6, 2016

stevvooe Nov 16, 2016

Choose a reason for hiding this comment

wking Nov 16, 2016

Choose a reason for hiding this comment

stevvooe Jan 25, 2017

Choose a reason for hiding this comment

wking Jan 25, 2017 • edited Loading

Choose a reason for hiding this comment

stevvooe Jan 25, 2017

Choose a reason for hiding this comment

wking Jan 25, 2017

Choose a reason for hiding this comment

wking commented Jan 25, 2017 • edited Loading

stevvooe commented Jan 25, 2017 • edited by caniszczyk Loading

wking commented Jan 26, 2017 via email

wking commented Jan 26, 2017 via email

stevvooe commented Jan 26, 2017

wking commented Jan 26, 2017 via email

vbatts commented Jan 31, 2017 • edited by caniszczyk Loading

stevvooe commented Jan 31, 2017 • edited by caniszczyk Loading

wking Jan 25, 2017 •

edited

Loading

wking commented Jan 25, 2017 •

edited

Loading

stevvooe commented Jan 25, 2017 •

edited by caniszczyk

Loading

vbatts commented Jan 31, 2017 •

edited by caniszczyk

Loading

stevvooe commented Jan 31, 2017 •

edited by caniszczyk

Loading