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

source and schema: differentiate with examples #276

Merged
merged 2 commits into from
Mar 9, 2016

Conversation

vbatts
Copy link
Member

@vbatts vbatts commented Dec 18, 2015

The standard is on the JSON schema (not yet IETF spec JSON-schema), such
that it is not implemenations specific. Thus far, the reference has been
in how golang source renders the JSON documents.

Having the JSON source and the markdown documents in sync has been an
ongoing step to keep in sync.

Separating these two allows the golang source to continue being a
reference, but the JSON schema in the documentation to be the
reference.

As validation tooling is refined, then it will facilitate ensuring
the available golang source conforms to the reference JSON.

@@ -127,4 +127,77 @@ For Linux-based systems the user structure has the following fields:
Interpretation of the platform section of the JSON file is used to find which platform-specific sections may be available in the document.
For example, if `os` is set to `linux`, then a JSON object conforming to the [Linux-specific schema](config-linux.md) SHOULD be found at the key `linux` in the `config.json`.

## Configuration Schema Example

Here is a full exmample `config.json` contents for reference (generated with `runc spec`).
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: “exmample” → “example”, and I'd drop “contents” and “(generated with runc spec)”. The generation note may be useful for a commit message, but doesn't seem useful in the spec itself.

@wking
Copy link
Contributor

wking commented Dec 18, 2015

On Fri, Dec 18, 2015 at 01:50:31PM -0800, Vincent Batts wrote:

Separating these two allows the golang source to continue being a
reference, but the JSON schema in the documentation to be the
reference.

I'm not sure these examples have much normative weight, but they
certainly are useful for showing how the normative sections (e.g. 1)
fit together. And it would let runC and similar drop the examples
they currently embed 2 by providing a centrally-managed example.
So +1 on this going in (although I've left a few inline comments about
the example content).

@vishh
Copy link
Contributor

vishh commented Dec 18, 2015

+1 for the overall intent of separating schema from the golang structures.

@crosbymichael
Copy link
Member

The only downside is that it makes the Go import path ugly.

@wking
Copy link
Contributor

wking commented Dec 18, 2015

On Fri, Dec 18, 2015 at 02:52:59PM -0800, Michael Crosby wrote:

The only downside is that it makes the Go import path ugly.

We expect Go import paths to get more interesting soon anyway, no?
@mrunalp's platform-directory revival 1 seemed to go over well in
the 2015-12-16 meeting. I don't remember hearing any pushback other
than @duglin's concern that it was adding platform-dependent
compilation instead of removing it 2, and once we cleared that up I
think everyone was in favor of it 3.

@vishh
Copy link
Contributor

vishh commented Dec 19, 2015

@vbatts: How do we differentiate between optional and required fields? And how will this address extensible fields?

@wking
Copy link
Contributor

wking commented Dec 19, 2015

On Fri, Dec 18, 2015 at 05:46:21PM -0800, Vish Kannan wrote:

How do we differentiate between optional and required fields? And
how will this address extensible fields?

The normative specification will still be things like 1. This is
just a global example to help tie all of that together.

@vbatts
Copy link
Member Author

vbatts commented Dec 21, 2015

@vishh that has to be done per field and value. Like the comments that describe them.

* **`path`** (string, required) ...
* **`readonly`** (bool, optional) ...

It's not very parsable, but is legible.

@vbatts
Copy link
Member Author

vbatts commented Dec 21, 2015

@crosbymichael you thinking those paths would be ugly was the first thing that came to mind. What do you recommend besides github.com/opencontainers/specs/src/specs ?

@vbatts
Copy link
Member Author

vbatts commented Dec 21, 2015

otherwise, I've rebased and updated the structs a little based on comments.
PTAL

@wking
Copy link
Contributor

wking commented Dec 21, 2015

On Mon, Dec 21, 2015 at 12:24:27PM -0800, Vincent Batts wrote:

otherwise, I've rebased and updated the structs a little based on comments.

All the ecccada99d1df7 changes look good to me, but this is still
duplicating fields required by #164 1. And there are still a number
of nulls in the resources that can be left out now that #233 has
landed a pointer-based approach to “don't touch this”.

@vbatts
Copy link
Member Author

vbatts commented Jan 13, 2016

rebased. PTAL

@wking
Copy link
Contributor

wking commented Jan 13, 2016

