feat(core): add support for direct Dockerfile builds - #455
Conversation
Sorry, something went wrong.
alexanderankin
left a comment
There was a problem hiding this comment.
im not sure this is the highest priority at the moment but its a solid start and im sure with some other concepts (we can discuss in the issue to keep the discussion centralized) we can find something which is accepted.
this is a very useful contribution either way to gauge the effort level it takes - it seems very much within reach.
please see #83 (comment)
Sorry, something went wrong.
|
Hello @alexanderankin ! Just to notice you that I cleaned up my commit history then I only have one commit now. |
Sorry, something went wrong.
alexanderankin
left a comment
There was a problem hiding this comment.
some misc thoughts - it really is like me thinking out loud because im not actually sure my comments are improvements until i implement it locally or see the code to compare. just going over all the PRs while i dont have the energy to dig into implementations.
Sorry, something went wrong.
| def from_image(self, repository: str, tag: str = "latest") -> "DockerImage": | ||
| """ | ||
| Pull an image from the registry. | ||
|
|
||
| Args: | ||
| repository (str): Image repository | ||
| tag (str): Image tag | ||
|
|
||
| Returns: | ||
| DockerImage: The current instance | ||
| """ | ||
| self.pull(repository=repository, tag=tag) | ||
| return self |
There was a problem hiding this comment.
it probably doesn't need to pull every time - can we make that configurable?
Sorry, something went wrong.
| @ft.wraps(ImageCollection.get) | ||
| def get(self, image: str) -> Image: | ||
| LOGGER.info(f"Getting image {image}") | ||
| image_obj = self._docker.images.get(image) | ||
| return image_obj | ||
|
|
||
| @ft.wraps(ImageCollection.remove) | ||
| def remove(self, **kwargs) -> None: | ||
| LOGGER.info(f"Removing image {self._image}") | ||
| self._image.remove(**kwargs) |
There was a problem hiding this comment.
this seems like a lot of stuff that is neat but may be painful to maintain - if this were java i would say lets annotate it with "incubating" or some other variant of message that this is a use at your own risk API. can we add a doc string comment here instead saying that this is an "incubating" API? i think this makes maintenance of this code much less intimidating
Sorry, something went wrong.
There was a problem hiding this comment.
same for exists... if its used anywhere, make it private, if not used - should come with big hazard warning that says it may go away
Sorry, something went wrong.
| @ft.wraps(ImageCollection.build) | ||
| def build(self, **kwargs) -> "DockerImage": | ||
| LOGGER.info("Building image from Dockerfile") | ||
| self._image, _ = self._docker.images.build(**kwargs) | ||
| return self |
There was a problem hiding this comment.
this should probably be a private method to abstract away the detail that sometimes it is a dockerfile and other times it is a docker image that the container runtime would pull.
Sorry, something went wrong.
| def __init__(self, docker_client_kw: Optional[dict] = None, **kwargs) -> None: | ||
| self._docker = DockerClient().from_env(**(docker_client_kw or {})) | ||
|
|
||
| def from_dockerfile(self, path: str, tag: str = "local/image") -> "DockerImage": |
There was a problem hiding this comment.
lets not make it dependent on an image, it should be possible to just use the image id, i think, both based on the java implementation link and also based on experience running docker build and forgetting to specify -t myimage - you can grab the id and tag with a separate command. not sure how feasible this is but without this, seems like it would be quite a limitation on concurrency...
Sorry, something went wrong.
| self.ports = {} | ||
| self.volumes = {} | ||
| self.image = image | ||
| self.image = image.get_wrapped_image() if isinstance(image, DockerImage) else image |
There was a problem hiding this comment.
can we simplify this logic to be:
- if string, construct a DockerImage
- get a string which can be passed to docker run
and try to contain the conditionals inside DockerImage
Sorry, something went wrong.
|
Is this obsolete now that we have #585 ? |
Sorry, something went wrong.
|
I haven't had time to go through and determine just how far away this and the other implementation is from my ideal version of it (also haven't had time to formulate this) so I am leaving open in case any ideas that shouldn't get lost. If I should close, I can close. |
Sorry, something went wrong.
Sorry, something went wrong.
Feature: Introduce DockerImage class for flexible image handling
Overview:
This PR introduces the DockerImage class, aimed at simplifying the way testcontainers handles Docker images. DockerContainer now support for the attribute image: DockerImage and str.
Motivation:
Implementation Details:
fixes #83