Add interface to remove mounts.#544
Conversation
| ) | ||
|
|
||
| func main() { | ||
| util.Skip("TODO: mounts generation options have not been implemented", "") |
There was a problem hiding this comment.
This test is for runtime validation (and the TODO is stale now that #279 has landed, but that's another matter).
Tests for generation functionality (which is what you're adding in this PR) should go in generate/generate_test.go.
There was a problem hiding this comment.
I am not sure why you would not want to have this in the validation code? This seems like the correct place unless you are dropping the validation code also?
There was a problem hiding this comment.
I am not sure why you would not want to have this in the validation code?
There are several types of validation in this repo:
- Config validation in
validate, with unit tests invalidate/validate_test.go. - Runtime validation (which calls the runtime executable, e.g. here) in
validation. There are no unit tests for this (yet?). - Config generation argument checks in
generate, with unit tests (of which there are currently few) ingenerate/generate_test.go.
The tests you're currently adding here (a type-2 location) belong to type-3 (generation unit tests).
There was a problem hiding this comment.
Ok copied it to the generate_test.go. But not sure if I should remove it totally from Validate. Currently tests in both places.
There was a problem hiding this comment.
Ok copied it to the generate_test.go.
That looks good to me.
But not sure if I should remove it totally from Validate.
I think you should, because the test has nothing to do with runtime compliance (which is what validation/ is for). It's just testing the generate implementation, so the generate_test.go test you've added is all you need.
rhatdan
force-pushed
the
mounts
branch
5 times, most recently
from
40c525c to
1be2b1e
Compare
cmd/oci-runtime-tool/generate.go
Outdated
| cli.StringSliceFlag{Name: "linux-sysctl", Usage: "add sysctl settings e.g net.ipv4.forward=1"}, | ||
| cli.StringSliceFlag{Name: "linux-uidmappings", Usage: "add UIDMappings e.g HostID:ContainerID:Size"}, | ||
| cli.StringSliceFlag{Name: "mounts-add", Usage: "configures additional mounts inside container"}, | ||
| cli.StringSliceFlag{Name: "mounts-remove", Usage: "remove destionation mountpoints from inside container"}, |
|
The appropriate content should be added to |
|
@q384566678 Added to completions/bash |
|
@mrunalp PTAL |
|
One of the commits isn't signed. Otherwise looks good. |
We want to add a mount point to /dev/shm using the default spec, but we don't want to mount /dev/shm via a tmpfs and then add our mountpoint over it. RemoveMount will remove a mount point based on the Destination, if it exists. Does not return any error if the mount point does not exist. Also added Mounts() interface so that I could populate a test for new feature. Returns the list of mounts. Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
|
@mrunalp Fixed PR. |
|
LGTM |
1 similar comment
|
LGTM |
4 participants
We want to add a mount point to /dev/shm using the default spec,
but we don't want to mount /dev/shm via a tmpfs and then add our mountpoint
over it.
RemoveMount will remove a mount point based on the Destination, if it exists.
Does not return any error if the mount point does not exist. Also added
Mounts() interface so that I could populate a test for new feature. Returns
the list of mounts.
Signed-off-by: Daniel J Walsh dwalsh@redhat.com