-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Allow + in container ID #675
Conversation
Signed-off-by: pankit thapar <pankit@umich.edu>
4 prs for a one char edit? :-) I've been there Here you go for next time when there is a merge conflict:
|
@mikebrow thanks for the guidance. I missed signoff once time and then it just went down the hill from there on. :) |
Sure np... I've done the same before :-) If you forget to sign ... I believe you should be able to just sign and force push to update it... git commit -s --amend |
@mikebrow IIUC containerId format should be valid path names as the state needs to be persisted in disk. It's totally valid to have a file called "something+stuff" so I guess this change makes sense. |
Limiting to ascii letters might be wrong as well... while thinking about it. |
On Tue, Mar 22, 2016 at 09:57:43AM -0700, Marcos Nils wrote:
Even this requirement was lifted in opencontainers/runtime-spec#225. |
Yes... I think Marco meant runc might need some code if the id can't be used as a path name.. |
@mikebrow exactly. |
@mikebrow I added just the |
Hmm.. thinking out loud...
|
On Tue, Mar 22, 2016 at 10:37:47AM -0700, Mike Brown wrote:
This probably isn't what you want, since runC's ‘list’ walks the set |
@wking yes.... and if hashed could fix list to get the cid from the state, after I store the original id in it... But yes your idea would also work. |
Issue opened ... #676 to address this secondary discussion |
On Tue, Mar 22, 2016 at 10:41:44AM -0700, Mike Brown wrote:
Good point, and hashing has nice sharding properties (I'm not sure how |
No need to close this one unless someone comes along and says we should not have '+' chars in cid names. |
LGTM |
LGTM |
Add ambient and bounding capability support
Fix a JSON typo which snuck in with eb114f0 (Add ambient and bounding capability support, 2017-02-02, opencontainers#675). Signed-off-by: W. Trevor King <wking@tremily.us>
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>
Signed-off-by: pankit thapar pankit@umich.edu