-
Notifications
You must be signed in to change notification settings - Fork 113
Fix/#489 #513
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Fix/#489 #513
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Mr-Sunglasses The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
efa6bee
to
77b00ba
Compare
Signed-off-by: Kanishk Pachauri <[email protected]>
Signed-off-by: Kanishk Pachauri <[email protected]>
Signed-off-by: Kanishk Pachauri <[email protected]>
Signed-off-by: Kanishk Pachauri <[email protected]>
77b00ba
to
ee13b44
Compare
/packit build |
@inknos PTAL |
""" | ||
env_dict = {} | ||
for env_var in env_list: | ||
if '=' not in env_var: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Mr-Sunglasses my sincere apologies for this PR taking so long, I postponed and forgot to review many times.
What about adding a type check on str
here?
Also, this new function needs to be unit testes. I made an example here so you can tell me what you think.
diff --git a/podman/tests/unit/test_containersmanager.py b/podman/tests/unit/test_containersmanager.py
index 47b3048..44dd47c 100644
--- a/podman/tests/unit/test_containersmanager.py
+++ b/podman/tests/unit/test_containersmanager.py
@@ -16,6 +16,7 @@ from podman import PodmanClient, tests
from podman.domain.containers import Container
from podman.domain.containers_manager import ContainersManager
from podman.errors import ImageNotFound, NotFound
+from podman.domain.containers_create import CreateMixin
FIRST_CONTAINER = {
"Id": "87e1325c82424e49a00abdd4de08009eb76c7de8d228426a9b8af9318ced5ecd",
@@ -313,6 +314,25 @@ class ContainersManagerTestCase(unittest.TestCase):
with self.assertRaises(TypeError):
self.client.containers.create("fedora", "/usr/bin/ls", unknown_key=100.0)
+ @requests_mock.Mocker()
+ def test_create_convert_env_list_to_dict(self, mock):
+ env_list1 = ["FOO=foo", "BAR=bar"]
+ # Test valid list
+ converted_dict1 = { "FOO": "foo", "BAR": "bar" }
+ self.assertEqual(CreateMixin._convert_env_list_to_dict(env_list1), converted_dict1)
+
+ # Test empty string
+ env_list2 = ["FOO=foo", ""]
+ self.assertRaises(ValueError, CreateMixin._convert_env_list_to_dict, env_list2)
+
+ # Test non iterable
+ env_list3 = ["FOO=foo", None]
+ self.assertRaises(ValueError, CreateMixin._convert_env_list_to_dict, env_list3)
+
+ # Test iterable with non string element
+ env_list4 = ["FOO=foo", []]
+ self.assertRaises(ValueError, CreateMixin._convert_env_list_to_dict, env_list4)
+
@requests_mock.Mocker()
def test_run_detached(self, mock):
mock.post(
The rest LGTM, please ping me when you are done so I will give you quick attention
fix: #489