From 4f843ad11c2e3a9cdafad1b911437a660d885d2d Mon Sep 17 00:00:00 2001 From: Kanishk Pachauri Date: Mon, 17 Feb 2025 23:36:01 +0530 Subject: [PATCH 1/8] fix: Correctly handle environment variables in container creation Signed-off-by: Kanishk Pachauri --- podman/domain/containers_create.py | 43 +++++++++++++++++++++++++++++- 1 file changed, 42 insertions(+), 1 deletion(-) diff --git a/podman/domain/containers_create.py b/podman/domain/containers_create.py index 80d7f7c8..da6c92df 100644 --- a/podman/domain/containers_create.py +++ b/podman/domain/containers_create.py @@ -383,6 +383,30 @@ def create( return self.get(container_id) + @staticmethod + def _convert_env_list_to_dict(env_list): + """Convert a list of environment variables to a dictionary. + + Args: + env_list (List[str]): List of environment variables in the format ["KEY=value"] + + Returns: + Dict[str, str]: Dictionary of environment variables + + Raises: + ValueError: If any environment variable is not in the correct format + """ + env_dict = {} + for env_var in env_list: + if '=' not in env_var: + raise ValueError( + f"Environment variable '{env_var}' is not in the correct format. " + "Expected format: 'KEY=value'" + ) + key, value = env_var.split('=', 1) # Split on first '=' only + env_dict[key] = value + return env_dict + # pylint: disable=too-many-locals,too-many-statements,too-many-branches @staticmethod def _render_payload(kwargs: MutableMapping[str, Any]) -> dict[str, Any]: @@ -410,6 +434,23 @@ def _render_payload(kwargs: MutableMapping[str, Any]) -> dict[str, Any]: with suppress(KeyError): del args[key] + # Handle environment variables + environment = args.pop("environment", None) + if environment is not None: + if isinstance(environment, list): + try: + environment = CreateMixin._convert_env_list_to_dict(environment) + except ValueError as e: + raise ValueError( + "Failed to convert environment variables list to dictionary. " + f"Error: {str(e)}" + ) from e + elif not isinstance(environment, dict): + raise TypeError( + "Environment variables must be provided as either a dictionary " + "or a list of strings in the format ['KEY=value']" + ) + # These keywords are not supported for various reasons. unsupported_keys = set(args.keys()).intersection( ( @@ -490,7 +531,7 @@ def to_bytes(size: Union[int, str, None]) -> Union[int, None]: "dns_search": pop("dns_search"), "dns_server": pop("dns"), "entrypoint": pop("entrypoint"), - "env": pop("environment"), + "env": environment, "env_host": pop("env_host"), # TODO document, podman only "expose": {}, "groups": pop("group_add"), From 23a0845b5ea4450d46bb1b3251c7cfe73a0c17c1 Mon Sep 17 00:00:00 2001 From: Kanishk Pachauri Date: Mon, 17 Feb 2025 23:36:39 +0530 Subject: [PATCH 2/8] test: Add tests for the enviroment variables Signed-off-by: Kanishk Pachauri --- .../integration/test_container_create.py | 40 +++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/podman/tests/integration/test_container_create.py b/podman/tests/integration/test_container_create.py index 2c4b56d9..e3a97ac5 100644 --- a/podman/tests/integration/test_container_create.py +++ b/podman/tests/integration/test_container_create.py @@ -102,6 +102,46 @@ def test_container_extra_hosts(self): for hosts_entry in formatted_hosts: self.assertIn(hosts_entry, logs) + def test_container_environment_variables(self): + """Test environment variables passed to the container.""" + with self.subTest("Check environment variables as dictionary"): + env_dict = {"MY_VAR": "123", "ANOTHER_VAR": "456"} + container = self.client.containers.create( + self.alpine_image, command=["env"], environment=env_dict + ) + self.containers.append(container) + + self.assertEqual( + container.attrs.get('Config', {}).get('Env', []), + [f"{k}={v}" for k, v in env_dict.items()], + ) + + container.start() + container.wait() + logs = b"\n".join(container.logs()).decode() + + for key, value in env_dict.items(): + self.assertIn(f"{key}={value}", logs) + + with self.subTest("Check environment variables as list"): + env_list = ["MY_VAR=123", "ANOTHER_VAR=456"] + container = self.client.containers.create( + self.alpine_image, command=["env"], environment=env_list + ) + self.containers.append(container) + + self.assertEqual( + container.attrs.get('Config', {}).get('Env', []), + env_list, + ) + + container.start() + container.wait() + logs = b"\n".join(container.logs()).decode() + + for env in env_list: + self.assertIn(env, logs) + def _test_memory_limit(self, parameter_name, host_config_name, set_mem_limit=False): """Base for tests which checks memory limits""" memory_limit_tests = [ From 068e23330f6a2bc3fe24f5b5b9abb8cddb51f136 Mon Sep 17 00:00:00 2001 From: Kanishk Pachauri Date: Mon, 17 Feb 2025 23:51:20 +0530 Subject: [PATCH 3/8] fix: broken tests Signed-off-by: Kanishk Pachauri --- .../tests/integration/test_container_create.py | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/podman/tests/integration/test_container_create.py b/podman/tests/integration/test_container_create.py index e3a97ac5..62ed3063 100644 --- a/podman/tests/integration/test_container_create.py +++ b/podman/tests/integration/test_container_create.py @@ -111,11 +111,12 @@ def test_container_environment_variables(self): ) self.containers.append(container) - self.assertEqual( - container.attrs.get('Config', {}).get('Env', []), - [f"{k}={v}" for k, v in env_dict.items()], - ) + # Verify that the user-provided environment variables are in the container's configuration + container_env = container.attrs.get('Config', {}).get('Env', []) + for key, value in env_dict.items(): + self.assertIn(f"{key}={value}", container_env) + # Start the container and verify the environment variables are set container.start() container.wait() logs = b"\n".join(container.logs()).decode() @@ -130,11 +131,12 @@ def test_container_environment_variables(self): ) self.containers.append(container) - self.assertEqual( - container.attrs.get('Config', {}).get('Env', []), - env_list, - ) + # Verify that the user-provided environment variables are in the container's configuration + container_env = container.attrs.get('Config', {}).get('Env', []) + for env in env_list: + self.assertIn(env, container_env) + # Start the container and verify the environment variables are set container.start() container.wait() logs = b"\n".join(container.logs()).decode() From ee13b44943803210db0b439aa4aeea1ef1fd686c Mon Sep 17 00:00:00 2001 From: Kanishk Pachauri Date: Tue, 18 Feb 2025 00:12:00 +0530 Subject: [PATCH 4/8] chore: removed unuseful comments Signed-off-by: Kanishk Pachauri --- podman/tests/integration/test_container_create.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/podman/tests/integration/test_container_create.py b/podman/tests/integration/test_container_create.py index 62ed3063..a530b98d 100644 --- a/podman/tests/integration/test_container_create.py +++ b/podman/tests/integration/test_container_create.py @@ -111,12 +111,10 @@ def test_container_environment_variables(self): ) self.containers.append(container) - # Verify that the user-provided environment variables are in the container's configuration container_env = container.attrs.get('Config', {}).get('Env', []) for key, value in env_dict.items(): self.assertIn(f"{key}={value}", container_env) - # Start the container and verify the environment variables are set container.start() container.wait() logs = b"\n".join(container.logs()).decode() @@ -131,12 +129,10 @@ def test_container_environment_variables(self): ) self.containers.append(container) - # Verify that the user-provided environment variables are in the container's configuration container_env = container.attrs.get('Config', {}).get('Env', []) for env in env_list: self.assertIn(env, container_env) - # Start the container and verify the environment variables are set container.start() container.wait() logs = b"\n".join(container.logs()).decode() From c150b07f295795c3831ca24b5eeb699a27c89cf2 Mon Sep 17 00:00:00 2001 From: Kanishk Pachauri Date: Tue, 24 Jun 2025 01:01:19 +0530 Subject: [PATCH 5/8] feat: Add exception for edge cases and add unit tests Signed-off-by: Kanishk Pachauri --- podman/domain/containers_create.py | 77 +++++-- podman/tests/unit/test_containersmanager.py | 234 +++++++++++++++++--- 2 files changed, 258 insertions(+), 53 deletions(-) diff --git a/podman/domain/containers_create.py b/podman/domain/containers_create.py index d8fa3e0e..d987ea62 100644 --- a/podman/domain/containers_create.py +++ b/podman/domain/containers_create.py @@ -17,7 +17,7 @@ logger = logging.getLogger("podman.containers") -NAMED_VOLUME_PATTERN = re.compile(r'[a-zA-Z0-9][a-zA-Z0-9_.-]*') +NAMED_VOLUME_PATTERN = re.compile(r"[a-zA-Z0-9][a-zA-Z0-9_.-]*") class CreateMixin: # pylint: disable=too-few-public-methods @@ -375,7 +375,9 @@ def create( payload = api.prepare_body(payload) response = self.client.post( - "/containers/create", headers={"content-type": "application/json"}, data=payload + "/containers/create", + headers={"content-type": "application/json"}, + data=payload, ) response.raise_for_status(not_found=ImageNotFound) @@ -396,14 +398,34 @@ def _convert_env_list_to_dict(env_list): Raises: ValueError: If any environment variable is not in the correct format """ + if not isinstance(env_list, list): + raise TypeError(f"Expected list, got {type(env_list).__name__}") + env_dict = {} + for env_var in env_list: - if '=' not in env_var: + if not isinstance(env_var, str): + raise TypeError( + f"Environment variable must be a string, " + f"got {type(env_var).__name__}: {repr(env_var)}" + ) + + # Handle empty strings + if not env_var.strip(): + raise ValueError(f"Environment variable cannot be empty") + if "=" not in env_var: raise ValueError( f"Environment variable '{env_var}' is not in the correct format. " "Expected format: 'KEY=value'" ) - key, value = env_var.split('=', 1) # Split on first '=' only + key, value = env_var.split("=", 1) # Split on first '=' only + + # Validate key is not empty + if not key.strip(): + raise ValueError( + f"Environment variable at index {i} has empty key: '{env_var}'" + ) + env_dict[key] = value return env_dict @@ -507,9 +529,9 @@ def to_bytes(size: Union[int, str, None]) -> Union[int, None]: try: return int(size) except ValueError as bad_size: - mapping = {'b': 0, 'k': 1, 'm': 2, 'g': 3} - mapping_regex = ''.join(mapping.keys()) - search = re.search(rf'^(\d+)([{mapping_regex}])$', size.lower()) + mapping = {"b": 0, "k": 1, "m": 2, "g": 3} + mapping_regex = "".join(mapping.keys()) + search = re.search(rf"^(\d+)([{mapping_regex}])$", size.lower()) if search: return int(search.group(1)) * (1024 ** mapping[search.group(2)]) raise TypeError( @@ -532,7 +554,9 @@ def to_bytes(size: Union[int, str, None]) -> Union[int, None]: "cni_networks": [pop("network")], "command": args.pop("command", args.pop("cmd", None)), "conmon_pid_file": pop("conmon_pid_file"), # TODO document, podman only - "containerCreateCommand": pop("containerCreateCommand"), # TODO document, podman only + "containerCreateCommand": pop( + "containerCreateCommand" + ), # TODO document, podman only "devices": [], "dns_option": pop("dns_opt"), "dns_search": pop("dns_search"), @@ -581,7 +605,9 @@ def to_bytes(size: Union[int, str, None]) -> Union[int, None]: "rootfs_propagation": pop("rootfs_propagation"), "sdnotifyMode": pop("sdnotifyMode"), # TODO document, podman only "seccomp_policy": pop("seccomp_policy"), # TODO document, podman only - "seccomp_profile_path": pop("seccomp_profile_path"), # TODO document, podman only + "seccomp_profile_path": pop( + "seccomp_profile_path" + ), # TODO document, podman only "secrets": [], # TODO document, podman only "selinux_opts": pop("security_opt"), "shm_size": to_bytes(pop("shm_size")), @@ -597,7 +623,9 @@ def to_bytes(size: Union[int, str, None]) -> Union[int, None]: "unified": pop("unified"), # TODO document, podman only "unmask": pop("unmasked_paths"), # TODO document, podman only "use_image_hosts": pop("use_image_hosts"), # TODO document, podman only - "use_image_resolve_conf": pop("use_image_resolve_conf"), # TODO document, podman only + "use_image_resolve_conf": pop( + "use_image_resolve_conf" + ), # TODO document, podman only "user": pop("user"), "version": pop("version"), "volumes": [], @@ -619,9 +647,15 @@ def to_bytes(size: Union[int, str, None]) -> Union[int, None]: params["log_configuration"]["driver"] = args["log_config"].get("Type") if "Config" in args["log_config"]: - params["log_configuration"]["path"] = args["log_config"]["Config"].get("path") - params["log_configuration"]["size"] = args["log_config"]["Config"].get("size") - params["log_configuration"]["options"] = args["log_config"]["Config"].get("options") + params["log_configuration"]["path"] = args["log_config"]["Config"].get( + "path" + ) + params["log_configuration"]["size"] = args["log_config"]["Config"].get( + "size" + ) + params["log_configuration"]["options"] = args["log_config"][ + "Config" + ].get("options") args.pop("log_config") for item in args.pop("mounts", []): @@ -648,7 +682,7 @@ def to_bytes(size: Union[int, str, None]) -> Union[int, None]: if _k in bool_options and v is True: options.append(option_name) elif _k in regular_options: - options.append(f'{option_name}={v}') + options.append(f"{option_name}={v}") elif _k in simple_options: options.append(v) @@ -676,7 +710,9 @@ def parse_host_port(_container_port, _protocol, _host): result.append(port_map) elif isinstance(_host, list): for host_list in _host: - host_list_result = parse_host_port(_container_port, _protocol, host_list) + host_list_result = parse_host_port( + _container_port, _protocol, host_list + ) result.extend(host_list_result) elif isinstance(_host, dict): _host_port = _host.get("port") @@ -750,12 +786,12 @@ def parse_host_port(_container_port, _protocol, _host): for item in args.pop("volumes", {}).items(): key, value = item - extended_mode = value.get('extended_mode', []) + extended_mode = value.get("extended_mode", []) if not isinstance(extended_mode, list): raise ValueError("'extended_mode' value should be a list") options = extended_mode - mode = value.get('mode') + mode = value.get("mode") if mode is not None: if not isinstance(mode, str): raise ValueError("'mode' value should be a str") @@ -770,10 +806,10 @@ def parse_host_port(_container_port, _protocol, _host): params["volumes"].append(volume) else: mount_point = { - "destination": value['bind'], + "destination": value["bind"], "options": options, "source": key, - "type": 'bind', + "type": "bind", } params["mounts"].append(mount_point) @@ -818,7 +854,8 @@ def parse_host_port(_container_port, _protocol, _host): if len(args) > 0: raise TypeError( - "Unknown keyword argument(s): " + " ,".join(f"'{k}'" for k in args.keys()) + "Unknown keyword argument(s): " + + " ,".join(f"'{k}'" for k in args.keys()) ) return params diff --git a/podman/tests/unit/test_containersmanager.py b/podman/tests/unit/test_containersmanager.py index 47b30482..9e9db707 100644 --- a/podman/tests/unit/test_containersmanager.py +++ b/podman/tests/unit/test_containersmanager.py @@ -8,12 +8,13 @@ # Python < 3.10 from collections.abc import Iterator -from unittest.mock import DEFAULT, patch, MagicMock +from unittest.mock import DEFAULT, MagicMock, patch import requests_mock from podman import PodmanClient, tests from podman.domain.containers import Container +from podman.domain.containers_create import CreateMixin from podman.domain.containers_manager import ContainersManager from podman.errors import ImageNotFound, NotFound @@ -64,7 +65,8 @@ def test_get(self, mock): "87e1325c82424e49a00abdd4de08009eb76c7de8d228426a9b8af9318ced5ecd" ) self.assertEqual( - actual.id, "87e1325c82424e49a00abdd4de08009eb76c7de8d228426a9b8af9318ced5ecd" + actual.id, + "87e1325c82424e49a00abdd4de08009eb76c7de8d228426a9b8af9318ced5ecd", ) @requests_mock.Mocker() @@ -104,10 +106,12 @@ def test_list(self, mock): self.assertIsInstance(actual, list) self.assertEqual( - actual[0].id, "87e1325c82424e49a00abdd4de08009eb76c7de8d228426a9b8af9318ced5ecd" + actual[0].id, + "87e1325c82424e49a00abdd4de08009eb76c7de8d228426a9b8af9318ced5ecd", ) self.assertEqual( - actual[1].id, "6dc84cc0a46747da94e4c1571efcc01a756b4017261440b4b8985d37203c3c03" + actual[1].id, + "6dc84cc0a46747da94e4c1571efcc01a756b4017261440b4b8985d37203c3c03", ) @requests_mock.Mocker() @@ -132,10 +136,12 @@ def test_list_filtered(self, mock): self.assertIsInstance(actual, list) self.assertEqual( - actual[0].id, "87e1325c82424e49a00abdd4de08009eb76c7de8d228426a9b8af9318ced5ecd" + actual[0].id, + "87e1325c82424e49a00abdd4de08009eb76c7de8d228426a9b8af9318ced5ecd", ) self.assertEqual( - actual[1].id, "6dc84cc0a46747da94e4c1571efcc01a756b4017261440b4b8985d37203c3c03" + actual[1].id, + "6dc84cc0a46747da94e4c1571efcc01a756b4017261440b4b8985d37203c3c03", ) @requests_mock.Mocker() @@ -148,10 +154,12 @@ def test_list_no_filters(self, mock): self.assertIsInstance(actual, list) self.assertEqual( - actual[0].id, "87e1325c82424e49a00abdd4de08009eb76c7de8d228426a9b8af9318ced5ecd" + actual[0].id, + "87e1325c82424e49a00abdd4de08009eb76c7de8d228426a9b8af9318ced5ecd", ) self.assertEqual( - actual[1].id, "6dc84cc0a46747da94e4c1571efcc01a756b4017261440b4b8985d37203c3c03" + actual[1].id, + "6dc84cc0a46747da94e4c1571efcc01a756b4017261440b4b8985d37203c3c03", ) @requests_mock.Mocker() @@ -228,8 +236,8 @@ def test_create_parse_host_port(self, mock): json=FIRST_CONTAINER, ) - port_str = {'2233': 3333} - port_str_protocol = {'2244/tcp': 3344} + port_str = {"2233": 3333} + port_str_protocol = {"2244/tcp": 3344} port_int = {2255: 3355} ports = {**port_str, **port_str_protocol, **port_int} self.client.containers.create("fedora", "/usr/bin/ls", ports=ports) @@ -237,24 +245,24 @@ def test_create_parse_host_port(self, mock): self.client.containers.client.post.assert_called() expected_ports = [ { - 'container_port': 2233, - 'host_port': 3333, - 'protocol': 'tcp', + "container_port": 2233, + "host_port": 3333, + "protocol": "tcp", }, { - 'container_port': 2244, - 'host_port': 3344, - 'protocol': 'tcp', + "container_port": 2244, + "host_port": 3344, + "protocol": "tcp", }, { - 'container_port': 2255, - 'host_port': 3355, - 'protocol': 'tcp', + "container_port": 2255, + "host_port": 3355, + "protocol": "tcp", }, ] - actual_ports = json.loads(self.client.containers.client.post.call_args[1]['data'])[ - 'portmappings' - ] + actual_ports = json.loads( + self.client.containers.client.post.call_args[1]["data"] + )["portmappings"] self.assertEqual(expected_ports, actual_ports) @requests_mock.Mocker() @@ -276,9 +284,9 @@ def test_create_userns_mode_simple(self, mock): self.client.containers.client.post.assert_called() expected_userns = {"nsmode": userns} - actual_userns = json.loads(self.client.containers.client.post.call_args[1]["data"])[ - "userns" - ] + actual_userns = json.loads( + self.client.containers.client.post.call_args[1]["data"] + )["userns"] self.assertEqual(expected_userns, actual_userns) @requests_mock.Mocker() @@ -300,9 +308,9 @@ def test_create_userns_mode_dict(self, mock): self.client.containers.client.post.assert_called() expected_userns = dict(**userns) - actual_userns = json.loads(self.client.containers.client.post.call_args[1]["data"])[ - "userns" - ] + actual_userns = json.loads( + self.client.containers.client.post.call_args[1]["data"] + )["userns"] self.assertEqual(expected_userns, actual_userns) def test_create_unsupported_key(self): @@ -313,6 +321,158 @@ def test_create_unknown_key(self): 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) + + # Test empty list + env_list5 = [] + converted_dict5 = {} + self.assertEqual( + CreateMixin._convert_env_list_to_dict(env_list5), converted_dict5 + ) + + # Test single valid environment variable + env_list6 = ["SINGLE=value"] + converted_dict6 = {"SINGLE": "value"} + self.assertEqual( + CreateMixin._convert_env_list_to_dict(env_list6), converted_dict6 + ) + + # Test environment variable with empty value + env_list7 = ["EMPTY="] + converted_dict7 = {"EMPTY": ""} + self.assertEqual( + CreateMixin._convert_env_list_to_dict(env_list7), converted_dict7 + ) + + # Test environment variable with multiple equals signs + env_list8 = ["URL=https://example.com/path?param=value"] + converted_dict8 = {"URL": "https://example.com/path?param=value"} + self.assertEqual( + CreateMixin._convert_env_list_to_dict(env_list8), converted_dict8 + ) + + # Test environment variable with spaces in value + env_list9 = ["MESSAGE=Hello World", "PATH=/usr/local/bin:/usr/bin"] + converted_dict9 = {"MESSAGE": "Hello World", "PATH": "/usr/local/bin:/usr/bin"} + self.assertEqual( + CreateMixin._convert_env_list_to_dict(env_list9), converted_dict9 + ) + + # Test environment variable with special characters + env_list10 = ["SPECIAL=!@#$%^&*()_+-=[]{}|;':\",./<>?"] + converted_dict10 = {"SPECIAL": "!@#$%^&*()_+-=[]{}|;':\",./<>?"} + self.assertEqual( + CreateMixin._convert_env_list_to_dict(env_list10), converted_dict10 + ) + + # Test environment variable with numeric values + env_list11 = ["PORT=8080", "TIMEOUT=30"] + converted_dict11 = {"PORT": "8080", "TIMEOUT": "30"} + self.assertEqual( + CreateMixin._convert_env_list_to_dict(env_list11), converted_dict11 + ) + + # Test environment variable with boolean-like values + env_list12 = ["DEBUG=true", "VERBOSE=false", "ENABLED=1", "DISABLED=0"] + converted_dict12 = { + "DEBUG": "true", + "VERBOSE": "false", + "ENABLED": "1", + "DISABLED": "0", + } + self.assertEqual( + CreateMixin._convert_env_list_to_dict(env_list12), converted_dict12 + ) + + # Test environment variable with whitespace in key (should preserve) + env_list13 = [" SPACED_KEY =value", "KEY= spaced_value "] + converted_dict13 = {" SPACED_KEY ": "value", "KEY": " spaced_value "} + self.assertEqual( + CreateMixin._convert_env_list_to_dict(env_list13), converted_dict13 + ) + + # Test missing equals sign + env_list14 = ["FOO=foo", "INVALID"] + self.assertRaises(ValueError, CreateMixin._convert_env_list_to_dict, env_list14) + + # Test environment variable with only equals sign (empty key) + env_list15 = ["FOO=foo", "=value"] + self.assertRaises(ValueError, CreateMixin._convert_env_list_to_dict, env_list15) + + # Test environment variable with only whitespace key + env_list16 = ["FOO=foo", " =value"] + self.assertRaises(ValueError, CreateMixin._convert_env_list_to_dict, env_list16) + + # Test whitespace-only string + env_list17 = ["FOO=foo", " "] + self.assertRaises(ValueError, CreateMixin._convert_env_list_to_dict, env_list17) + + # Test various non-string types in list + env_list18 = ["FOO=foo", 123] + self.assertRaises(TypeError, CreateMixin._convert_env_list_to_dict, env_list18) + + env_list19 = ["FOO=foo", {"key": "value"}] + self.assertRaises(TypeError, CreateMixin._convert_env_list_to_dict, env_list19) + + env_list20 = ["FOO=foo", True] + self.assertRaises(TypeError, CreateMixin._convert_env_list_to_dict, env_list20) + + # Test duplicate keys (last one should win) + env_list21 = ["KEY=first", "KEY=second", "OTHER=value"] + converted_dict21 = {"KEY": "second", "OTHER": "value"} + self.assertEqual( + CreateMixin._convert_env_list_to_dict(env_list21), converted_dict21 + ) + + # Test very long environment variable + long_value = "x" * 1000 + env_list22 = [f"LONG_VAR={long_value}"] + converted_dict22 = {"LONG_VAR": long_value} + self.assertEqual( + CreateMixin._convert_env_list_to_dict(env_list22), converted_dict22 + ) + + # Test environment variable with newlines and tabs + env_list23 = ["MULTILINE=line1\nline2\ttabbed"] + converted_dict23 = {"MULTILINE": "line1\nline2\ttabbed"} + self.assertEqual( + CreateMixin._convert_env_list_to_dict(env_list23), converted_dict23 + ) + + # Test environment variable with unicode characters + env_list24 = ["UNICODE=こんにちは", "EMOJI=🚀🌟"] + converted_dict24 = {"UNICODE": "こんにちは", "EMOJI": "🚀🌟"} + self.assertEqual( + CreateMixin._convert_env_list_to_dict(env_list24), converted_dict24 + ) + + # Test case sensitivity + env_list25 = ["path=/usr/bin", "PATH=/usr/local/bin"] + converted_dict25 = {"path": "/usr/bin", "PATH": "/usr/local/bin"} + self.assertEqual( + CreateMixin._convert_env_list_to_dict(env_list25), converted_dict25 + ) + @requests_mock.Mocker() def test_run_detached(self, mock): mock.post( @@ -334,7 +494,9 @@ def test_run_detached(self, mock): json=FIRST_CONTAINER, ) - with patch.multiple(Container, logs=DEFAULT, wait=DEFAULT, autospec=True) as mock_container: + with patch.multiple( + Container, logs=DEFAULT, wait=DEFAULT, autospec=True + ) as mock_container: mock_container["logs"].return_value = [] mock_container["wait"].return_value = {"StatusCode": 0} @@ -367,7 +529,9 @@ def test_run(self, mock): b"This is a unittest - line 2", ) - with patch.multiple(Container, logs=DEFAULT, wait=DEFAULT, autospec=True) as mock_container: + with patch.multiple( + Container, logs=DEFAULT, wait=DEFAULT, autospec=True + ) as mock_container: mock_container["wait"].return_value = 0 with self.subTest("Results not streamed"): @@ -375,18 +539,22 @@ def test_run(self, mock): actual = self.client.containers.run("fedora", "/usr/bin/ls") self.assertIsInstance(actual, bytes) - self.assertEqual(actual, b'This is a unittest - line 1This is a unittest - line 2') + self.assertEqual( + actual, b"This is a unittest - line 1This is a unittest - line 2" + ) # iter() cannot be reset so subtests used to create new instance with self.subTest("Stream results"): mock_container["logs"].return_value = iter(mock_logs) - actual = self.client.containers.run("fedora", "/usr/bin/ls", stream=True) + actual = self.client.containers.run( + "fedora", "/usr/bin/ls", stream=True + ) self.assertNotIsInstance(actual, bytes) self.assertIsInstance(actual, Iterator) self.assertEqual(next(actual), b"This is a unittest - line 1") self.assertEqual(next(actual), b"This is a unittest - line 2") -if __name__ == '__main__': +if __name__ == "__main__": unittest.main() From fac45dd5badce7a5791d47dbb333315474fe7425 Mon Sep 17 00:00:00 2001 From: Kanishk Pachauri Date: Tue, 24 Jun 2025 01:11:40 +0530 Subject: [PATCH 6/8] chore: fix formatting errors Signed-off-by: Kanishk Pachauri --- podman/domain/containers_create.py | 35 ++------ podman/tests/unit/test_containersmanager.py | 95 ++++++--------------- 2 files changed, 37 insertions(+), 93 deletions(-) diff --git a/podman/domain/containers_create.py b/podman/domain/containers_create.py index d987ea62..af7997d8 100644 --- a/podman/domain/containers_create.py +++ b/podman/domain/containers_create.py @@ -422,9 +422,7 @@ def _convert_env_list_to_dict(env_list): # Validate key is not empty if not key.strip(): - raise ValueError( - f"Environment variable at index {i} has empty key: '{env_var}'" - ) + raise ValueError(f"Environment variable at index {i} has empty key: '{env_var}'") env_dict[key] = value return env_dict @@ -554,9 +552,7 @@ def to_bytes(size: Union[int, str, None]) -> Union[int, None]: "cni_networks": [pop("network")], "command": args.pop("command", args.pop("cmd", None)), "conmon_pid_file": pop("conmon_pid_file"), # TODO document, podman only - "containerCreateCommand": pop( - "containerCreateCommand" - ), # TODO document, podman only + "containerCreateCommand": pop("containerCreateCommand"), # TODO document, podman only "devices": [], "dns_option": pop("dns_opt"), "dns_search": pop("dns_search"), @@ -605,9 +601,7 @@ def to_bytes(size: Union[int, str, None]) -> Union[int, None]: "rootfs_propagation": pop("rootfs_propagation"), "sdnotifyMode": pop("sdnotifyMode"), # TODO document, podman only "seccomp_policy": pop("seccomp_policy"), # TODO document, podman only - "seccomp_profile_path": pop( - "seccomp_profile_path" - ), # TODO document, podman only + "seccomp_profile_path": pop("seccomp_profile_path"), # TODO document, podman only "secrets": [], # TODO document, podman only "selinux_opts": pop("security_opt"), "shm_size": to_bytes(pop("shm_size")), @@ -623,9 +617,7 @@ def to_bytes(size: Union[int, str, None]) -> Union[int, None]: "unified": pop("unified"), # TODO document, podman only "unmask": pop("unmasked_paths"), # TODO document, podman only "use_image_hosts": pop("use_image_hosts"), # TODO document, podman only - "use_image_resolve_conf": pop( - "use_image_resolve_conf" - ), # TODO document, podman only + "use_image_resolve_conf": pop("use_image_resolve_conf"), # TODO document, podman only "user": pop("user"), "version": pop("version"), "volumes": [], @@ -647,15 +639,9 @@ def to_bytes(size: Union[int, str, None]) -> Union[int, None]: params["log_configuration"]["driver"] = args["log_config"].get("Type") if "Config" in args["log_config"]: - params["log_configuration"]["path"] = args["log_config"]["Config"].get( - "path" - ) - params["log_configuration"]["size"] = args["log_config"]["Config"].get( - "size" - ) - params["log_configuration"]["options"] = args["log_config"][ - "Config" - ].get("options") + params["log_configuration"]["path"] = args["log_config"]["Config"].get("path") + params["log_configuration"]["size"] = args["log_config"]["Config"].get("size") + params["log_configuration"]["options"] = args["log_config"]["Config"].get("options") args.pop("log_config") for item in args.pop("mounts", []): @@ -710,9 +696,7 @@ def parse_host_port(_container_port, _protocol, _host): result.append(port_map) elif isinstance(_host, list): for host_list in _host: - host_list_result = parse_host_port( - _container_port, _protocol, host_list - ) + host_list_result = parse_host_port(_container_port, _protocol, host_list) result.extend(host_list_result) elif isinstance(_host, dict): _host_port = _host.get("port") @@ -854,8 +838,7 @@ def parse_host_port(_container_port, _protocol, _host): if len(args) > 0: raise TypeError( - "Unknown keyword argument(s): " - + " ,".join(f"'{k}'" for k in args.keys()) + "Unknown keyword argument(s): " + " ,".join(f"'{k}'" for k in args.keys()) ) return params diff --git a/podman/tests/unit/test_containersmanager.py b/podman/tests/unit/test_containersmanager.py index 9e9db707..3b60f90a 100644 --- a/podman/tests/unit/test_containersmanager.py +++ b/podman/tests/unit/test_containersmanager.py @@ -260,9 +260,9 @@ def test_create_parse_host_port(self, mock): "protocol": "tcp", }, ] - actual_ports = json.loads( - self.client.containers.client.post.call_args[1]["data"] - )["portmappings"] + actual_ports = json.loads(self.client.containers.client.post.call_args[1]["data"])[ + "portmappings" + ] self.assertEqual(expected_ports, actual_ports) @requests_mock.Mocker() @@ -284,9 +284,9 @@ def test_create_userns_mode_simple(self, mock): self.client.containers.client.post.assert_called() expected_userns = {"nsmode": userns} - actual_userns = json.loads( - self.client.containers.client.post.call_args[1]["data"] - )["userns"] + actual_userns = json.loads(self.client.containers.client.post.call_args[1]["data"])[ + "userns" + ] self.assertEqual(expected_userns, actual_userns) @requests_mock.Mocker() @@ -308,9 +308,9 @@ def test_create_userns_mode_dict(self, mock): self.client.containers.client.post.assert_called() expected_userns = dict(**userns) - actual_userns = json.loads( - self.client.containers.client.post.call_args[1]["data"] - )["userns"] + actual_userns = json.loads(self.client.containers.client.post.call_args[1]["data"])[ + "userns" + ] self.assertEqual(expected_userns, actual_userns) def test_create_unsupported_key(self): @@ -323,13 +323,10 @@ def test_create_unknown_key(self): @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 - ) + self.assertEqual(CreateMixin._convert_env_list_to_dict(env_list1), converted_dict1) # Test empty string env_list2 = ["FOO=foo", ""] @@ -346,51 +343,37 @@ def test_create_convert_env_list_to_dict(self, mock): # Test empty list env_list5 = [] converted_dict5 = {} - self.assertEqual( - CreateMixin._convert_env_list_to_dict(env_list5), converted_dict5 - ) + self.assertEqual(CreateMixin._convert_env_list_to_dict(env_list5), converted_dict5) # Test single valid environment variable env_list6 = ["SINGLE=value"] converted_dict6 = {"SINGLE": "value"} - self.assertEqual( - CreateMixin._convert_env_list_to_dict(env_list6), converted_dict6 - ) + self.assertEqual(CreateMixin._convert_env_list_to_dict(env_list6), converted_dict6) # Test environment variable with empty value env_list7 = ["EMPTY="] converted_dict7 = {"EMPTY": ""} - self.assertEqual( - CreateMixin._convert_env_list_to_dict(env_list7), converted_dict7 - ) + self.assertEqual(CreateMixin._convert_env_list_to_dict(env_list7), converted_dict7) # Test environment variable with multiple equals signs env_list8 = ["URL=https://example.com/path?param=value"] converted_dict8 = {"URL": "https://example.com/path?param=value"} - self.assertEqual( - CreateMixin._convert_env_list_to_dict(env_list8), converted_dict8 - ) + self.assertEqual(CreateMixin._convert_env_list_to_dict(env_list8), converted_dict8) # Test environment variable with spaces in value env_list9 = ["MESSAGE=Hello World", "PATH=/usr/local/bin:/usr/bin"] converted_dict9 = {"MESSAGE": "Hello World", "PATH": "/usr/local/bin:/usr/bin"} - self.assertEqual( - CreateMixin._convert_env_list_to_dict(env_list9), converted_dict9 - ) + self.assertEqual(CreateMixin._convert_env_list_to_dict(env_list9), converted_dict9) # Test environment variable with special characters env_list10 = ["SPECIAL=!@#$%^&*()_+-=[]{}|;':\",./<>?"] converted_dict10 = {"SPECIAL": "!@#$%^&*()_+-=[]{}|;':\",./<>?"} - self.assertEqual( - CreateMixin._convert_env_list_to_dict(env_list10), converted_dict10 - ) + self.assertEqual(CreateMixin._convert_env_list_to_dict(env_list10), converted_dict10) # Test environment variable with numeric values env_list11 = ["PORT=8080", "TIMEOUT=30"] converted_dict11 = {"PORT": "8080", "TIMEOUT": "30"} - self.assertEqual( - CreateMixin._convert_env_list_to_dict(env_list11), converted_dict11 - ) + self.assertEqual(CreateMixin._convert_env_list_to_dict(env_list11), converted_dict11) # Test environment variable with boolean-like values env_list12 = ["DEBUG=true", "VERBOSE=false", "ENABLED=1", "DISABLED=0"] @@ -400,16 +383,12 @@ def test_create_convert_env_list_to_dict(self, mock): "ENABLED": "1", "DISABLED": "0", } - self.assertEqual( - CreateMixin._convert_env_list_to_dict(env_list12), converted_dict12 - ) + self.assertEqual(CreateMixin._convert_env_list_to_dict(env_list12), converted_dict12) # Test environment variable with whitespace in key (should preserve) env_list13 = [" SPACED_KEY =value", "KEY= spaced_value "] converted_dict13 = {" SPACED_KEY ": "value", "KEY": " spaced_value "} - self.assertEqual( - CreateMixin._convert_env_list_to_dict(env_list13), converted_dict13 - ) + self.assertEqual(CreateMixin._convert_env_list_to_dict(env_list13), converted_dict13) # Test missing equals sign env_list14 = ["FOO=foo", "INVALID"] @@ -440,38 +419,28 @@ def test_create_convert_env_list_to_dict(self, mock): # Test duplicate keys (last one should win) env_list21 = ["KEY=first", "KEY=second", "OTHER=value"] converted_dict21 = {"KEY": "second", "OTHER": "value"} - self.assertEqual( - CreateMixin._convert_env_list_to_dict(env_list21), converted_dict21 - ) + self.assertEqual(CreateMixin._convert_env_list_to_dict(env_list21), converted_dict21) # Test very long environment variable long_value = "x" * 1000 env_list22 = [f"LONG_VAR={long_value}"] converted_dict22 = {"LONG_VAR": long_value} - self.assertEqual( - CreateMixin._convert_env_list_to_dict(env_list22), converted_dict22 - ) + self.assertEqual(CreateMixin._convert_env_list_to_dict(env_list22), converted_dict22) # Test environment variable with newlines and tabs env_list23 = ["MULTILINE=line1\nline2\ttabbed"] converted_dict23 = {"MULTILINE": "line1\nline2\ttabbed"} - self.assertEqual( - CreateMixin._convert_env_list_to_dict(env_list23), converted_dict23 - ) + self.assertEqual(CreateMixin._convert_env_list_to_dict(env_list23), converted_dict23) # Test environment variable with unicode characters env_list24 = ["UNICODE=こんにちは", "EMOJI=🚀🌟"] converted_dict24 = {"UNICODE": "こんにちは", "EMOJI": "🚀🌟"} - self.assertEqual( - CreateMixin._convert_env_list_to_dict(env_list24), converted_dict24 - ) + self.assertEqual(CreateMixin._convert_env_list_to_dict(env_list24), converted_dict24) # Test case sensitivity env_list25 = ["path=/usr/bin", "PATH=/usr/local/bin"] converted_dict25 = {"path": "/usr/bin", "PATH": "/usr/local/bin"} - self.assertEqual( - CreateMixin._convert_env_list_to_dict(env_list25), converted_dict25 - ) + self.assertEqual(CreateMixin._convert_env_list_to_dict(env_list25), converted_dict25) @requests_mock.Mocker() def test_run_detached(self, mock): @@ -494,9 +463,7 @@ def test_run_detached(self, mock): json=FIRST_CONTAINER, ) - with patch.multiple( - Container, logs=DEFAULT, wait=DEFAULT, autospec=True - ) as mock_container: + with patch.multiple(Container, logs=DEFAULT, wait=DEFAULT, autospec=True) as mock_container: mock_container["logs"].return_value = [] mock_container["wait"].return_value = {"StatusCode": 0} @@ -529,9 +496,7 @@ def test_run(self, mock): b"This is a unittest - line 2", ) - with patch.multiple( - Container, logs=DEFAULT, wait=DEFAULT, autospec=True - ) as mock_container: + with patch.multiple(Container, logs=DEFAULT, wait=DEFAULT, autospec=True) as mock_container: mock_container["wait"].return_value = 0 with self.subTest("Results not streamed"): @@ -539,17 +504,13 @@ def test_run(self, mock): actual = self.client.containers.run("fedora", "/usr/bin/ls") self.assertIsInstance(actual, bytes) - self.assertEqual( - actual, b"This is a unittest - line 1This is a unittest - line 2" - ) + self.assertEqual(actual, b"This is a unittest - line 1This is a unittest - line 2") # iter() cannot be reset so subtests used to create new instance with self.subTest("Stream results"): mock_container["logs"].return_value = iter(mock_logs) - actual = self.client.containers.run( - "fedora", "/usr/bin/ls", stream=True - ) + actual = self.client.containers.run("fedora", "/usr/bin/ls", stream=True) self.assertNotIsInstance(actual, bytes) self.assertIsInstance(actual, Iterator) self.assertEqual(next(actual), b"This is a unittest - line 1") From a6ca81cec22c8daaba77765c785837960efcf1fb Mon Sep 17 00:00:00 2001 From: Kanishk Pachauri Date: Tue, 24 Jun 2025 01:27:09 +0530 Subject: [PATCH 7/8] fix: failing test due to type error Signed-off-by: Kanishk Pachauri --- podman/tests/unit/test_containersmanager.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/podman/tests/unit/test_containersmanager.py b/podman/tests/unit/test_containersmanager.py index 3b60f90a..28e20d33 100644 --- a/podman/tests/unit/test_containersmanager.py +++ b/podman/tests/unit/test_containersmanager.py @@ -334,11 +334,11 @@ def test_create_convert_env_list_to_dict(self, mock): # Test non iterable env_list3 = ["FOO=foo", None] - self.assertRaises(ValueError, CreateMixin._convert_env_list_to_dict, env_list3) + self.assertRaises(TypeError, 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) + self.assertRaises(TypeError, CreateMixin._convert_env_list_to_dict, env_list4) # Test empty list env_list5 = [] From e472ae020d5a3228b56c2f442e45e38eded2f48b Mon Sep 17 00:00:00 2001 From: Kanishk Pachauri Date: Tue, 24 Jun 2025 01:31:18 +0530 Subject: [PATCH 8/8] fix: remove unused variable Signed-off-by: Kanishk Pachauri --- podman/domain/containers_create.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/podman/domain/containers_create.py b/podman/domain/containers_create.py index af7997d8..b6a5ddff 100644 --- a/podman/domain/containers_create.py +++ b/podman/domain/containers_create.py @@ -422,7 +422,7 @@ def _convert_env_list_to_dict(env_list): # Validate key is not empty if not key.strip(): - raise ValueError(f"Environment variable at index {i} has empty key: '{env_var}'") + raise ValueError(f"Environment variable has empty key: '{env_var}'") env_dict[key] = value return env_dict