Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Lenormju
Copy link

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 😃

Copy link
Contributor

@mgorsk1 mgorsk1 left a 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
Copy link
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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants