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@f979525). Learn more about missing BASE report.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #702   +/-   ##
=======================================
  Coverage        ?   85.35%           
=======================================
  Files           ?       12           
  Lines           ?      669           
  Branches        ?      105           
=======================================
  Hits            ?      571           
  Misses          ?       75           
  Partials        ?       23

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

🚀 New features to boost your workflow:

❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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

Super nice, fixed

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)

This is great, but url.hostname is still "Any" and we expects "str".
but I totally agree with the possible None so the logic improvement is critical 🙏

(No need to apologize, trust me you have done above and beyond)

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

@CarliJoy Mypy seems to disagree:

Update to latest mypy, still the same behavior
mypy 1.15.0 (compiled: yes)

Found the issue :)
under "[tool.mypy]" on pyproject.toml
we have strict = true

I'll be adding # type: ignore[comparison-overlap] in the relevant locations (future PRs) @CarliJoy

if self == ConnectionMode.bridge_ip:

Seems to be the type correct solution.

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.

Tranquility2 · 2025-03-15T10:59:48Z

@CarliJoy Updated with all of the Fixes

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

Tranquility2 mentioned this pull request Oct 2, 2024

feat(core): add buildargs to DockerImage #614

Closed

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

Tranquility2 closed this Nov 16, 2024

Tranquility2 reopened this Nov 16, 2024

CarliJoy suggested changes Feb 28, 2025

View reviewed changes

Tranquility2 added 4 commits March 15, 2025 10:05

fix(core): Typed docker_client

b61a684

Fix issues from recent merges

7d71ca6

Fix comparison-overlap check for config

db625e8

CR fixes

59372c6

Tranquility2 force-pushed the typed_docker_client branch from f4b0d0a to 59372c6 Compare March 15, 2025 10:53

Tranquility2 requested a review from CarliJoy March 15, 2025 10:58

Merge branch 'main' into typed_docker_client

ea6aa92

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

Tranquility2 Mar 15, 2025

CarliJoy Feb 28, 2025

Tranquility2 Mar 15, 2025 •

edited

Loading

CarliJoy Feb 28, 2025

Tranquility2 Mar 15, 2025

CarliJoy Feb 28, 2025

Tranquility2 Mar 15, 2025 •

edited

Loading

Tranquility2 Mar 15, 2025

CarliJoy Feb 28, 2025

Tranquility2 Mar 15, 2025

Tranquility2 Mar 15, 2025 •

edited

Loading

Tranquility2 Mar 15, 2025 •

edited

Loading

Tranquility2 Mar 15, 2025

CarliJoy Mar 15, 2025

Tranquility2 commented Mar 8, 2025 •

edited

Loading

Tranquility2 commented Mar 15, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tranquility2 Mar 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tranquility2 Mar 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tranquility2 Mar 15, 2025 • edited Loading

Choose a reason for hiding this comment

Tranquility2 Mar 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tranquility2 commented Mar 8, 2025 • edited Loading

Tranquility2 commented Mar 15, 2025 • edited Loading

codecov bot commented Sep 16, 2024 •

edited

Loading

Tranquility2 commented Nov 16, 2024 •

edited

Loading

Tranquility2 Mar 15, 2025 •

edited

Loading

Tranquility2 Mar 15, 2025 •

edited

Loading

Tranquility2 Mar 15, 2025 •

edited

Loading

Tranquility2 Mar 15, 2025 •

edited

Loading

Tranquility2 commented Mar 8, 2025 •

edited

Loading

Tranquility2 commented Mar 15, 2025 •

edited

Loading