format specs with 4 spaces indent #846

Mashimiao · 2017-05-23T07:33:29Z

Signed-off-by: Ma Shimiao mashimiao.fnst@cn.fujitsu.com

TomSweeneyRedHat · 2017-05-23T12:42:45Z

runtime.md

-This MUST be unique across all containers on this host.
-There is no requirement that it be unique across hosts.
+    This MUST be unique across all containers on this host.
+    There is no requirement that it be unique across hosts.
 * **`status`** (string, REQUIRED) is the runtime state of the container.


I'm not sure what's happening, but when I view this document the bullet for the status line is a line above the word "status'
o
status

I'm not sure if this will help, but I think the following “The value MAY be one of:” should be indented as well.

@TomSweeneyRedHat I'm not sure what's your browser. This may be that's the browser's problem. If you use another browser to view, does it look OK?

I don't think this is a browser thing. Looking at the rendered HTML:

$ curl -s https://github.com/Mashimiao/specs/blob/3d92dd2a8907af6fbabe47caf2f04e8cf628b34f/runtime.md | grep -A10 -B10 status | head -n21 </a><ul><a name="user-content-runtimeState"> <li> <code>ociVersion</code> (string, REQUIRED) is the OCI specification version used when creating the container. </li> <li> <code>id</code> (string, REQUIRED) is the container's ID. This MUST be unique across all containers on this host. There is no requirement that it be unique across hosts. </li> </a><li><a name="user-content-runtimeState"> <code>status</code> (string, REQUIRED) is the runtime state of the container. The value MAY be one of: </a><ul><a name="user-content-runtimeState"> </a><li><a name="user-content-runtimeState"><code>creating</code>: the container is being created (step 2 in the </a><a href="#lifecycle">lifecycle</a>)</li> <li><code>created</code>: the runtime has finished the <a href="#create">create operation</a> (after step 2 in the <a href="#lifecycle">lifecycle</a>), and the container process has neither exited nor executed the user-specified program</li> <li><code>running</code>: the container process has executed the user-specified program but has not exited (after step 5 in the <a href="#lifecycle">lifecycle</a>)</li> <li><code>stopped</code>: the container process has exited (step 7 in the <a href="#lifecycle">lifecycle</a>)</li> </ul> Additional values MAY be defined by the runtime, however, they MUST be used to represent new runtime states not defined above. </li> <li>

There are more user-content-runtimeState names than there should be, one of those <a> tags wraps the first two <li> entries, and only the status <li> entry gets some internal  tags. It seems like GitHub may be having issues with the partially-loose list, but according to their GFM spec (and this example), a single item with a blank line between two block-level elements should be enough to make the whole list loose.

So I think this is a GitHub rendering bug, and we should just accept the broken rendering until we can get GitHub's Markdown renderer patched. But it's probably worth washing this Markdown through Pandoc too to get a second opinion.

That may be a GitHub's bug. But different browsers have different views.
For example, chrome and firefox. By chrome, it looks OK

FWIW, I viewed this in Firefox and Chrome this morning. the issue did not show up in Chrome as you suspected. I'm fine with merging this as I agree it's a github/firefox rendering issue.

TomSweeneyRedHat · 2017-05-23T12:48:43Z

config.md

