Skip to content

Rewrite mounts description in config.md - #443

Merged
crosbymichael merged 1 commit into
opencontainers:masterfrom
hqhq:hq_config_new_line
Jul 25, 2016
Merged

Rewrite mounts description in config.md#443
crosbymichael merged 1 commit into
opencontainers:masterfrom
hqhq:hq_config_new_line

Conversation

@hqhq

@hqhqhqhq commented May 23, 2016

Copy link
Copy Markdown
Contributor

Also apply one sentence per line rule for list items.

Signed-off-by: Qiang Huang h.huangqiang@huawei.com

@wking

Copy link
Copy Markdown
Contributor

On Mon, May 23, 2016 at 12:06:27AM -0700, Qiang Huang wrote:

  1. Apple one sentence per line rule

“Apple” → “Apply”.

And I'm not sure there's consensus about whether the rule applies to
list items. I've been using single-sentence-lines in list items, but
unless there is a consensus it's probably not worth changing existing
instances unless you're also tweaking the content.

Comment threadconfig.md
* **`readonly`** (bool, optional) If true then the root filesystem MUST be read-only inside the container. Defaults to false.
* **`path`** (string, required) Specifies the path to the root filesystem for the container.
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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I liked “Defaults to false” better as its own sentence, but I expect folks will get the idea either way ;).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

And defaults are a general enough idea that it may make more sense to mark them up (e.g. “readonly (bool, default: false) If true, then …”). So settings would either be required or have a default value.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I liked “Defaults to false” better as its own sentence

I think it makes sense to combine them in one sentence, and it looks simpler, I'll change if more people think we should keep its own sentence.

And defaults are a general enough idea that it may make more sense to mark them up (e.g. “readonly (bool, default: false) If true, then …”). So settings would either be required or have a default value.

Since it's a general change for all such defaults usage in runtime-spec, I'll leave it as a separate discussion if needed.

@hqhq
hqhqforce-pushed the hq_config_new_line branch from 23fa592 to 1895e07CompareMay 31, 2016 11:27
@hqhqhqhq changed the title Some changes in config.mdRewrite mounts description in config.mdMay 31, 2016
@hqhq

hqhq commented May 31, 2016

Copy link
Copy Markdown
ContributorAuthor

Updated.

@wking

Copy link
Copy Markdown
Contributor

1895e07 looks good to me (with the exception of the default issue
1, and I'm fine spinning that out into a separate discussion 2).
I like your new, specific commit-summary a lot more too :).

Also apply one sentence per line rule for list items.
Signed-off-by: Qiang Huang <h.huangqiang@huawei.com>
@wking

Copy link
Copy Markdown
Contributor

On Tue, May 31, 2016 at 11:10:29AM -0700, W. Trevor King wrote:

1895e07 looks good to me (with the exception of the default issue
1, and I'm fine spinning that out into a separate discussion [2]).

Is there any chance of this landing soon? I want to extend the spec
to talk about the ‘bind’ type and ‘rbind’ options 1, neither of
which are covered by the mount(8) FILESYSTEM-* MOUNT OPTIONS sections.
They are covered as long options in mount(8), but I think we need
some wording around them (and maybe --make-shared, --make-slave,
--make-rshared, etc.?), and I don't want to workup a PR for that
before we've dropped the Arch wiki as a lower layer of this spec :p.

@hqhq
hqhqforce-pushed the hq_config_new_line branch from 1895e07 to 0e8e069CompareJuly 22, 2016 06:39
@hqhq

hqhq commented Jul 22, 2016

Copy link
Copy Markdown
ContributorAuthor

Rebased, ping @vbatts@mrunalp@crosbymichael

@wking

Copy link
Copy Markdown
Contributor

On Thu, Jul 21, 2016 at 11:40:59PM -0700, Qiang Huang wrote:

Rebased…

1895e070e8e069 looks fine to me. Besides the context changes,
there's some ‘:’ → ‘,’ and a sentence break that look minor to me.

@hqhq
hqhqforce-pushed the hq_config_new_line branch from 0e8e069 to 1752ce8CompareJuly 22, 2016 07:12
@hqhq

hqhq commented Jul 22, 2016

Copy link
Copy Markdown
ContributorAuthor

@wking I consisted some ':' and ',' usage.

@wking

Copy link
Copy Markdown
Contributor

On Fri, Jul 22, 2016 at 12:13:29AM -0700, Qiang Huang wrote:

@wking I consisted some ':' and ',' usage.

0e8e0691752ce8 looks good to me.

@vbatts

vbatts commented Jul 23, 2016

Copy link
Copy Markdown
Member

LGTM

Approved with PullApprove

1 similar comment
@crosbymichael

crosbymichael commented Jul 25, 2016

Copy link
Copy Markdown
Member

LGTM

Approved with PullApprove

@crosbymichael
crosbymichael merged commit eccaa08 into opencontainers:masterJul 25, 2016
@hqhq
hqhq deleted the hq_config_new_line branch July 26, 2016 01:24
Sign up for freeto 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

@hqhq@wking@vbatts@crosbymichael