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

Rewrite mounts description in config.md #443

Merged
merged 1 commit into from
Jul 25, 2016

Conversation

hqhq
Copy link
Contributor

@hqhq hqhq commented May 23, 2016

Also apply one sentence per line rule for list items.

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

@wking
Copy link
Contributor

wking commented May 23, 2016

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.

* **`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
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
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
Contributor Author

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 hqhq changed the title Some changes in config.md Rewrite mounts description in config.md May 31, 2016
@hqhq
Copy link
Contributor Author

hqhq commented May 31, 2016

Updated.

@wking
Copy link
Contributor

wking commented May 31, 2016

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
Contributor

wking commented Jul 22, 2016

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
Copy link
Contributor Author

hqhq commented Jul 22, 2016

Rebased, ping @vbatts @mrunalp @crosbymichael

@wking
Copy link
Contributor

wking commented Jul 22, 2016

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
Copy link
Contributor Author

hqhq commented Jul 22, 2016

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

@wking
Copy link
Contributor

wking commented Jul 22, 2016

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
Copy link
Member

vbatts commented Jul 23, 2016

LGTM

Approved with PullApprove

1 similar comment
@crosbymichael
Copy link
Member

crosbymichael commented Jul 25, 2016

LGTM

Approved with PullApprove

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