feat(core): add support for direct Dockerfile builds #455
Conversation
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)
Redefines the approach to building and managing Docker images within testcontainers by introducing the DockerImage class. This class encapsulates the logic for building images from Dockerfiles and pulling images from repositories. Key Changes: - Implements DockerImage as a central class for image operations, including build, pull, get, and remove. - Add this class as acceptable image param type for DockerContainer and DockerClient - Enables direct Dockerfile support while preserving the option to pull existing images, facilitating a more dynamic testing setup. This refactor addresses feedback on the initial implementation, proposing a cleaner, more extensible design.
|
Hello @alexanderankin ! Just to notice you that I cleaned up my commit history then I only have one commit now. |
totallyzen
changed the title
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.
| 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?
| @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
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
| @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.
| 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...
| self.env = {} | ||
| 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 insideDockerImage
Feature: Introduce DockerImage class for flexible image handling
Overview:
This PR introduces the
DockerImageclass, aimed at simplifying the way testcontainers handles Docker images. DockerContainer now support for the attribute image:DockerImageandstr.Motivation:
Implementation Details:
from_dockerfile,from_image,build,pull,get, andremove. This design simplifies the interface for users, making it straightforward to build or pull images as needed.fixes #83