Clarify root path #820

lowenna · 2017-05-13T01:36:31Z

Signed-off-by: John Howard jhoward@microsoft.com

This changes the root path to OPTIONAL as it is not supported for Hyper-V containers on Windows, where the root filesystem isn't accessible on the host operating system - it only has meaning inside the utility VM running the container.

lowenna · 2017-05-13T03:35:21Z

Not sure what travis is complaining about.... can someone help me understand? 😬

wking · 2017-05-13T05:34:42Z

config.md

  The path is either an absolute path or a relative path to the bundle.
-  On Linux, for example, with a bundle at `/to/bundle` and a root filesystem at `/to/bundle/rootfs`, the `path` value can be either `/to/bundle/rootfs` or `rootfs`.
-  A directory MUST exist at the path declared by the field.
+  On Linux, for example, with a bundle at `/to/bundle` and a root filesystem at `/to/bundle/rootfs`, the `path` value can be either `/to/bundle/rootfs` or `rootfs`. 


Travis doesn't like the trailing whitespace here:

$ git show --oneline --check origin/pr/820 23a0b15 Clarify root path config.md:33: trailing whitespace. + On Linux, for example, with a bundle at `/to/bundle` and a root filesystem at `/to/bundle/rootfs`, the `path` value can be either `/to/bundle/rootfs` or `rootfs`.

Thanks! Fixed in next push

wking · 2017-05-13T05:44:04Z

config.md

-  On Linux, for example, with a bundle at `/to/bundle` and a root filesystem at `/to/bundle/rootfs`, the `path` value can be either `/to/bundle/rootfs` or `rootfs`.
-  A directory MUST exist at the path declared by the field.
+  On Linux, for example, with a bundle at `/to/bundle` and a root filesystem at `/to/bundle/rootfs`, the `path` value can be either `/to/bundle/rootfs` or `rootfs`. 
+  On Windows, for Windows Server Containers, this field is REQUIRED. For Hyper-V Containers, this field MUST NOT be supplied.


For writing validation tooling, how should runtime-tools distinguish between a Windows Server config and a Hyper-V config?

This change will also need some knock ons:

Adjusting the bundle requirements to only require the root filesystem directory when root.path is set. I still don't see a need for that bundle requirement (last discussed in bundle: Remove rootfs requirement #469) and think that the config.md requirement (which you preserve in this PR) for “A directory MUST exist at the path declared by the field” is sufficient for sanity. But if you can't get consensus around something like that, you'll still need to do something to bundle.md, since the current wording there makes it impossible to leave root.path unset.

An omitempty in the Go type.

Dropping the path requirement from the JSON Schema.

For writing validation tooling, how should runtime-tools distinguish between a Windows Server config and a Hyper-V config?

See the associated PR in #818

Have updated the bundle requirements, the go type and the schema

wking · 2017-05-15T19:00:39Z

bundle.md

