fixing typo in device access error #673
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: rajasec <rajasec79@gmail.com> fixing typo in device access error Signed-off-by: rajasec <rajasec79@gmail.com> Fixed review comments Signed-off-by: rajasec <rajasec79@gmail.com>
| @@ -385,7 +385,7 @@ func createCgroupConfig(name string, spec *specs.Spec) (*configs.Cgroup, error) | |||
| minor = *d.Minor | |||
| } | |||
| if d.Access == nil || *d.Access == "" { | |||
| return nil, fmt.Errorf("device access at %d field canot be empty", i) | |||
| return nil, fmt.Errorf("device access at %d field can not be empty", i) | |||
There was a problem hiding this comment.
For this sentence, I believe the proper spelling + grammar is 'cannot.' :-)
|
My initial commit was cannot :) , then later I changed to can not |
|
While I can not say that the second commit is better I can say that the first commit was :-) Use cannot to indicate other options are not possible... use can not to indicate something like can not be done at this time... but is valid if... |
|
I'm repushing my first commit, because this can interpreted in different way. |
|
LGTM |
stefanberger
pushed a commit
to stefanberger/runc
that referenced
this pull request
Sep 8, 2017
narrative cleanup in support of Base Config Compatibility opencontainers#303
stefanberger
pushed a commit
to stefanberger/runc
that referenced
this pull request
Sep 8, 2017
This wording just landed via 718f9f3 (origin/pr/673) minor narrative cleanup regarding config compatibility, 2017-01-30, opencontainers#673), but the rule is generic and not unique to platform-specific properties. Also adjust the wording somewhat to match the more established wording from the "Extensibility" section. Signed-off-by: W. Trevor King <wking@tremily.us>
stefanberger
pushed a commit
to stefanberger/runc
that referenced
this pull request
Sep 8, 2017
The current "For example, valid values for Linux..." wording in config.md does not seem strong enough to support this condition, especially since the spec makes no claims about what valid capabilities are for non-Linux OSes. And process.capabilities has been nominally legal for non-Linux OSes since 718f9f3 (minor narrative cleanup regarding config compatibility, 2017-01-30, opencontainers#673). Signed-off-by: W. Trevor King <wking@tremily.us>
stefanberger
pushed a commit
to stefanberger/runc
that referenced
this pull request
Sep 8, 2017
This line landed in 718f9f3 (origin/pr/673) minor narrative cleanup regarding config compatibility, 2017-01-30, opencontainers#673), but I'm not clear on the motivation. The wording reads to me like "you don't have to support valid values for platform-specific fields if you don't want to", but we already cover unsupported value handling with the "MUST generate an error when invalid or unsupported values are encountered" language separated out in c763e64 (config: Move valid-value rules to their own section, 2017-02-07, opencontainers#681). This commit removes the ambiguous line. Signed-off-by: W. Trevor King <wking@tremily.us>
stefanberger
pushed a commit
to stefanberger/runc
that referenced
this pull request
Sep 8, 2017
Roll back the genericization from 718f9f3 (minor narrative cleanup regarding config compatibility, 2017-01-30, opencontainers#673). Lifting the restriction there seems to have been motivated by "Solaris supports capabilities", but that was before the split into a capabilities object which happened in eb114f0 (Add ambient and bounding capability support, 2017-02-02, opencontainers#675). It's not clear if Solaris supports ambient caps, or what Solaris API noNewPrivileges were punting to [1]. And John Howard has recently confirmed that Windows does not support capabilities and is unlikely to do so in the future [2]. He also confirmed that Windows does not support rlimits [3]. John's statement didn't directly address noNewPrivileges, but we can always restore any of these properties to the Solaris/Windows platforms if/when we get docs about which API we're punting to on those platforms. Also add some backticks, remove the hyphens in "OPTIONAL) - the", standardize lines I touch to use "the process" [4], and use four-space indents here to keep Pandoc happy (see 7795661 (runtime.md: Fix sub-bullet indentation, 2016-06-08, opencontainers#495). [1]: opencontainers/runtime-spec#673 (comment) [2]: opencontainers/runtime-spec#810 (comment) [3]: opencontainers/runtime-spec#835 (comment) [4]: opencontainers/runtime-spec#809 (comment) Signed-off-by: W. Trevor King <wking@tremily.us>
stefanberger
pushed a commit
to stefanberger/runc
that referenced
this pull request
Sep 8, 2017
This property was initially Linux-specific. 718f9f3 (minor narrative cleanup regarding config compatibility, 2017-01-30, opencontainers#673) removed the Linux restriction, but the rlimit concept is from POSIX and Windows doesn't support it [1]. This commit adds new subsections for the POSIX-specific and Linux-specific process entries (to match the approach we currently use for process.user), and punts to POSIX for the Solaris values and compliance testing approach. If/when we get a Solaris-specific doc for valid values, we can replace the POSIX punt there, but we probably want to continue punting to POSIX for getrlimit(3)-based compliance testing. I've renamed the overly-specific LinuxRlimit to POSIXRlimit. We could use the generic Rlimit, but then we'd be stuck if/when Windows adds support for some rlimit-like thing that doesn't match up cleanly enough for us to use the POSIX structure. [1]: opencontainers/runtime-spec#835 (comment) Signed-off-by: W. Trevor King <wking@tremily.us>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixed the typo in error for device access.
Signed-off-by: rajasec rajasec79@gmail.com