testcontainers-python
testcontainers-python copied to clipboard
feat(core): add mount files when starting a container
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 addedfrom docker.types.services import Mount
. I noticed that thetestcontainers
package had very few imports of thedocker
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 addedfrom testcontainers.core.container import docker
, whosedocker
is thedocker
package, is it OK ?
- [ ] on the same subject, in
- [ ] 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
incore/testcontainers/core/container.py
) accepts its paths arguments as bothstr
andpathlib.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 onlystr
s ?
Thanks in advance 😃