+
+On Windows, for Windows Server containers, this directory is REQUIRED. For Hyper-V containers, it MUST be omitted.
+
+If set, this directory MUST be referenced by [`root`](config.md#root) within the `config.json` file.


I think these are all part of the second list entry. To get multi-paragraph list entries in Pandoc, you need four-space (or tab) indents (more in #495). Can you use:

2. <a name="containerFormat02" />A directory representing the root filesystem of the container. While the name of this directory may be arbitrary, users should consider using a conventional name, such as `rootfs`. On non-Windows platforms, this directory is REQUIRED. On Windows, for Windows Server containers, this directory is REQUIRED. For Hyper-V containers, it MUST be omitted. If set, this directory MUST be referenced by [`root`](config.md#root) within the `config.json` file.

Indeed. Updated.

wking · 2017-05-15T20:25:23Z

bundle.md

+
+    If set, this directory MUST be referenced by [`root`](config.md#root) within the `config.json` file.
+
+When supplied, while these artifacts MUST all be present in a single directory on the local filesystem, that directory itself is not part of the bundle. In other words, a tar archive of a *bundle* will have these artifacts at the root of the archive, not nested within a top-level directory.


You seem to be removing the newline before “In other words” while not touching the content of that sentence. I think you want to keep that sentence on its own line.

Also, “When supplied, while …” is a bit awkward. I keep circling back to #469 when trying to think up clearer phrasing, so I don't have anything new to propose as an alternative. But probably something that separates out the same-directory MUST from the definition of “bundle” as the parent of config.json (and any other artifacts listed here).

Signed-off-by: John Howard <jhoward@microsoft.com>

lowenna · 2017-05-15T20:33:49Z

You seem to be removing the newline before “In other words” while not touching the content of that sentence. I think you want to keep that sentence on its own line.

Hmmm. Yes, I see from the link. But from a grammatical perspective, it seems more natural to me as it directly relates to the previous sentence. Reverted though.

wking · 2017-05-15T20:40:27Z

On Mon, May 15, 2017 at 01:33:50PM -0700, John Howard wrote: But from a grammatical perspective, it seems more natural to me as it directly relates to the previous sentence.

The rendered page will include both lines in the same paragraph, since Markdown needs a blank line (e.g. two consecutive newlines, not just one) to split paragraphs [1]. The one-sentence-per-line policy is mostly for convenient diffing (e.g. ‘git blame …’ will continue to show the second line as last touched by 106ec2d, #446). [1]: https://daringfireball.net/projects/markdown/syntax#p

tianon · 2017-05-17T23:47:51Z

LGTM

vbatts · 2017-05-18T18:16:11Z

LGTM

Through c83b8c8 (Merge pull request opencontainers#820 from jhowardms\ ft/clarifyrootpath, 2017-05-18). Signed-off-by: W. Trevor King <wking@tremily.us>

Through f4d221c (Merge pull request opencontainers#880 from dqminh/wking-linux-only-capabilities-again, 2017-07-05). The rc6 release picked up an earlier version of these notes, and those entries are mostly unchanged except for: * The credentialSpec entry, which was opencontainers#814 for credentialspec and now also includes opencontainers#859 for credentialSpec. * The root(.path) Hyper-V entry, which was opencontainers#820 for root.path and now also includes opencontainers#838 for root. I also moved this into the "breaking changes" section, because rc5 Hyper-V configs required root to be set, and rc6 Hyper-V configs require it to not be set. Although whether rc5 allowed Hyper-V configs at all is not clear to me. * Fixed indenting for the typo-fixes entry, as well as a number of more recent typo-fix PRs. Signed-off-by: W. Trevor King <wking@tremily.us>

lowenna force-pushed the clarifyrootpath branch from 1e79718 to 23a0b15 Compare May 13, 2017 01:37

wking reviewed May 13, 2017

View reviewed changes

lowenna force-pushed the clarifyrootpath branch from 23a0b15 to 115b84d Compare May 15, 2017 18:55

wking reviewed May 15, 2017

View reviewed changes

lowenna force-pushed the clarifyrootpath branch 3 times, most recently from 393d49d to 1cbf3e3 Compare May 15, 2017 20:17

wking reviewed May 15, 2017

View reviewed changes

Clarify root path

7c9acf6

Signed-off-by: John Howard <jhoward@microsoft.com>

lowenna force-pushed the clarifyrootpath branch from 1cbf3e3 to 7c9acf6 Compare May 15, 2017 20:33

lowenna mentioned this pull request May 15, 2017

Windows targets for 1.0? #817

Closed

vbatts added the platforms/windows label May 17, 2017

vbatts merged commit c83b8c8 into opencontainers:master May 18, 2017

wking mentioned this pull request May 18, 2017

config: Bump Hyper-V condition from root.path to root itself #838

Merged

vbatts mentioned this pull request May 19, 2017

Bump version (rc6 and back to dev) #842

Closed

vbatts mentioned this pull request Jul 5, 2017

bump to v1.0.0-rc6 #882

Merged

wking mentioned this pull request Jul 10, 2017

ChangeLog: Document changes since c83b8c8 #888

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clarify root path #820

Clarify root path #820

lowenna commented May 13, 2017

lowenna commented May 13, 2017

wking May 13, 2017

lowenna May 15, 2017

wking May 13, 2017

lowenna May 15, 2017

wking May 15, 2017

lowenna May 15, 2017

wking May 15, 2017

lowenna commented May 15, 2017

wking commented May 15, 2017 via email

tianon commented May 17, 2017 •

edited by caniszczyk

Loading

vbatts commented May 18, 2017 •

edited by caniszczyk

Loading


		On Windows, for Windows Server containers, this directory is REQUIRED. For Hyper-V containers, it MUST be omitted.

		If set, this directory MUST be referenced by [`root`](config.md#root) within the `config.json` file.


		If set, this directory MUST be referenced by [`root`](config.md#root) within the `config.json` file.

		When supplied, while these artifacts MUST all be present in a single directory on the local filesystem, that directory itself is not part of the bundle. In other words, a tar archive of a bundle will have these artifacts at the root of the archive, not nested within a top-level directory.

Clarify root path #820

Clarify root path #820

Conversation

lowenna commented May 13, 2017

lowenna commented May 13, 2017

wking May 13, 2017

Choose a reason for hiding this comment

lowenna May 15, 2017

Choose a reason for hiding this comment

wking May 13, 2017

Choose a reason for hiding this comment

lowenna May 15, 2017

Choose a reason for hiding this comment

wking May 15, 2017

Choose a reason for hiding this comment

lowenna May 15, 2017

Choose a reason for hiding this comment

wking May 15, 2017

Choose a reason for hiding this comment

lowenna commented May 15, 2017

wking commented May 15, 2017 via email

tianon commented May 17, 2017 • edited by caniszczyk Loading

vbatts commented May 18, 2017 • edited by caniszczyk Loading

tianon commented May 17, 2017 •

edited by caniszczyk

Loading

vbatts commented May 18, 2017 •

edited by caniszczyk

Loading