Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

fix: get_wrapped_container returns container all the time now#906

Open
alexanderankin wants to merge 1 commit into
main from
fix/less-crazy-get_wrapped_container
Open

fix: get_wrapped_container returns container all the time now #906
alexanderankin wants to merge 1 commit into
main from
fix/less-crazy-get_wrapped_container

Conversation

@alexanderankin

@alexanderankin alexanderankin commented Oct 15, 2025

Copy link
Copy Markdown
Member

fix #905

Copy link
Copy Markdown
Member Author

ok, have to think about this one more carefully. seems like there is more to this than appears

docker_compose_cmd += ["--env-file", env_file]
return docker_compose_cmd

def _get_docker_client(self) -> DockerClient:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def _get_docker_client(self) -> DockerClient:
def _get_docker_client(self) -> DockerClient:
if self._docker_client is None:
self._docker_client = DockerClient(**(self.docker_client_kw or {}))
return self._docker_client

def get_wrapped_container(self) -> Container:
"""Get the underlying container object for compatibility."""
return self
return self._docker_compose._get_docker_client().containers.get(self.ID)

@surister surister Oct 16, 2025
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dumb question: can .containers.get(self.ID) return a value other than Container, e.g. None?

what if the container is not found?

alexanderankin commented Oct 16, 2025 via email

Copy link
Copy Markdown
Member Author
it throws docker api exception or something. its in the docstr of the get method. just navigate to it and you should see it also
...
On Thu, Oct 16, 2025 at 8:58 AM Ivan ***@***.***> wrote: ***@***.**** approved this pull request. ------------------------------ In core/testcontainers/compose/compose.py <#906 (comment)> : > @@ -259,6 +263,13 @@ def compose_command_property(self) -> list[str]: docker_compose_cmd += ["--env-file", env_file] return docker_compose_cmd + def _get_docker_client(self) -> DockerClient: ⬇️ Suggested change - def _get_docker_client(self) -> DockerClient: + def _get_docker_client(self) -> DockerClient: + if self._docker_client is None: + self._docker_client = DockerClient(**(self.docker_client_kw or {})) + return self._docker_client ------------------------------ In core/testcontainers/compose/compose.py <#906 (comment)> : > """Get the underlying container object for compatibility.""" - return self + return self._docker_compose._get_docker_client().containers.get(self.ID) Dumb question: can .containers.get(self.ID) return a value other than Container, e.g. None? what if the container is not found? — Reply to this email directly, view it on GitHub <#906 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACECGJDPPFF3B2BECIZBBUD3X6I6RAVCNFSM6AAAAACJIPB3Z2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTGNBUG4YTAMBUHA> . You are receiving this because you authored the thread.Message ID: ***@***.*** com>

...

def get_wrapped_container(self) -> Any:
def get_wrapped_container(self) -> Container:

@jankatins jankatins Oct 18, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️

@alexanderankin alexanderankin Oct 18, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We recently had a PR that added a bunch of data classes, I think I should actually be returning one of those here

@Tranquility2 Tranquility2 added 🔧 fix 🛠️ needs more work Need to invest more time, can be a rebase or code updates labels Apr 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@jankatins jankatins jankatins left review comments

@surister surister surister approved these changes

Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

🔧 fix 🛠️ needs more work Need to invest more time, can be a rebase or code updates

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Bug: HealthcheckWaitStrategy returns error with DockerCompose

AltStyle によって変換されたページ (->オリジナル) /