Conversation
zhouhao3
force-pushed
the
fix-plat
branch
2 times, most recently
from
4a08bf7 to
d1898f7
Compare
|
I will add the |
validate/validate.go
Outdated
|
|
||
| if v.spec.Platform.OS != "linux" { | ||
| if v.spec.Linux != nil { | ||
| msgs = append(msgs, fmt.Sprintf("When system is not linux, Linux field should be empty")) |
There was a problem hiding this comment.
This is not supported by any spec wording, which only SHOULDs the setting (and says nothing about unsetting) the platform-specific sections based on platform.os.
There was a problem hiding this comment.
But spec has changed in this PR.
Ah, I'd missed that (that PR changed a lot ;). I'm not sure how the new approach will scale to a vm section if/when VM support lands in the spec, but I guess we'll see.
Anyhow, thanks for re-citing master. I'm fine with your implementation landing as soon as runtime-tools switches from targeting v1.0.0-rc4 (which has the old language) to rc5 (or whichever spec release we target next which has the new language).
As a minor nit, I'd recommend wording that more closely follows the spec. Something like:
msgs = append(msgs, fmt.Sprintf("'linux' MUST NOT be set when platform.os is %q", v.spec.Platform.OS))
There was a problem hiding this comment.
updated,thanks for your advice.
|
LGTM |
validate/validate.go
Outdated
|
|
||
| if v.spec.Platform.OS != "linux" { | ||
| if v.spec.Linux != nil { | ||
| msgs = append(msgs, fmt.Sprintf("'linux' MUST NOT set when platform.os is %q", v.spec.Platform.OS)) |
validate/validate.go
Outdated
|
|
||
| if v.spec.Platform.OS != "solaris" { | ||
| if v.spec.Solaris != nil { | ||
| msgs = append(msgs, fmt.Sprintf("'solaris' MUST NOT set when platform.os is %q", v.spec.Platform.OS)) |
Signed-off-by: zhouhao <zhouhao@cn.fujitsu.com>
|
LGTM |
1 similar comment
|
LGTM |
4 participants
I think we should increase the judgment of several OS.
https://github.com/opencontainers/runtime-spec/blame/master/config.md#L324
Signed-off-by: zhouhao zhouhao@cn.fujitsu.com