-
Notifications
You must be signed in to change notification settings - Fork 375
Conversation
alexanderankin
commented
Aug 12, 2024
i usually prefer using typing.Union[str, os.PathLike] - if you wanna use strings with this as well as paths
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@ ## main #676 +/- ## ======================================= Coverage ? 76.44% ======================================= Files ? 12 Lines ? 624 Branches ? 96 ======================================= Hits ? 477 Misses ? 120 Partials ? 27 ☔ View full report in Codecov by Sentry. |
alexanderankin
commented
Aug 12, 2024
one other thing to consider is that there is this great interface "Transferable" in other language implementations-- does this PR make it harder or easier to add this to this implementation as well.
the interface (found here) is basically:
class Transferable(abc.ABC): @abstractmethod def transfer_to(content_stream, destination: typing.Union[str, os.PathLike]): ...
where content_stream is some bytesio or something pythonic for a place where you can write bytes to.
alexanderankin
commented
Aug 12, 2024
when i tried to do it, i accidentally interpreted the interface a bit too widely and instead of passing just the content output stream, i passed the whole container as that first argument.
4b7db74 - misc functions, next commit doesnt make sense without reading first
bbc44ec - the commit where i add the transferable stuff
alexanderankin
commented
Aug 12, 2024
so im tiny bit hesitant to merge without a plan for adding transferable on top of this
do we need it to be 1:1 with java implementation though? any binary object now can be passed here using tempfile.NamedTemporaryObject:
with tempfile.NamedTemporaryObject() as f:
f.write(binary_data)
DockerContainer("nginx").with_copy_file_to_container(f.name, "/tmp/target.data").start()
alexanderankin
commented
Aug 12, 2024
we dont need to be 1-1 but what if instead of a file you had a string? i think it is beneficial to not force people to create temporary files.
refactored for Transferable although creating temp files cannot be avoided, now instead of asking users for 3 extra lines we add some complexity to codebase here. working example
input:
from pathlib import Path
from testcontainers.core.container import DockerContainer, Transferable
from testcontainers.core.network import Network
c = DockerContainer('nginx')
network = Network()
c \
.with_copy_file_to_container(Transferable(b'asd', Path('/tmp/mariusz.md1'))) \
.with_copy_file_to_container(Transferable(Path('/Users/mariusz/Documents/Programming/testcontainers-python/README.md'),Path('/tmp/mariusz.md2'))) \
.start()
c2 = DockerContainer('nginx')
print(c._container.exec_run('head /tmp/mariusz.md1'))
print(c._container.exec_run('head /tmp/mariusz.md2'))
output
ExecResult(exit_code=0, output=b'asd')
ExecResult(exit_code=0, output=b'[](https://python-poetry.org/)\n[](https://github.com/astral-sh/ruff)\n\n[](https://github.com/testcontainers/testcontainers-python/blob/main/LICENSE)\n[](https://pypi.python.org/pypi/testcontainers)\n[](https://codecov.io/gh/testcontainers/testcontainers-python)\n\n\n[](http://testcontainers-python.readthedocs.io/en/latest/?badge=latest)\n\n')
Lenormju
commented
Aug 12, 2024
@mgorsk1 I have opened #677 to solve the same issue (I did not see you already had opened one 😓).
Yours have the Transferable feature. But I have the read_only feature.
And they are not implemented the same way :
- I am passing the paths in the
HostConfig.Bindto the docker_client.create (so the file exist before the container starts) - You are writing the files using the
docker.models.containers.Container.put_archivemethod (which works after the container has been started)
Are our two approaches complementary ?
Reading again the Java documentation for copying files, there does not seem to be a difference, at the API level at least, between copying to a container before or after its startup.
I think copying before can already be solved by with_volume_mapping 😅
Signed-off-by: mgorsk1 <gorskimariusz13@gmail.com>
Signed-off-by: mgorsk1 <gorskimariusz13@gmail.com>
Signed-off-by: mgorsk1 <gorskimariusz13@gmail.com>
Signed-off-by: mgorsk1 <gorskimariusz13@gmail.com>
Signed-off-by: mgorsk1 <gorskimariusz13@gmail.com>
Signed-off-by: mgorsk1 <gorskimariusz13@gmail.com>
e5a2325 to
a2f6094
Compare
Signed-off-by: mgorsk1 <gorskimariusz13@gmail.com>
Signed-off-by: mgorsk1 <gorskimariusz13@gmail.com>
alexanderankin
commented
Aug 13, 2024
committed some code here - main...feat/665-with-copy-file-to-container
alexanderankin
commented
Aug 13, 2024
alexanderankin
commented
Aug 13, 2024
since this is not a high priority feature this can probably wait for 1) more api refinement - feedback welcomed on my suggestions above 2) more of my free time, as I'm not sure ill be able to spend too much more time on this this week (just spent my entire week's time budget on this today).
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 found this PR noticing that copying files isn't supported, a bit surprising since it's a pretty important one and found in at least many of the other languages.
I noticed this implementation doesn't seem to match others, though this is probably because of how the general docker running is implemented. Generally, the container is created, then files copied, then the container started, so the files are present when the command runs.
Here, the files are just copied after startup so can't be used by the entrypoint - perhaps there's some use case for this but I don't know how common it is. Note that volume mounting is an alternative but doesn't always work due to e.g. permission mode issues - copying is generally more robust.
Given the behavior would be so different from other implementations of testcontainers, I think this PR can cause a lot of confusion so probably needs to rework the lifecycle like that.
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.
Making this happen during start doesn't feel like the right interface here. While there's definitely good reasons to copy in files just after it starts, as proposed here that is the only time we can copy files into the container.
I'd propose instead creating a copy_file_into_container, which would basically be the inverse of copy_file_from_container proposed above. It could be implemented by simply calling get_archive, but provide an interface which used os.PathLike.
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.
IIUC, the temporary file could be avoided if using tar.addfile - it is somewhat more tedious to set up a TarInfo but maybe worth it to skip the temp file
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.
Also, I think all the copy_file APIs in testcontainers accept a file mode. With the current pattern, it can be something like filter=lambda tarinfo: tarinfo.mode = foo, but if switching to addfile it could be set directly to the TarInfo
In #676 for copying to/from `DockerContainer`, it was suggested that we should clarify the interface & usage a bit before proceeding. This PR aims to push that conversation forward with some test cases illustrating proposed usages. Although here I created a `Transferable` object, in most cases there's no need for the caller to explicitly construct a Transferable, just pass the `bytes | Path` **Proposed use cases:** 1. Copy into a container by passing a `TransferSpec` into the initializer: ``` DockerContainer(... transferrables=[TransferSpec(source, destination_in_container), ...) ``` 2. Copy into the container via the builder pattern, prior to starting the container: ``` DockerContainer(...) .with_copy_into_container(b"some_bytes", destination_in_container) .with_copy_into_container(some_path, destination_in_container) ``` 3. Copy into the container at runtime: ``` with DockerContainer(...) as container: container.copy_into_container(b"some_bytes", destination_in_container) container.copy_into_container(some_path, destination_in_container) ``` --------- Co-authored-by: Roy Moore <roy@moore.co.il>
Tranquility2
commented
Apr 1, 2026
Hi, just a quick checkup, is this still relevant now that we have container.copy_into_container(my_file, "/tmp/my_file.txt") ready?
solves #665