Skip to content

Conversation

@s-urbaniak
Copy link
Collaborator

Fixes #74

@s-urbaniak s-urbaniak force-pushed the validate-config branch 3 times, most recently from 6e049d2 to 7a6d374 Compare May 19, 2016 23:32
Sergiusz Urbaniak added 4 commits May 19, 2016 20:07
Signed-off-by: Sergiusz Urbaniak <[email protected]>
This implements the validation for the config/image JSON format.

Signed-off-by: Sergiusz Urbaniak <[email protected]>
@s-urbaniak
Copy link
Collaborator Author

@stevvooe @philips @vbatts PTAL

"type": "string"
},
"config": {
"$ref": "defs-config.json#/definitions/config"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is probably outside the scope of this PR: let's make sure we have some negative examples of the config to ensure that the validation is working with these recursively definitions. I know I had some issues with this when I first started experimenting with the validators.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 I have this on my radar, as well as the support for providing the erroneous line number in case of validation failure.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@s-urbaniak I've filed #83. Feel free to expand on anything I've missed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is recursive here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vbatts config-schema.json pulls in defs-config.json. During my experimentation, I was not convinced this was working correctly. Either way, we should have negative examples to confirm this and ensure that bad schemas are actually being rejected.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, so. Not working is an understandable concern. I just did not see recursion. :-)

}

for _, example := range examples {
if example.Err == errFormatInvalid { // ignore
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm why?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are many examples/snippets in the spec not meant for validation.
Hmm, but you're right, we should only ignore if there is no MediaType set.

@s-urbaniak
Copy link
Collaborator Author

@jonboulle PTAL

@jonboulle
Copy link
Contributor

LGTM

"properties": {
"created": {
"type": "string",
"format": "date-time"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this format too strict for "2015-10-31T22:22:56.015925234Z"?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On Mon, May 23, 2016 at 01:51:33PM -0700, Vincent Batts wrote:

  • "created": {
  •  "type": "string",
    
  •  "format": "date-time"
    

is this format too strict for "2015-10-31T22:22:56.015925234Z"?

The v4 JSON Schema RFC doesn't mention date-time 1, but it's listed
here 2 referencing RFC 3339, section 5.6. And your string is
compatible with that 3.

@vbatts
Copy link
Member

vbatts commented May 23, 2016

overall LGTM, but a couple of questions

@stevvooe
Copy link
Contributor

LGTM

1 similar comment
@vbatts
Copy link
Member

vbatts commented May 23, 2016

LGTM

@vbatts vbatts merged commit 0aafcef into opencontainers:master May 23, 2016
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.

5 participants