-
Notifications
You must be signed in to change notification settings - Fork 375
feat(core): add mount files when starting a container#677
Conversation
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.
I think it overlaps quite a lot with with_volume_mapping no?
alexanderankin
commented
Aug 13, 2024
the difference between mounting and copying is that one uses the linux file system, and the other uses a dedicated docker api where you can PUT a tar archive and docker engine un-tars for you inside the virtual file system of a running container, i think this addition blurs that distinction. going to leave open while we sort out a solution
anuraaga
commented
Jan 30, 2025
Found this wishing for copying, related comment in #676 (comment)
Agree that mounting isn't copying, so the method shouldn't be called with_copy_file_to_container - while it could make sense as something like with_mount to make sure it's not confused with copying, since we already have both volume mapping and with_kwargs to use the mount API, I feel it isn't all that hard for a user to do it already without extra API.
oelhammouchi
commented
Feb 15, 2025
Do you still intend to move forward with this? I really need a way of inputting a config file into a test container.
Lenormju
commented
Feb 17, 2025
@oelhammouchi look at the code I wrote for with_copy_file_to_container, it is just one method that you can add in your project.
anuraaga
commented
Feb 17, 2025
@oelhammouchi If you're asking since you need actual copying instead of mounting, here's my current workaround. Note it relies on the container crashing when the file isn't present, which is probably common (in this case, it crashes while creds.json is being copied.
# Roughly based on proposed logic in https://github.com/testcontainers/testcontainers-python/pull/676/files def copy_file_to_container(container: DockerContainer, host_path: str, container_path: str, mode: int): data = BytesIO() def set_mode(tarinfo: tarfile.TarInfo): tarinfo.mode = mode return tarinfo with tarfile.open(fileobj=data, mode='w') as tar: tar.add(host_path, arcname=container_path, filter=set_mode) data.seek(0) if not container._container.put_archive('/', data): raise Exception('Failed to copy file to container') def main(): ... with ( DockerContainer('gcr.io/cloud-sql-connectors/cloud-sql-proxy:2.14.3') .with_command(command) .with_exposed_ports(5432, 9090) .with_env(environment_vars.CREDENTIALS, '/creds/creds.json') # Currently there is no way to copy a file before container starts, so we workaround by allowing the container # to restart on failure, which will stop when the copy has completed. # https://github.com/testcontainers/testcontainers-python/pull/676#discussion_r1934888327 .with_kwargs(restart_policy={'Name': 'on-failure', 'MaximumRetryCount': 100}) ) as container: copy_file_to_container(container, creds_path, '/creds/creds.json', 0o644)
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.
Even if this PR moves forward, I'd strongly advise against this name. This is mounting, not copying
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.
I agree. But I seem to recall there were discussions about following the Java conventions (or not).
alexanderankin
commented
Apr 30, 2026
if this is still needed (something is possible with this code and not possible with the version on the main branch) lets open a new pr with required changes
Lenormju
commented
May 4, 2026
if this is still needed (something is possible with this code and not possible with the version on the main branch) lets open a new pr with required changes
As discussed in the linked issue (#665), this was not the right approach. Also, there is now a solution to do what I wanted (cf #665 (comment)).
So yeah let's forget this one.
I have created the issue #665 on which I proposed to implement it.
I have read the contribution guidelines.
The feature works fine, as you can see by the tests.
But I have some doubts about my solution.
I made some decisions that you may not agree with, so I'd like to have your feedback whether or not it suits you, so that I can fix this PR if required.
core/testcontainers/core/container.pyI addedfrom docker.types.services import Mount. I noticed that thetestcontainerspackage had very few imports of thedockerpackage. Is it OK to do it ? Or should it be made the same way than *volumes* (i.e. do not use the type defined in thedocker.types` package, create an untyped dict instead) ?core/tests/test_core.pyI addedfrom testcontainers.core.container import docker, whosedockeris thedockerpackage, is it OK ?core/tests/test_core.pytest file. Is it OK to add all of them ?DockerContainer.with_copy_file_to_containerincore/testcontainers/core/container.py) accepts its paths arguments as bothstrandpathlib.Path(thanks toUnion). I think it makes for better APIs (because there is no need to convert before to call them, they handle it). But that was not the way it was done in the rest of the file. Is it OK to do it like that ? Or should I change to accept onlystrs ?Thanks in advance 😃