-
Notifications
You must be signed in to change notification settings - Fork 339
feat(compose): add structured container inspect information #897
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?
feat(compose): add structured container inspect information #897
Conversation
ok so i have just one small issue with this and its that i thought we wanted to give the same information from compose and container about the container - so i think these same data classes need to be returned from "DockerContainer" class which is in core package too, right? I think if we can do that, we will necessarily find these dataclasses a better home as well, which i think needs to be close to the docker client (not the docker python library but the docker client abstraction in this library). looks very promising so far. |
i can review java as well, i have not done that before writing the above comment. |
Yes, you’re right, my first thought was to add it to compose.py, since that’s where I noticed the container state information was missing. However, in Java, the implementation is actually in a generic container interface called GenericContainer, and later the ComposeContainer extends that class. So you’re absolutely correct , I’ll move it to the core module. However, I’m unsure about the best place to put the dataclasses. Given the files we currently have, these models don’t seem to fit perfectly anywhere. Should we perhaps create a separate file for them, something like models.py or similar? Or maybe place them in utils.py instead? |
I was thinking here tbh https://github.com/testcontainers/testcontainers-python/blob/main/core/testcontainers/core/docker_client.py assuming these are models of API objects as described by the docker api https://docs.docker.com/reference/api/engine/version/v1.51/ |
Okay, good to know, working on that, thanks. |
Hi, now is full compliant with docker api response and all dataclasses are in docker_client.py |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #897 +/- ##
==========================================
+ Coverage 79.78% 83.76% +3.98%
==========================================
Files 14 14
Lines 1182 1583 +401
Branches 184 237 +53
==========================================
+ Hits 943 1326 +383
- Misses 197 209 +12
- Partials 42 48 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
this is amazing - adding those data classes takes a huge step forward to moving us away from the python docker client and we cannot add enough distance between us. if we can incorporate more of these data classes and really minimize the API items we inherited from the python client - can probably get much closer to a uniform implementation with other languages which are more stable than this one. thank you for your work on this!!!
ExitCode: Optional[int] = None | ||
Publishers: list[PublishedPortModel] = field(default_factory=list) | ||
_docker_compose: Optional["DockerCompose"] = field(default=None, init=False, repr=False) | ||
_cached_container_info: Optional[ContainerInspectInfo] = field(default=None, init=False, repr=False) |
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.
do we need to care about invalidating?
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.
This is something I was thinking about, and I guess the answer depends, of course, network conditions, container status, and statistics can change along the way. But in a test scenario, you would usually create the container, run your tests, and then destroy it. I see that this new functionality would be used to collect the status at the end of the test, so this simple cache helps avoid extra overhead during test execution. What do you think?
Otherwise, we have a few options here. The easiest one would be to always execute the docker inspect command and return fresh information, or we could define a default TTL for the data so that after this period, the next call retrieves updated data or something like this.
i will try to look at this within a week or so, currently (not very) sick and busy with work |
Take all the time you need, I wish you a speedy recovery! |
Summary
This PR adds the ability to retrieve detailed container information from docker inspect in a structured format for ComposeContainer objects, with lazy loading and caching for optimal performance.
Related with #857