On Wed, Jan 13, 2016 at 01:22:20PM -0800, Vincent Batts wrote:

rebased. PTAL

$ diff -u <(git show -M50 99d1df7) <(git show -M50 998344)
--- /dev/fd/63 2016-01-13 13:33:17.048080367 -0800
+++ /dev/fd/62 2016-01-13 13:33:17.048080367 -0800
@@ -1,4 +1,4 @@
-commit 99d1df7
+commit 9983445
Author: Vincent Batts [email protected]
Date: Fri Dec 18 15:47:21 2015 -0500

So that is just “rebased onto the new master to pickup changes in the
renamed files”. I still see the nulls I suggest we remove 1.

@mrunalp
Copy link
Contributor

mrunalp commented Jan 13, 2016

We can do this using ocitools to generate and a Makefile target to update this.

@wking
Copy link
Contributor

wking commented Jan 13, 2016

On Wed, Jan 13, 2016 at 02:43:21PM -0800, Mrunal Patel wrote:

We can do this using ocitools to generate and a Makefile target to
update this.

I'm concerned that:

  1. Write normative Markdown (this repo)
  2. Update informative Go types (this repo)
  3. Update ocitools to pull in new Go types
  4. Generate new example JSON with ocitools
  5. Update these informative examples (this repo)

is too long a loop, involving a number of non-normative moving parts.
It also means that without some ocitools hoop-jumping, you'd have to
land a single semantic update in two PRs (one PR for (1) and (2), and
another PR for (5)).

But if the idea is that PR submitters will use whatever tooling they
want (including a Make-spawned ocitools or hand-editing) to update
this section, then +1 to anything folks think makes that easier.

@vbatts
Copy link
Member Author

vbatts commented Jan 19, 2016

rebased

@vbatts
Copy link
Member Author

vbatts commented Jan 20, 2016

brainstorming: would the sample json be better served as an config.example.json file sitting in the directory, rather than appended to the *.md?

"CAP_NET_BIND_SERVICE"
],
"apparmorProfile": "",
"selinuxProcessLabel": ""
Copy link
Contributor

Choose a reason for hiding this comment

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

Before these moved @mrunalp and I were suggesting we drop the empty values. Since #329 they're omitempty, so I see no reason to keep them in this example.

Copy link
Member

Choose a reason for hiding this comment

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

Its fine to have these in an example because how else would ppl know that they are there without them being in an EXAMPLE

Copy link
Contributor

Choose a reason for hiding this comment

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

On Tue, Mar 08, 2016 at 10:43:21AM -0800, Michael Crosby wrote:

  •   "apparmorProfile": "",
    
  •   "selinuxProcessLabel": ""
    

Its fine to have these in an example because how else would ppl know
that they are there without them being in an EXAMPLE

I'm fine listing them here with meaningful content. And regardless of
whether they're listed here, I think people should be able to discover
them via the process documentation [1](which seems to use
‘selinuxLabel’ and not ‘selinuxProcessLabel’ anyway).

@vbatts
Copy link
Member Author

vbatts commented Mar 9, 2016

rebased

@mrunalp
Copy link
Contributor

mrunalp commented Mar 9, 2016

Should we format the json using jq to get a consistent output?

@crosbymichael
Copy link
Member

import "github.com/opencontainers/specs/spec-go"

@vbatts
Copy link
Member Author

vbatts commented Mar 9, 2016

@crosbymichael done

The standard is on the JSON schema (not yet IETF spec JSON-schema), such
that it is not implemenations specific. Thus far, the reference has been
in how golang source renders the JSON documents.

Having the JSON source and the markdown documents in sync has been an
ongoing step to keep in sync.

Separating these two allows the golang source to continue being _a_
reference, but the JSON schema in the documentation to be _the_
reference.

As validation tooling is refined, then it will facilitate ensuring
the available golang source conforms to the reference JSON.

Signed-off-by: Vincent Batts <[email protected]>
@vbatts
Copy link
Member Author

vbatts commented Mar 9, 2016

rebased again

@crosbymichael
Copy link
Member

LGTM

