pkg/namespaces: add tests for UsernsMode and NetworkMode#28953
Conversation
Honny1
left a comment
Honny1
left a comment
There was a problem hiding this comment.
Thanks, I have some comments.
| // "default" is the default *value* but is not in Valid()'s allowed list. | ||
| {mode: "default", isDefault: true, isPrivate: true, valid: false}, |
There was a problem hiding this comment.
its a bit of a non-case acc to me the Valid() has no callers and also "default" isnt actually a valid --userns value as the parseUserNamespace dont accept it
There was a problem hiding this comment.
Rather than lock in behavior. .. . I have dropped that row
There was a problem hiding this comment.
now the empty-string "" case still covers the default value
| isDefault bool | ||
| isPrivate bool | ||
| isNS bool | ||
| isContainer bool |
There was a problem hiding this comment.
Could this be done better? New type, for example: modeType
There was a problem hiding this comment.
yupp i had thought of usernsModeTest and the networkModeTest types for that
There was a problem hiding this comment.
just lemme know if you had a different structure in mind regarding that to what i have implemented in that
| } | ||
|
|
||
| func TestNetworkModeNS(t *testing.T) { | ||
| assert.True(t, NetworkMode("ns:/run/netns/x").IsNS()) |
There was a problem hiding this comment.
I check the implementation of IsNS. And the network version would also accept only this string ns. Is this correct?
There was a problem hiding this comment.
what i fell is that it's inconsistent not sure
There was a problem hiding this comment.
NetworkMode used HasPrefix(n, "ns") ... so a network named for ex nsproxy was wrongly treated as a namespace and whereas UsernsMode requires ns: .... I tried to fix NetworkMode to require the ns: prefix and added a test specifically for thatt
Add unit tests for the UsernsMode and NetworkMode predicate/parser methods. While testing, NetworkMode.IsNS matched any value starting with "ns" (so a network named e.g. "nsproxy" was treated as a namespace path), unlike UsernsMode.IsNS which requires the "ns:" prefix. Require the "ns:" prefix in NetworkMode.IsNS too so only a real namespace path matches. Fixes: podman-container-tools#28952 Signed-off-by: ROKUMATE <rohitkumawat0110@gmail.com>
ROKUMATE
force-pushed
the
test-pkg-namespaces
branch
from
4ac7615 to
1d960ce
Compare
|
[NON-BLOCKING] Packit jobs failed. @containers/packit-build please check. Everyone else, feel free to ignore. |
| // IsNS indicates a network namespace passed in by path (ns:<path>) | ||
| func (n NetworkMode) IsNS() bool { | ||
| return strings.HasPrefix(string(n), nsType) | ||
| return strings.HasPrefix(string(n), nsType+":") |
There was a problem hiding this comment.
I would put this change into a separate commit.
| mode UsernsMode | ||
| isHost bool | ||
| isKeepID bool | ||
| isNoMap bool | ||
| isAuto bool | ||
| isDefault bool | ||
| isPrivate bool | ||
| isNS bool | ||
| isContainer bool |
There was a problem hiding this comment.
Nonblocking: Is there a better way to define this testcase struct?
| isNS bool | ||
| } | ||
|
|
||
| func TestNetworkMode(t *testing.T) { |
There was a problem hiding this comment.
The networkModeTest could include a userDefined field and an nsPath field to avoid needing a separate test that seems to be a duplicate.
|
|
||
| func TestUsernsMode(t *testing.T) { | ||
| tests := []usernsModeTest{ | ||
| {mode: "", isDefault: true, isPrivate: true, valid: true}, |
There was a problem hiding this comment.
This test case has an empty test name. I am not sure if it is a good idea. Maybe some if when it is "" set name (empty string) or something like that?
2 participants
What
Add unit tests for the UsernsMode and NetworkMode methods in pkg/namespaces.
These parse the --userns / --network mode strings and had no test coverage.
Why
The methods parse user-supplied "mode:options" input (keep-id options, ns:,
container:, pasta, user-defined names, etc.). Tests lock the parsing and
predicate behavior in.