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

examples: validate markdown examples #41

Merged
merged 2 commits into from
Apr 29, 2016

Conversation

stevvooe
Copy link
Contributor

To ensure that examples in the specification match up with the json
schemas, we propose a validation approach that will extract the fenced
code blocks from the markdown document and validate them. Examples are
marked with specification mediatypes which are then validated against
the respective json shema doc.

From here, we can propose that all examples be part of the
specification. Each act of clarification would be represented within the
specification and validated against the schema.

The tool can be run from the root repository directory with the
following command:

$ go run examples.go < manifest.md
ok   application/vnd.oci.image.manifest.list.v1+json
ok   application/vnd.oci.image.manifest.v1+json

This is a work in progress to validate the concept. The following needs
to be addressed before merging:

  1. Decide on output format.
  2. Provide more context for each example.
  3. Integrate with top-level Makefile.
  4. Move tools (validate.go, examples.go) to cmd/ directory.
  5. Run validation as part of travis CI testsuite.

Closes #34, requires #40

Signed-off-by: Stephen J Day stephen.day@docker.com


var specsByMediaType = map[string]string{
"application/vnd.oci.image.manifest.v1+json": "schema/image-manifest-schema.json",
"application/vnd.oci.image.manifest.list.v1+json": "schema/manifest-list-schema.json",
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably better to have a pattern like “remove the application/vnd.oci. prefix and replace + with .” to get filenames from media types.

Copy link
Member

Choose a reason for hiding this comment

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

eh? I don't follow.

@stevvooe it does make me think the schema files could be named more aligned with the mime type

Copy link
Contributor

Choose a reason for hiding this comment

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

@vbatts Why not name the files exactly the same as the mime type?

Copy link
Member

Choose a reason for hiding this comment

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

gosh. I must be dyslexic.
This is what I was thinking

Copy link
Contributor

Choose a reason for hiding this comment

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

On Fri, Apr 22, 2016 at 07:26:53AM -0700, Brandon Philips:
“Why not name the files exactly the same as the mime type?”

Reasons why I like:

$ echo application/vnd.oci.image.manifest.v1+json |

sed 's|application/vnd.oci.||;s|[+]|.|'
image.manifest.v1.json

more are:

  1. application/stuff is a filename on Windows and a path into a
    subdirectory on POSIX.
  2. vnd.oci. is noise in this repo (although that noise isn't a
    terrible problem if you want to keep it).
  3. +json is an odd file extension (although again, you can keep it if
    you like).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vbatts @philips @wking There is already quite a but of magic going on here. I thought it would sensible to have this file mapping explicitly called out. I suspect this list will remain small, so maintaining it should be of minimal burden.

Copy link
Contributor

Choose a reason for hiding this comment

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

WFM for now

Copy link
Contributor

Choose a reason for hiding this comment

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

@stevvooe It would be good to stay consistent. For example image-manifest-schema and image-manifest-list-schema. manifest-list is missing image-.

#bikeshed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@philips I noticed that. I'm also not sure about the image- prefix.

Let's fix it up in another PR.

(sidenote: international emoji for bikeshed is 🚲 🏠)

Copy link
Contributor

Choose a reason for hiding this comment

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

ack

@wking
Copy link
Contributor

wking commented Apr 22, 2016

On Thu, Apr 21, 2016 at 05:39:16PM -0700, Stephen Day wrote:

To ensure that examples in the specification match up with the json
schemas, we propose a validation approach that will extract the fenced
code blocks from the markdown document and validate them.

The drawback to this approach (vs. stand-alone files will all embedded
examples being subsets of the stand-alone files 1), is that all your
embedded examples need to completely fill a given schema. You'll need
a different approach if you want to validate subsets. I don't see any
subsets like that in this repository at the moment, but the
runtime-spec repo uses lots of them (e.g. 2).

@philips
Copy link
Contributor

philips commented Apr 22, 2016

@wking if it gets annoying we can reconsider.

@philips
Copy link
Contributor

philips commented Apr 27, 2016

@stevvooe Shall we just merge this as is?

@stevvooe
Copy link
Contributor Author

@philips I've been out with a surgery for a few days. I'll take a clean up pass over this and we can go forward with it.

@philips
Copy link
Contributor

philips commented Apr 27, 2016

@stevvooe Oh! Well, I hope you have a speedy recovery! If you want one of us can take it over and merge it in. Let us know!

@philips
Copy link
Contributor

philips commented Apr 28, 2016

@stevvooe Shall I just merge this as-is then do follow-up PRs?

@stevvooe
Copy link
Contributor Author

@philips I'm putting the changes in right now.

To ensure that examples in the specification match up with the json
schemas, we propose a validation approach that will extract the fenced
code blocks from the markdown document and validate them. Examples are
marked with specification mediatypes which are then validated against
the respective json shema doc.

From here, we can propose that all examples be part of the
specification. Each act of clarification would be represented within the
specification and validated against the schema.

The tool can be run from the root repository directory with the
following command:

```console
$ go run examples.go < manifest.md
ok	 application/vnd.oci.image.manifest.list.v1+json
ok	 application/vnd.oci.image.manifest.v1+json
```

This changeset should be followeb by the following tasks:

1. Integrate with top-level Makefile.
2. Move tools (validate.go, examples.go) to `cmd/` directory.
3. Run validation as part of travis CI testsuite.

Note that there seems to be a bug in the schema validator when using
recursive references. We can investigate this after this PR is merged.

Signed-off-by: Stephen J Day <stephen.day@docker.com>
We can work out the hierarchy more carefully and likely merge these, but
these tools have been moved into cmd/, per Go best practices.

Signed-off-by: Stephen J Day <stephen.day@docker.com>
@stevvooe stevvooe force-pushed the validate-examples branch from 8b8e367 to 48c6221 Compare April 28, 2016 21:23
@stevvooe
Copy link
Contributor Author

Changes:

  • Cleaned up code and output
  • Changed to future-proof format for lang in fenced blocks
  • Moved validation tools to cmd/ hierarchy
  • Moved Makefile to top-level

Follow up items:

  • Maintainers should play with tool
  • Project build/layout considerations
  • Add checks to travis
  • Possibly merge validation tools

Caveats:

  • There is a problem with recursive schema validation. Try editing a manifest descriptor in the specification; the errors don't show up. Please confirm and I'll file a bug with gojsonschema.

Recommendation:

  • Merge this as is and take on follow up tasks

@philips
Copy link
Contributor

philips commented Apr 28, 2016

LGTM

@s-urbaniak is looking at merging validation tools via #43

@stevvooe stevvooe changed the title [WIP] examples: validate markdown examples examples: validate markdown examples Apr 28, 2016
@vbatts
Copy link
Member

vbatts commented Apr 29, 2016

cool cool. @stevvooe is there value in keeping these as two commits?

@stevvooe
Copy link
Contributor Author

@vbatts I separated the implementation and the Makefile move.

@vbatts
Copy link
Member

vbatts commented Apr 29, 2016

LGTM

@vbatts vbatts merged commit b3ef20f into opencontainers:master Apr 29, 2016
@stevvooe stevvooe deleted the validate-examples branch April 29, 2016 03:15
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.

4 participants