"hooks": {
"prestart": [
{
"path": "/",
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be /usr/bin/uptime
args[0] could match that or be something else or just empty.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

Signed-off-by: Vincent Batts <[email protected]>
@mrunalp
Copy link
Contributor

mrunalp commented Mar 9, 2016

LGTM

mrunalp pushed a commit that referenced this pull request Mar 9, 2016
source and schema: differentiate with examples
@mrunalp mrunalp merged commit fae9a3e into opencontainers:master Mar 9, 2016
wking added a commit to wking/opencontainer-runtime-spec that referenced this pull request Mar 9, 2016
The label changed in 5a8a779 (Move process specific settings to
process, 2016-03-02, opencontainers#329) and 7bf06d5 (source and schema:
differentiate with examples, 2015-12-18, opencontainers#276) missed this instance
when rebasing around opencontainers#329.

Signed-off-by: W. Trevor King <[email protected]>
wking added a commit to wking/opencontainer-runtime-spec that referenced this pull request Mar 10, 2016
Also:

* Update the link to Go bindings after 7bf06d5 (source and schema:
  differentiate with examples, 2015-12-18, opencontainers#276).
* Add a reference to the JSON Schema after cdcabde (schema: JSON
  Schema and validator for `config.json`, 2016-01-19, opencontainers#313).

It's pretty clear that the Go bindings cannot be canonical on their
own, because they do not define limits (e.g. the 0 through 512 range
for FileMode).  The JSON Schema is closer, but still does not cover
everything (e.g. "a directory must exist at root.path").  Both the Go
bindings and the JSON Schema could grow to cover the full spec by
adding that sort of thing to comments and descriptions, but that's not
how things seem to be working now.

Signed-off-by: W. Trevor King <[email protected]>
wking added a commit to wking/opencontainer-runtime-spec that referenced this pull request Mar 10, 2016
Catch up with 7bf06d5 (source and schema: differentiate with examples,
2015-12-18, opencontainers#276).

Signed-off-by: W. Trevor King <[email protected]>
wking added a commit to wking/opencontainer-runtime-spec that referenced this pull request Mar 10, 2016
Also:

* Update the link to Go bindings after 7bf06d5 (source and schema:
  differentiate with examples, 2015-12-18, opencontainers#276).
* Add a reference to the JSON Schema after cdcabde (schema: JSON
  Schema and validator for `config.json`, 2016-01-19, opencontainers#313).

It's pretty clear that the Go bindings cannot be canonical on their
own, because they do not define limits (e.g. the 0 through 512 range
for FileMode).  The JSON Schema is closer, but still does not cover
everything (e.g. "a directory must exist at root.path").  Both the Go
bindings and the JSON Schema could grow to cover the full spec by
adding that sort of thing to comments and descriptions, but that's not
how things seem to be working now.

Signed-off-by: W. Trevor King <[email protected]>
wking added a commit to wking/opencontainer-runtime-spec that referenced this pull request Mar 10, 2016
Catch up with 7bf06d5 (source and schema: differentiate with examples,
2015-12-18, opencontainers#276).

Signed-off-by: W. Trevor King <[email protected]>
@vbatts vbatts deleted the schema branch April 12, 2016 03:51
Mashimiao pushed a commit to Mashimiao/specs that referenced this pull request Aug 19, 2016
The label changed in 5a8a779 (Move process specific settings to
process, 2016-03-02, opencontainers#329) and 7bf06d5 (source and schema:
differentiate with examples, 2015-12-18, opencontainers#276) missed this instance
when rebasing around opencontainers#329.

Signed-off-by: W. Trevor King <[email protected]>
Mashimiao pushed a commit to Mashimiao/specs that referenced this pull request Aug 19, 2016
Also:

* Update the link to Go bindings after 7bf06d5 (source and schema:
  differentiate with examples, 2015-12-18, opencontainers#276).
* Add a reference to the JSON Schema after cdcabde (schema: JSON
  Schema and validator for `config.json`, 2016-01-19, opencontainers#313).

It's pretty clear that the Go bindings cannot be canonical on their
own, because they do not define limits (e.g. the 0 through 512 range
for FileMode).  The JSON Schema is closer, but still does not cover
everything (e.g. "a directory must exist at root.path").  Both the Go
bindings and the JSON Schema could grow to cover the full spec by
adding that sort of thing to comments and descriptions, but that's not
how things seem to be working now.

Signed-off-by: W. Trevor King <[email protected]>
Mashimiao pushed a commit to Mashimiao/specs that referenced this pull request Aug 19, 2016
Catch up with 7bf06d5 (source and schema: differentiate with examples,
2015-12-18, opencontainers#276).

Signed-off-by: W. Trevor King <[email protected]>
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