FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

feat(core): add mount files when starting a container by Lenormju · Pull Request #677 · testcontainers/testcontainers-python · GitHub

/ testcontainers-python Public

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

Closed
Lenormju wants to merge 1 commit into
testcontainers:mainfrom
Lenormju:665-feature-add-dockercontainer-with-copy-file-to-container
ConversationCommits (1)ChecksFiles changed
Closed

feat(core): add mount files when starting a container#677
Lenormju wants to merge 1 commit into
testcontainers:mainfrom
Lenormju:665-feature-add-dockercontainer-with-copy-file-to-container

Conversation

Lenormju commented Aug 12, 2024

Copy link
Copy Markdown

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.

  • 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 mentioned this pull request Aug 12, 2024
Open
mgorsk1 reviewed Aug 13, 2024
View reviewed changes

mgorsk1 left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think it overlaps quite a lot with with_volume_mapping no?

alexanderankin commented Aug 13, 2024

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown

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

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Author

@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

Copy link
Copy Markdown

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

rhoban13 reviewed May 27, 2025
View reviewed changes
Comment thread core/testcontainers/core/container.py
self.volumes[host] = mapping
return self

def with_copy_file_to_container(

rhoban13 May 27, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Even if this PR moves forward, I'd strongly advise against this name. This is mounting, not copying

Lenormju May 27, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I agree. But I seem to recall there were discussions about following the Java conventions (or not).

alexanderankin commented Apr 30, 2026

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Author

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.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers
mgorsk1 mgorsk1 left review comments
rhoban13 rhoban13 left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

👀 intend to merge this or some version of it

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL