fix(core): Typing in docker_client #702

Tranquility2 · 2024-09-16T05:59:50Z

poetry run mypy --config-file pyproject.toml core/testcontainers/core/docker_client.py 
Success: no issues found in 1 source file

Old

                    Error Summary                     
┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━┓
┃ File Path                                 ┃ Errors ┃
┡━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━┩
│ core/testcontainers/core/version.py       │ 12     │
│ core/testcontainers/core/docker_client.py │ 14     │
│ core/testcontainers/core/image.py         │ 17     │
│ core/testcontainers/core/waiting_utils.py │ 8      │
│ core/testcontainers/core/container.py     │ 20     │
│ core/tests/test_new_docker_api.py         │ 4      │
│ core/tests/test_docker_in_docker.py       │ 2      │
│ core/testcontainers/compose/compose.py    │ 22     │
│ core/testcontainers/compose/__init__.py   │ 2      │
│ core/tests/test_version.py                │ 2      │
│ core/tests/test_ryuk.py                   │ 2      │
│ core/tests/test_registry.py               │ 1      │
│ core/tests/test_image.py                  │ 3      │
│ core/tests/test_compose.py                │ 7      │
└───────────────────────────────────────────┴────────┘
Found 116 errors in 14 files.

New

                    Error Summary                     
┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━┓
┃ File Path                                 ┃ Errors ┃
┡━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━┩
│ core/testcontainers/core/version.py       │ 12     │
│ core/testcontainers/core/network.py       │ 3      │
│ core/testcontainers/core/image.py         │ 17     │
│ core/testcontainers/core/waiting_utils.py │ 8      │
│ core/testcontainers/core/container.py     │ 19     │
│ core/tests/test_new_docker_api.py         │ 4      │
│ core/tests/test_docker_in_docker.py       │ 2      │
│ core/testcontainers/compose/compose.py    │ 22     │
│ core/testcontainers/compose/__init__.py   │ 2      │
│ core/tests/test_version.py                │ 2      │
│ core/tests/test_ryuk.py                   │ 2      │
│ core/tests/test_registry.py               │ 1      │
│ core/tests/test_image.py                  │ 3      │
│ core/tests/test_compose.py                │ 7      │
└───────────────────────────────────────────┴────────┘
Found 104 errors in 14 files.

codecov · 2024-09-16T06:03:08Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (main@ace2a7d). Learn more about missing BASE report.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #702   +/-   ##
=======================================
  Coverage        ?   85.37%           
=======================================
  Files           ?       12           
  Lines           ?      670           
  Branches        ?      105           
=======================================
  Hits            ?      572           
  Misses          ?       75           
  Partials        ?       23

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Tranquility2 · 2024-11-16T09:39:06Z

Please note:

core/testcontainers/core/config.py:46: error: X | Y syntax for unions requires Python 3.10  [syntax]
        connection_mode: str | None = environ.get("TESTCONTAINERS_CONNECTION_MODE")

