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: add copying files to/from container #676

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

mgorsk1
Copy link
Contributor

@mgorsk1 mgorsk1 commented Aug 12, 2024

solves #665

@alexanderankin
Copy link
Member

i usually prefer using typing.Union[str, os.PathLike] - if you wanna use strings with this as well as paths

Copy link

codecov bot commented Aug 12, 2024

Codecov Report

Attention: Patch coverage is 32.00000% with 17 lines in your changes missing coverage. Please review.

Please upload report for BASE (main@1e43923). Learn more about missing BASE report.

Files Patch % Lines
core/testcontainers/core/container.py 32.00% 16 Missing and 1 partial ⚠️
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.
📢 Have feedback on the report? Share it here.

@alexanderankin
Copy link
Member

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
Copy link
Member

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
Copy link
Member

so im tiny bit hesitant to merge without a plan for adding transferable on top of this

@mgorsk1
Copy link
Contributor Author

mgorsk1 commented Aug 12, 2024

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
Copy link
Member

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.

@mgorsk1
Copy link
Contributor Author

mgorsk1 commented Aug 12, 2024

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'[![Poetry](https://img.shields.io/endpoint?url=https://python-poetry.org/badge/v0.json)](https://python-poetry.org/)\n[![Ruff](https://img.shields.io/endpoint?url=https://raw.githubusercontent.com/astral-sh/ruff/main/assets/badge/v2.json)](https://github.com/astral-sh/ruff)\n![PyPI - Version](https://img.shields.io/pypi/v/testcontainers)\n[![PyPI - License](https://img.shields.io/pypi/l/testcontainers.svg)](https://github.com/testcontainers/testcontainers-python/blob/main/LICENSE)\n[![PyPI - Python Version](https://img.shields.io/pypi/pyversions/testcontainers.svg)](https://pypi.python.org/pypi/testcontainers)\n[![codecov](https://codecov.io/gh/testcontainers/testcontainers-python/branch/master/graph/badge.svg)](https://codecov.io/gh/testcontainers/testcontainers-python)\n![Core Tests](https://github.com/testcontainers/testcontainers-python/actions/workflows/ci-core.yml/badge.svg)\n![Community Tests](https://github.com/testcontainers/testcontainers-python/actions/workflows/ci-community.yml/badge.svg)\n[![Docs](https://readthedocs.org/projects/testcontainers-python/badge/?version=latest)](http://testcontainers-python.readthedocs.io/en/latest/?badge=latest)\n\n')

@Lenormju
Copy link

@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.Bind to the docker_client.create (so the file exist before the container starts)
  • You are writing the files using the docker.models.containers.Container.put_archive method (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.

@mgorsk1
Copy link
Contributor Author

mgorsk1 commented Aug 13, 2024

I think copying before can already be solved by with_volume_mapping 😅

Signed-off-by: mgorsk1 <[email protected]>
Signed-off-by: mgorsk1 <[email protected]>
Signed-off-by: mgorsk1 <[email protected]>
Signed-off-by: mgorsk1 <[email protected]>
Signed-off-by: mgorsk1 <[email protected]>
@mgorsk1 mgorsk1 force-pushed the feat/665-with-copy-file-to-container branch from e5a2325 to a2f6094 Compare August 13, 2024 11:08
Signed-off-by: mgorsk1 <[email protected]>
@alexanderankin
Copy link
Member

committed some code here - main...feat/665-with-copy-file-to-container

@alexanderankin
Copy link
Member

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

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