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

generate: make AddBindMount() options a slice #265

Merged
merged 1 commit into from
Nov 7, 2016

Conversation

cyphar
Copy link
Member

@cyphar cyphar commented Nov 6, 2016

This allows for the set of options to be more useful than just "rw" or
"ro". It also makes it match the AddTmpfsMount far more.

We should also probably implement AddMount which just simply
adds a mount.

Signed-off-by: Aleksa Sarai asarai@suse.com

@wking
Copy link
Contributor

wking commented Nov 6, 2016

Through 878097f looks good to me.

On Sun, Nov 06, 2016 at 04:58:24AM -0800, Aleksa Sarai wrote:

We should also probably implement AddMount which just simply
adds a mount.

+1.

@@ -17,7 +17,7 @@ var generateFlags = []cli.Flag{
cli.StringFlag{Name: "apparmor", Usage: "specifies the the apparmor profile for the container"},
cli.StringFlag{Name: "arch", Value: runtime.GOARCH, Usage: "architecture the container is created for"},
cli.StringSliceFlag{Name: "args", Usage: "command to run in the container"},
cli.StringSliceFlag{Name: "bind", Usage: "bind mount directories src:dest:(rw,ro)"},
cli.StringSliceFlag{Name: "bind", Usage: "bind mount directories src:dest[:option1:option2:...]"},

Choose a reason for hiding this comment

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

How about change it to [:options]?
[***] just means this can be set multiple times.

Copy link
Member Author

Choose a reason for hiding this comment

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

From my experience with Unix commands, [something] means that its optional. [something...] means it can be set more than once.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @cyphar, and there are docs for this in http://man7.org/linux/man-pages/man7/man-pages.7.html

Choose a reason for hiding this comment

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

Well, I didn't find any rules about multiple options format in man7.
But from most of pages in man-pages, [:options] or [:options...] or [:options]... are all acceptable.

Copy link
Contributor

Choose a reason for hiding this comment

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

From man-pages(7):

Brackets ([]) surround optional arguments, vertical bars (|) separate choices, and ellipses (...) can be repeated.

default:
return source, dest, options, fmt.Errorf("--bind should have format src:dest:[options]")
return source, dest, options, fmt.Errorf("--bind should have format src:dest[:option1:option2:...]")

Choose a reason for hiding this comment

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

comment as above

@@ -29,15 +29,15 @@ read the configuration from `config.json`.

--args "/usr/bin/httpd" --args "-D" --args "FOREGROUND"

**--bind**=*[[HOST-DIR:CONTAINER-DIR][:OPTIONS]]*
**--bind**=*[[HOST-DIR:CONTAINER-DIR][:OPTIONS:...]]*

Choose a reason for hiding this comment

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

I think there is no need to add ...

Copy link
Member Author

Choose a reason for hiding this comment

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

I think most commands have the ... suffix if you can specify multiple of a type. But I'll change it.

This allows for the set of options to be more useful than just "rw" or
"ro". It also makes it match the AddTmpfsMount far more.

Signed-off-by: Aleksa Sarai <asarai@suse.com>
@cyphar cyphar force-pushed the generate-fix-Add_Mount branch from 878097f to 038b0c9 Compare November 7, 2016 03:43
@cyphar
Copy link
Member Author

cyphar commented Nov 7, 2016

@Mashimiao I changed to [:options...] and squashed my commits. PTAL.

@Mashimiao
Copy link

Mashimiao commented Nov 7, 2016

LGTM

Approved with PullApprove

@Mashimiao
Copy link

On 11/07/2016 12:36 PM, W. Trevor King wrote:

From man-pages(7):

Brackets (|[]|) surround optional arguments, vertical bars (|||) separate choices, and ellipses (|...|) can be repeated.

Oh, it really is, thanks for your information.

@mrunalp
Copy link
Contributor

mrunalp commented Nov 7, 2016

LGTM

Approved with PullApprove

@mrunalp mrunalp merged commit 49919f3 into opencontainers:master Nov 7, 2016
@cyphar cyphar deleted the generate-fix-Add_Mount branch November 8, 2016 10:57
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