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

config.md: clarify the root filesystem path #558

Merged

Conversation

Mashimiao
Copy link

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

@@ -25,6 +25,8 @@ For example, if an implementation is compliant with version 1.0.1 of the spec, i
**`root`** (object, required) configures the container's root filesystem.

* **`path`** (string, required) Specifies the path to the root filesystem for the container.
The path MUST be an a relative path (not starting with /), which is relative to the bundle. e.g.
Copy link
Contributor

Choose a reason for hiding this comment

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

We used to have a relative path restriction, but it was (wisely, I think) lifted in #394. There is in-flight work in #469 to finish off the work that #394 started, but I don't think we want to go back to requiring relative paths here.

Copy link
Contributor

Choose a reason for hiding this comment

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

However, I think it does make sense to clarify the base from which relative paths are interpreted, in cases where someone does use a relative path here. I'd recommend doing that with a stand-alone section early in this file, so other relative-path locations can benefit from it. That would allow us to relax some absolute-path requirements we're still carrying around (e.g. for namespace paths).

Copy link
Author

Choose a reason for hiding this comment

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

Acutally, I think no matter absolute path or relative path is OK.
I just want to clarify which kind of value can be set to path.

@Mashimiao Mashimiao force-pushed the config-clarify-root-filesystem-path branch from 3f1763f to b7f7912 Compare September 8, 2016 04:00
@@ -25,6 +25,8 @@ For example, if an implementation is compliant with version 1.0.1 of the spec, i
**`root`** (object, required) configures the container's root filesystem.

* **`path`** (string, required) Specifies the path to the root filesystem for the container.
The path can be an absolute path (starting with /) or a relative path (not starting with /), which is relative to the bundle. e.g.
If the absolute path of root filesystem is /to/bundle/rootfs, we can set it or rootfs to`path.
Copy link
Member

Choose a reason for hiding this comment

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

looks like a dangling backtick `
and i'm not sure i follow the "we can set it or rootfs to path" sentence.

Copy link
Contributor

Choose a reason for hiding this comment

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

On Thu, Sep 08, 2016 at 07:00:35AM -0700, Vincent Batts wrote:

@@ -25,6 +25,8 @@ For example, if an implementation is compliant with version 1.0.1 of the spec, i
root (object, required) configures the container's root filesystem.

  • path (string, required) Specifies the path to the root filesystem for the container.
    • The path can be an absolute path (starting with /) or a relative path (not starting with /), which is relative to the bundle. e.g.
    • If the absolute path of root filesystem is /to/bundle/rootfs, we can set it or rootfs to`path.

looks like a dangling backtick `
and i'm not sure i follow the "we can set it or rootfs to path" sentence.

How about:

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.

Copy link
Author

Choose a reason for hiding this comment

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

On 09/09/2016 12:39 AM, W. Trevor King wrote:

How about: 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.
It looks better than mine. Fixed.

@Mashimiao Mashimiao force-pushed the config-clarify-root-filesystem-path branch from b7f7912 to 3a8696d Compare September 9, 2016 00:55
@@ -25,6 +25,8 @@ For example, if an implementation is compliant with version 1.0.1 of the spec, i
**`root`** (object, required) configures the container's root filesystem.

* **`path`** (string, required) Specifies the path to the root filesystem for the container.
The path can be an absolute path (starting with /) or a relative path (not starting with /), which is relative to the bundle.
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`.
Copy link
Contributor

Choose a reason for hiding this comment

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

The example should be explicit for *unix since other platforms like windows don't take / as root.

Signed-off-by: Ma Shimiao <mashimiao.fnst@cn.fujitsu.com>
@Mashimiao Mashimiao force-pushed the config-clarify-root-filesystem-path branch from 3a8696d to 61e2a60 Compare September 9, 2016 06:02
@Mashimiao
Copy link
Author

any comments?

@hqhq
Copy link
Contributor

hqhq commented Sep 12, 2016

LGTM

Approved with PullApprove

@vbatts
Copy link
Member

vbatts commented Sep 13, 2016

LGTM

Approved with PullApprove

@vbatts vbatts merged commit b3ce195 into opencontainers:master Sep 13, 2016
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