generate: Adjust to Spec.Linux being a pointer #112
Conversation
|
Your commit to runtime spec is checked in. I don't like the '{}' way, how about using 'reflect.DeepEqual', like this: |
|
On Tue, Jun 28, 2016 at 02:57:30AM -0700, 梁辰晔 (Liang Chenye) wrote:
I'd rather hold off on landing this PR until runtime-spec cuts another
A agree that the JSON marshaling feels awkward, but it is checking $ ./ocitools generate -template <(echo '{}') My guess is that DeepEquals doesn't work because spec.Annotations is |
wking
force-pushed
the
optional-linux
branch
2 times, most recently
from
2f3e807
to
1cd5062
Compare
|
Rebased onto master with 1cd5062 → f2107e5. Between #140 and #144,
the only thing left here is the “don't serialize Spec.Linux as an
empty JSON object” check. Although to notice now, you have to do
something like:
$ ocitools generate --template <(echo '{"linux": {"namespaces": [{"type": "user"}]}}') --user=host
which generates a ‘"linux": {}’ entry on master but not with this
branch.
|
Using jq to format the output so we don't have to worry about
ocitools' default tab indents or lack of trailing newlines, neither of
which play nicely with <<-EOF here documents.
The tests are known failures, because runtime-spec v1.0.0-rc1 lacks
good omitempty handling.
$ ocitools generate --template <(echo '{}')
…
"hooks": {},
"linux": {},
"solaris": {
"cappedCPU": {},
"cappedMemory": {}
}
}
These issues are addressed by the in-flight [1,2], although their late
landings in runtime-spec mean we'll never be able to address them in
the v1.0.0.rc1 branch of ocitools.
[1]: opencontainers/runtime-spec#427
Subject: config: Explicitly list 'hooks' as optional
[2]: opencontainers#112
Subject: generate: Adjust to Spec.Linux being a pointer
Signed-off-by: W. Trevor King <wking@tremily.us>
| @@ -959,6 +969,9 @@ func (g *Generator) SetupPrivileged(privileged bool) { | |||
| g.spec.Process.Capabilities = finalCapList | |||
| g.spec.Process.SelinuxLabel = "" | |||
| g.spec.Process.ApparmorProfile = "" | |||
| if g.spec.Linux == nil { | |||
| g.spec.Linux = &rspec.Linux{} | |||
| } | |||
| g.spec.Linux.Seccomp = nil | |||
There was a problem hiding this comment.
I think this is not needed any more. Because we have g.initSpecLinux()
wking
force-pushed
the
optional-linux
branch
from
f2107e5
to
7fcc366
Compare
|
On Fri, Sep 23, 2016 at 01:31:59AM -0700, Ma Shimiao wrote:
|
This saves a few lines of code and removes a layer of indirection.
It's now obvious to the caller exactly what is changing, although
there is sometimes less per-setting validation. That's ok though;
folks interested in validating the config can call the validator on it
after they've finished mucking about.
This new approach also initializes any needer pointer fields at the
top. With the current runtime-spec types, that can lead to some
orphaned pointer fields, e.g.;
$ ./oci-runtime-tool generate --template <(echo '{}')
{
"ociVersion": "1.0.0-rc1-dev",
"platform": {
"os": "linux",
"arch": "amd64"
},
"process": {
"user": {
"uid": 0,
"gid": 0
},
"args": null,
"cwd": "/"
},
"root": {
"path": "rootfs"
},
"hooks": {},
"linux": {
"resources": {
"devices": null
}
}
}
but the devices issue was fixed in runtime-spec 1.0.0-rc2 [1] and
there are other approaches to collapsing unused pointer properties
[2].
[1]: opencontainers/runtime-spec#526
[2]: opencontainers#112
Signed-off-by: W. Trevor King <wking@tremily.us>
|
need rebase @wking |
The "does the marshaled JSON look like '{}'?" check is a pretty cheap
trick, but it was the easiest way I could think of for "is there
anything useful in here?".
Signed-off-by: W. Trevor King <wking@tremily.us>
wking
force-pushed
the
optional-linux
branch
from
7fcc366
to
9004f6d
Compare
|
@liangchenye any comments? |
|
https://golang.org/src/encoding/json/encode.go?s=5895:5938#L585 |
|
LGTM |
1 similar comment
|
LGTM |
Catch up with the in-flight opencontainers/runtime-spec@6323157 (specs-go/config: Make Linux and Solaris omitempty (again), 2016-06-17, opencontainers/runtime-spec#502).
The “does the marshaled JSON look like '{}'?” check is a pretty cheap trick, but it was the easiest way I could think of for “is there anything useful in here?”. An alternative approach that conditionally added an
&rspec.Linux{}only if needed seemed too tedious.This should not land before opencontainers/runtime-spec#502, and it may not even be worth reviewing until then. But I'd worked up the changes while testing the runtime-spec PR, so I thought I might as well push them in case other folks wanted to see how I'd tested it.