-
Notifications
You must be signed in to change notification settings - Fork 599
[Config Linux] Replace "required" (lower case) #729
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
Conversation
Replaces "required" (lower case) with "necessary" since this instance does not seem to be intended as "REQUIRED" Signed-off-by: Rob Dolin <robdolin@microsoft.com>
|
|
||
| A runtime MUST at least use the minimum set of cgroup controllers required to fulfill the `resources` settings. | ||
| A runtime MUST at least use the minimum set of cgroup controllers necessary to fulfill the `resources` settings. | ||
| However, a runtime MAY attach the container process to additional cgroup controllers supported by the system. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we even want to keep the line you're bumping? It sounds like any runtime which violated that constraint would necessarily violate some more specific constraint on a resources setting. Maybe replace this paragraph with:
Runtimes MAY attach the container process to additional cgroup controllers beyond those required to fulfill the
resourcessettings.
|
LGTM Suffers from a Travis failure: (might need a rebase and/or DCO munging?) $ TRAVIS_COMMIT_RANGE="${TRAVIS_COMMIT_RANGE/.../..}" make .gitvalidation
git-validation -v -run DCO,short-subject,dangling-whitespace -range 78e6667ae2d67aad100b28ee9580b41b7a24e667..HEAD
2017/03/15 22:55:26 exit status 128
make: *** [.gitvalidation] Error 1
The command "TRAVIS_COMMIT_RANGE="${TRAVIS_COMMIT_RANGE/.../..}" make .gitvalidation" exited with 2.Seems to me that regardless of whether we keep the line, avoiding the word |
Any runtime which violated that constraint would necessarily violate some more specific constraint on a 'resources' setting. This also removes a non-spec-requirement "required" to avoid any confusion with the spec-requirement REQUIRED [1]. [1]: opencontainers#729 (comment) Signed-off-by: W. Trevor King <wking@tremily.us>
|
On Wed, May 10, 2017 at 01:17:47PM -0700, Tianon Gravi wrote:
Seems to me that regardless of whether we keep the line, avoiding
the word `REQUIRED` where it isn't intended to be a strict
validation-related usage is a sane thing to do.
Agreed, although I don't see the point of making the narrow fix if we
can come to an agreement on dropping the redundant MUST. I've filed
#800 with a version of my earlier proposal that avoids “required” in
case maintainers agree that the current MUST is redundant.
|
|
Given #800, my preference is to close this and merge that one instead (since IMO it makes the intent of this bit more clear). |
|
Obsolete with #800 landed? |
|
Yeah, thanks @wking (and @RobDolinMS) |
Replaces "required" (lower case) with "necessary" since this instance does not seem to be intended as "REQUIRED"
Signed-off-by: Rob Dolin robdolin@microsoft.com