Found when rebasing and updated to support 3.9 (I'm not aware of deprecating 3.9 in tc-python, feel free to update)

Tranquility2 · 2024-12-14T18:29:27Z

@alexanderankin @kiview can you please rerun this? (I assume this was an issue with the docker registry)

CarliJoy

Please use cast only when required (i.e. for dict[str, Any]).

If possible always improve the runtime type safety.

CarliJoy · 2025-02-28T12:35:39Z

core/testcontainers/core/docker_client.py


    def bridge_ip(self, container_id: str) -> str:
        """
        Get the bridge ip address for a container.
        """
        container = self.get_container(container_id)
        network_name = self.network_name(container_id)
-        return container["NetworkSettings"]["Networks"][network_name]["IPAddress"]
+        return cast(str, container["NetworkSettings"]["Networks"][network_name]["IPAddress"])


Suggested change

return cast(str, container["NetworkSettings"]["Networks"][network_name]["IPAddress"])

return str(container["NetworkSettings"]["Networks"][network_name]["IPAddress"])

Ensures the typing instead of suggesting it.
It more readable and more runtime safe.

CarliJoy · 2025-02-28T12:38:09Z

core/testcontainers/core/docker_client.py

@@ -190,15 +190,15 @@ def network_name(self, container_id: str) -> str:
        name = container["HostConfig"]["NetworkMode"]
        if name == "default":
            return "bridge"
-        return name
+        return cast(str, name)


Instead of

container = self.get_container(container_id) name = container["HostConfig"]["NetworkMode"] if name == "default": return "bridge" return cast(str, name)

Use

container = self.get_container(container_id) name = str(container["HostConfig"]["NetworkMode"]) if name == "default": return "bridge" return name

It is more runtime safe

CarliJoy · 2025-02-28T12:38:39Z

core/testcontainers/core/docker_client.py


    def gateway_ip(self, container_id: str) -> str:
        """
        Get the gateway ip address for a container.
        """
        container = self.get_container(container_id)
        network_name = self.network_name(container_id)
-        return container["NetworkSettings"]["Networks"][network_name]["Gateway"]
+        return cast(str, container["NetworkSettings"]["Networks"][network_name]["Gateway"])


Suggested change

return cast(str, container["NetworkSettings"]["Networks"][network_name]["Gateway"])

return str(container["NetworkSettings"]["Networks"][network_name]["Gateway"])

CarliJoy · 2025-02-28T12:49:53Z

core/testcontainers/core/docker_client.py

@@ -237,7 +237,7 @@ def host(self) -> str:
            # see https://github.com/testcontainers/testcontainers-python/issues/415
            if url.hostname == "localnpipe" and utils.is_windows():
                return "localhost"
-            return url.hostname
+            return cast(str, url.hostname)


Instead of

if url.hostname == "localnpipe" and utils.is_windows(): return "localhost" return cast(str, url.hostname)

use

hostname = url.hostname if not hostname or (hostname == "localnpipe" and utils.is_windows()): return "localhost" return url.hostname

Don't cast a possible None into a str it is not runtime safe.
Instead handle the case correctly.

(Sorry suggestion in Gitlab don't work good if you want to suggest a larger change)

CarliJoy · 2025-02-28T12:57:25Z

core/testcontainers/core/config.py

@@ -19,7 +19,7 @@ def use_mapped_port(self) -> bool:

        This is true for everything but bridge mode.
        """
-        if self == self.bridge_ip:
+        if cast(str, self) == self.bridge_ip:


Suggested change

if cast(str, self) == self.bridge_ip:

if self == self.bridge_ip:

That is neither required nor correct.

The original code does not issue any Typing Errors:
Not in mypy nor pyright

Tranquility2 · 2025-03-08T22:32:34Z

@CarliJoy apricate you taking the time to comment :) I'll be happy to fix and update as suggested.
I assume/hope #700 & #701 will go in first so I'll know we are really rolling with this track.

fix(core): Typed docker_client

d414f91

Tranquility2 changed the title ~~fix(core): Typed docker_client~~ fix(core): Typing in docker_client Sep 16, 2024

Merge branch 'main' into typed_docker_client

4b024af

Tranquility2 mentioned this pull request Oct 2, 2024

feat(core): add buildargs to DockerImage #614

Closed

Tranquility2 added 2 commits October 14, 2024 19:04

Merge branch 'main' into typed_docker_client

eede4e7

Merge branch 'main' into typed_docker_client

a35a7b2

alexanderankin added the intend to merge this or some version of it label Nov 11, 2024

Merge branch 'main' into typed_docker_client

cf6ca6b

Tranquility2 closed this Nov 16, 2024

Tranquility2 reopened this Nov 16, 2024

Tranquility2 added 2 commits November 16, 2024 09:42

Fix issues from recent merges

b8e76bc

Merge branch 'main' into typed_docker_client

7189955

Merge branch 'main' into typed_docker_client

f4b0d0a

CarliJoy suggested changes Feb 28, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(core): Typing in docker_client #702

fix(core): Typing in docker_client #702

Tranquility2 commented Sep 16, 2024

codecov bot commented Sep 16, 2024 •

edited

Loading

Tranquility2 commented Nov 16, 2024 •

edited

Loading

Tranquility2 commented Dec 14, 2024

CarliJoy left a comment

CarliJoy Feb 28, 2025

CarliJoy Feb 28, 2025

CarliJoy Feb 28, 2025

CarliJoy Feb 28, 2025

CarliJoy Feb 28, 2025

Tranquility2 commented Mar 8, 2025 •

edited

Loading

	return cast(str, container["NetworkSettings"]["Networks"][network_name]["IPAddress"])
	return str(container["NetworkSettings"]["Networks"][network_name]["IPAddress"])

	if cast(str, self) == self.bridge_ip:
	if self == self.bridge_ip:

fix(core): Typing in docker_client #702

Are you sure you want to change the base?

fix(core): Typing in docker_client #702

Conversation

Tranquility2 commented Sep 16, 2024

codecov bot commented Sep 16, 2024 • edited Loading

Codecov Report

Tranquility2 commented Nov 16, 2024 • edited Loading

Tranquility2 commented Dec 14, 2024

CarliJoy left a comment

Choose a reason for hiding this comment

CarliJoy Feb 28, 2025

Choose a reason for hiding this comment

CarliJoy Feb 28, 2025

Choose a reason for hiding this comment

CarliJoy Feb 28, 2025

Choose a reason for hiding this comment

CarliJoy Feb 28, 2025

Choose a reason for hiding this comment

CarliJoy Feb 28, 2025

Choose a reason for hiding this comment

Tranquility2 commented Mar 8, 2025 • edited Loading

codecov bot commented Sep 16, 2024 •

edited

Loading

Tranquility2 commented Nov 16, 2024 •

edited

Loading

Tranquility2 commented Mar 8, 2025 •

edited

Loading