Handle sysctls maps #764
Handle sysctls maps #764
Conversation
lemmi
force-pushed
the
sysctls-dict
branch
2 times, most recently
from
1c741d7
to
2f8de64
Compare
|
When I ran the tests with I switched the test class over to I'm not very familiar with python or its tooling, so there might be a better solution. If you want me to move the |
|
|
||
| args0 = await container_to_args(c, cnt0) | ||
| args1 = await container_to_args(c, cnt1) | ||
| self.assertEqual( |
There was a problem hiding this comment.
I think this tests performs the check in a roundabout way. It's the actual contents of args which is important, not that two invocations of container_to_args produce identical results. If someone breaks sysctls completely in the future by e.g. ignoring them this test would still pass.
I think it makes sense to split test into two and compare args to an explicit list of strings like in the test above. This is more verbose, but also more readable as it's immediately clear what kind of result we are expecting.
There was a problem hiding this comment.
Yeah makes sense. I split it in two test cases and made the assertion explicit.
| @@ -25,7 +25,7 @@ def get_minimal_container(): | |||
| } | |||
|
|
|||
|
|
|||
| class TestContainerToArgs(unittest.TestCase): | |||
| class TestContainerToArgs(unittest.IsolatedAsyncioTestCase): | |||
There was a problem hiding this comment.
This is great discovery, thanks.
lemmi
force-pushed
the
sysctls-dict
branch
3 times, most recently
from
f901a92
to
08cc994
Compare
Signed-off-by: lemmi <lemmi@nerd2nerd.org>
- Add support to handle sysctls maps. - Directly raise an error if sysctls is neither an array nor map instead of letting podman fail with an unhelpful message. Support for sysctls arrays was added in containers#261. Fixes containers#754: sysctls only works with arrays, not maps Signed-off-by: lemmi <lemmi@nerd2nerd.org>
|
Rebased and reordered commits. |
Support for sysctls arrays was added in #261.
Fixes #754: sysctls only works with arrays, not maps