@@ -38,7 +38,8 @@ For example, if a configuration is compliant with version 1.1 of this specificat
    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`.


FWIW, I think these all should be sub-bullets.

o On Windows, ...
o On all other platforms, ...
o On Linux, ...
o If defined, ...

TomSweeneyRedHat · 2017-05-23T13:11:18Z

bundle.md

-This REQUIRED file MUST reside in the root of the bundle directory and MUST be named `config.json`.
-See [`config.json`](config.md) for more details.
+    This REQUIRED file MUST reside in the root of the bundle directory and MUST be named `config.json`.
+    See [`config.json`](config.md) for more details.

 2. <a name="containerFormat02" />A directory representing the root filesystem of the container.
    On Windows, for Windows Server containers, this directory is REQUIRED. For Hyper-V containers, it MUST be omitted.


I know it's not part of this review, but I think the "On Windows...", "For Hyper-V..." and "On all other ..." lines should be sub-bullets here.

I know it's not part of this review…

File a separate PR? Note that I have #838 and #840 in flight touching this requirement (although #838 doesn't touch this line). I'm happy to rebase if your bullet-addition lands first though.

wking · 2017-05-23T16:44:24Z

config-solaris.md

-Mapped to `allowed-address` in the [zonecfg(1M)][zonecfg.1m_2] man page.
+    If `allowedAddress` has not been specified, then they can use any IP address on the associated physical interface for the network resource.
+    Otherwise, when allowedAddress is specified, the container cannot use IP addresses that are not in the allowedAddress list for the physical address.
+    Mapped to `allowed-address` in the [zonecfg(1M)][zonecfg.1m_2] man page.
 * **`configureAllowedAddress`** *(string, OPTIONAL)* If configureAllowedAddress is set to true, the addresses specified by allowedAddress are automatically configured on the interface each time the container starts.


If you're backticking allowdAddress (like you do a few lines above), you probably want to get it (and configureAllowedAddress?) here too. Or we can punt some/all of the backtick fixup to a follow-up PR.

wking · 2017-05-23T16:46:07Z

config.md


    * **`type`** (string, REQUIRED) - the platform resource being limited, for example on Linux as defined in the [setrlimit(2)][setrlimit.2] man page.
    * **`soft`** (uint64, REQUIRED) - the value of the limit enforced for the corresponding resource.
-    * **`hard`** (uint64, REQUIRED) - the ceiling for the soft limit that could be set by an unprivileged process. Only a privileged process (e.g. under Linux: one with the CAP_SYS_RESOURCE capability) can raise a hard limit.
+    * **`hard`** (uint64, REQUIRED) - the ceiling for the soft limit that could be set by an unprivileged process.
+    Only a privileged process (e.g. under Linux: one with the CAP_SYS_RESOURCE capability) can raise a hard limit.


You want more indents here to place this line under the hard list entry.

Mashimiao · 2017-05-24T02:52:48Z

@wking @TomSweeneyRedHat all comments have been fixed.

wking · 2017-05-24T04:12:30Z

config.md


    If defined, a directory MUST exist at the path declared by the field.
-* **`readonly`** (bool, OPTIONAL) If true then the root filesystem MUST be read-only inside the container, defaults to false. On Windows, this field must be omitted or false.
+* **`readonly`** (bool, OPTIONAL) If true then the root filesystem MUST be read-only inside the container, defaults to false.
+    * On Windows, this field must be omitted or false.


This line will need at least a “must” → “MUST”, but this PR is already touching so much that I'd rather leave that fix to a follow-up PR.

Mashimiao · 2017-05-24T05:42:22Z

ping @opencontainers/runtime-spec-maintainers
If this PR looks OK, can you let it in as soon as possible?
As the PR modifies several files, if not, I think I have to rebase many times.

TomSweeneyRedHat · 2017-05-24T13:41:29Z

config-windows.md

-* **`utilityvmpath`** *(string, OPTIONAL)* - specifies the path to the image used for the utility VM. This would be specified if using a base image which does not contain a utility VM image. If not supplied, the runtime will search the container filesystem layers from the bottom-most layer upwards, until it locates "UtilityVM", and default to that path.
+* **`utilityvmpath`** *(string, OPTIONAL)* - specifies the path to the image used for the utility VM.
+    This would be specified if using a base image which does not contain a utility VM image.
+    If not supplied, the runtime will search the container filesystem layers from the bottom-most layer upwards, until it locates "UtilityVM", and default to that path.

 * **`sandboxpath`** *(string, REQUIRED)* - specifies the root of the path to the sandbox to be used for the container.


Looks like https://github.com/opencontainers/runtime-spec/pull/849/files is getting ready to remove this, you might have some merge fun between the two PR's.

TomSweeneyRedHat

LGTM, thanks for the touch ups.

vbatts · 2017-05-31T21:40:52Z

needs a rebase, but LGTM

Signed-off-by: Ma Shimiao <mashimiao.fnst@cn.fujitsu.com>

Mashimiao · 2017-06-01T02:10:17Z

@vbatts @mrunalp rebased

vbatts · 2017-06-01T12:49:10Z

LGTM

tianon · 2017-06-01T14:56:39Z

LGTM

The Markdown spec is liberal for the first paragraph [1]: > To make lists look nice, you can wrap items with hanging > indents... But if you want to be lazy, you don’t have to... However, it's a bit more strict about subsequent paragraphs [1]: > List items may consist of multiple paragraphs. Each subsequent > paragraph in a list item must be indented by either 4 spaces or one > tab: That doesn't matter for our use here, because all of our entries are single-paragraph. But runtime-spec has been bitten by Pandoc strictness for multiple paragraphs before [2], and their RELEASES.md has used four-space indents since [3]. By adopting the stricter behavior here, we make it easier for OCI Projects to stay synchronized with the template while maintaining their stricter local conventions. OCI Projects that do not have strict local conventions probably don't care either way. [1]: https://daringfireball.net/projects/markdown/syntax#list [2]: opencontainers/runtime-spec#495 [3]: opencontainers/runtime-spec#846 Signed-off-by: W. Trevor King <wking@tremily.us>

Mashimiao force-pushed the format-specs branch from 865ec3c to b6b3a2e Compare May 23, 2017 07:39

TomSweeneyRedHat reviewed May 23, 2017

View reviewed changes

wking reviewed May 23, 2017

View reviewed changes

Mashimiao force-pushed the format-specs branch from b6b3a2e to 3d92dd2 Compare May 24, 2017 01:22

wking reviewed May 24, 2017

View reviewed changes

Mashimiao force-pushed the format-specs branch 2 times, most recently from 746b721 to b4f31ee Compare May 24, 2017 05:33

TomSweeneyRedHat reviewed May 24, 2017

View reviewed changes

wking mentioned this pull request May 24, 2017

Mount correction #854

Merged

Mashimiao force-pushed the format-specs branch from b4f31ee to c41bbe6 Compare May 26, 2017 01:23

Ma Shimiao added 2 commits June 1, 2017 10:08

format specs with 4 spaces indent

137b9c3

Signed-off-by: Ma Shimiao <mashimiao.fnst@cn.fujitsu.com>

replace must with MUST

01c0762

Signed-off-by: Ma Shimiao <mashimiao.fnst@cn.fujitsu.com>

Mashimiao force-pushed the format-specs branch from c41bbe6 to 01c0762 Compare June 1, 2017 02:09

tianon merged commit f79b61d into opencontainers:master Jun 1, 2017

Mashimiao mentioned this pull request Jun 1, 2017

consistency and style fix #811

Merged

wking mentioned this pull request Jun 1, 2017

bundle: DRYer root.path entry #840

Merged

vbatts mentioned this pull request Jul 5, 2017

bump to v1.0.0-rc6 #882

Merged

wking mentioned this pull request Sep 11, 2017

Add vTPM specification #920

Closed

wking mentioned this pull request Apr 6, 2018

*: Use four-space indents for list-entry continuation opencontainers/project-template#54

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

format specs with 4 spaces indent #846

format specs with 4 spaces indent #846

Mashimiao commented May 23, 2017

TomSweeneyRedHat May 23, 2017

wking May 23, 2017

Mashimiao May 24, 2017

wking May 24, 2017

Mashimiao May 24, 2017

TomSweeneyRedHat May 24, 2017

TomSweeneyRedHat May 23, 2017

TomSweeneyRedHat May 23, 2017

wking May 23, 2017

wking May 23, 2017

wking May 23, 2017

Mashimiao commented May 24, 2017

wking May 24, 2017

Mashimiao commented May 24, 2017 •

edited

Loading

TomSweeneyRedHat May 24, 2017

TomSweeneyRedHat left a comment

vbatts commented May 31, 2017

Mashimiao commented Jun 1, 2017

vbatts commented Jun 1, 2017 •

edited by caniszczyk

Loading

tianon commented Jun 1, 2017 •

edited by caniszczyk

Loading

		@@ -38,7 +38,8 @@ For example, if a configuration is compliant with version 1.1 of this specificat
		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`.

format specs with 4 spaces indent #846

format specs with 4 spaces indent #846

Conversation

Mashimiao commented May 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mashimiao commented May 24, 2017

Choose a reason for hiding this comment

Mashimiao commented May 24, 2017 • edited Loading

Choose a reason for hiding this comment

TomSweeneyRedHat left a comment

Choose a reason for hiding this comment

vbatts commented May 31, 2017

Mashimiao commented Jun 1, 2017

vbatts commented Jun 1, 2017 • edited by caniszczyk Loading

tianon commented Jun 1, 2017 • edited by caniszczyk Loading

Mashimiao commented May 24, 2017 •

edited

Loading

vbatts commented Jun 1, 2017 •

edited by caniszczyk

Loading

tianon commented Jun 1, 2017 •

edited by caniszczyk

Loading