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: Replace "SHOULD consider" for 'rootfs' path value #847

Merged
merged 1 commit into from
Jun 1, 2017

Conversation

wking
Copy link
Contributor

@wking wking commented May 23, 2017

SHOULD means:

… there may exist valid reasons in particular circumstances to ignore a particular item, but the full implications must be understood and carefully weighed before choosing a different course.

So consideration is already baked into the definition, and we can just say “SHOULD be” and warn folks whenever they use a value other than rootfs.

Also break this into its own paragraph, because the value recommendation is orthogonal to and less important than the value semantics covered in the preceding sentences.

Copy link

@TomSweeneyRedHat TomSweeneyRedHat left a comment

Choose a reason for hiding this comment

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

LGTM

@hqhq
Copy link
Contributor

hqhq commented May 26, 2017

LGTM

Approved with PullApprove

config.md Outdated
@@ -29,7 +29,8 @@ For example, if a configuration is compliant with version 1.1 of this specificat
**`root`** (object, REQUIRED) specifies the container's root filesystem.

* **`path`** (string, OPTIONAL) Specifies the path to the root filesystem for the container. The path is either an absolute path or a relative path to the bundle.
Users SHOULD consider using a conventional name, such as `rootfs`.

The value SHOULD be the conventional `rootfs`.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Neither the old nor the new wording applies to Windows (since #849, because rootfs is not a GUID path). I address that in the in-flight #838, and am leaving it alone here since it seems orthogonal to dropping “consider”. If folks do want me to address the “does not apply to Windows” issue in this PR as well, I'm happy to pull over some of the #838 wording to do that.

SHOULD means that [1]:

  ... there may exist valid reasons in particular circumstances to
  ignore a particular item, but the full implications must be
  understood and carefully weighed before choosing a different course.

So consideration is already baked into the definition, and we can just
say "SHOULD be" and warn folks whenever they use a value other than
'rootfs'.

Also move this under the non-Windows conditions, because the advice
they doesn't apply to Windows where the value MUST be a volume GUID
path (since 2283e63, Windows: Remove Sandbox, additional tweaks,
2017-05-23, opencontainers#849).

[1]: https://tools.ietf.org/html/rfc2119#section-3

Signed-off-by: W. Trevor King <wking@tremily.us>
@wking wking force-pushed the no-should-consider branch from 5d273c0 to 7de3cb8 Compare June 1, 2017 15:18
@wking
Copy link
Contributor Author

wking commented Jun 1, 2017

Rebased around #846 with 5d273c07de3cb8.

@crosbymichael
Copy link
Member

crosbymichael commented Jun 1, 2017

LGTM

Approved with PullApprove

1 similar comment
@mrunalp
Copy link
Contributor

mrunalp commented Jun 1, 2017

LGTM

Approved with PullApprove

@mrunalp mrunalp merged commit 69380b0 into opencontainers:master Jun 1, 2017
@wking wking deleted the no-should-consider branch June 2, 2017 04:18
@vbatts vbatts mentioned this pull request Jul 5, 2017
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.

5 participants