testcontainers-python icon indicating copy to clipboard operation
testcontainers-python copied to clipboard

feat(core): add mount files when starting a container

Open Lenormju opened this issue 1 year ago • 5 comments

I have created the issue testcontainers/testcontainers-python#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.

  • [ ] in core/testcontainers/core/container.py I added from docker.types.services import Mount. I noticed that the testcontainers package had very few imports of the docker package. 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 the docker.types` package, create an untyped dict instead) ?
    • [ ] on the same subject, in core/tests/test_core.py I added from testcontainers.core.container import docker, whose docker is the docker package, is it OK ?
  • [ ] I have added 7 tests, with long and descriptive names. There ware few tests before in the core/tests/test_core.py test file. Is it OK to add all of them ?
  • [ ] For the tests themselves, they globally follow the same pattern : create a short and simple Dockerfile, create some files, start the container, and check that the file copy has been correctly set up. But to me, they loog a bit too long and verbose. Do you think I should try to re-factorize them in a smaller size ?
  • [ ] The new method (DockerContainer.with_copy_file_to_container in core/testcontainers/core/container.py) accepts its paths arguments as both str and pathlib.Path (thanks to Union). 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 only strs ?

Thanks in advance 😃

Lenormju avatar Aug 12 '24 22:08 Lenormju

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

alexanderankin avatar Aug 13 '24 12:08 alexanderankin

Found this wishing for copying, related comment in https://github.com/testcontainers/testcontainers-python/pull/676#discussion_r1934888327

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.

anuraaga avatar Jan 30 '25 01:01 anuraaga

Do you still intend to move forward with this? I really need a way of inputting a config file into a test container.

oelhammouchi avatar Feb 15 '25 13:02 oelhammouchi

@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.

Lenormju avatar Feb 17 '25 07:02 Lenormju

@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)

anuraaga avatar Feb 17 '25 07:02 anuraaga