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: support copy files to container #730

Merged
merged 2 commits into from
Sep 13, 2024

Conversation

guenhter
Copy link
Contributor

@guenhter guenhter commented Sep 10, 2024

This PR supports copying files into the container like this:

let container = GenericImage::new("alpine", "latest")
        .with_wait_for(WaitFor::seconds(2))
        .with_copy_to("/tmp/somefile", "foobar".to_string().into_bytes())
        .start()
        .await?;

Copy link

netlify bot commented Sep 10, 2024

Deploy Preview for testcontainers-rust ready!

Name Link
🔨 Latest commit 4249029
🔍 Latest deploy log https://app.netlify.com/sites/testcontainers-rust/deploys/66e2922e737ec80008eade49
😎 Deploy Preview https://deploy-preview-730--testcontainers-rust.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Collaborator

@DDtKey DDtKey left a comment

Choose a reason for hiding this comment

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

Hi there 👋

Thank you a lot for the contribution 👍

However it would be nice to rationalize the need and discuss the proposal before putting an effort 🙏

So, I guess mountfunctionality doesn't cover your needs? You want a full independent copy of files, right? So we will have both mounting and copy features (like in Java version)

testcontainers/src/core/client.rs Outdated Show resolved Hide resolved
@guenhter
Copy link
Contributor Author

Hi,

the rational behind is specially when using Docker In Docker environments with the docker-socket mounted. Here, copying files to the container instead of mounting them is way easier and less error prone. This is one of the main reasons why this is needed.

@DDtKey
Copy link
Collaborator

DDtKey commented Sep 10, 2024

the rational behind is specially when using Docker In Docker environments with the docker-socket mounted. Here, copying files to the container instead of mounting them is way easier and less error prone. This is one of the main reasons why this is needed.

I actually think it's a useful feature, so I don't mind for sure.
It's just always a shame and sad that a person has made a lot of effort, but the PR is not accepted for some other reason

That's why I prefer to have at least a short issue to have visibility

@DDtKey DDtKey changed the title Support copy files to container feat: support copy files to container Sep 10, 2024
@guenhter
Copy link
Contributor Author

Completely understandable. Next time, I'll raise an issue first for discussion.

testcontainers/Cargo.toml Outdated Show resolved Hide resolved
testcontainers/src/core/copy.rs Outdated Show resolved Hide resolved
testcontainers/src/core/client.rs Outdated Show resolved Hide resolved
testcontainers/src/core/client.rs Outdated Show resolved Hide resolved
@kiview
Copy link
Member

kiview commented Sep 12, 2024

Just to chime in quickly from a meta Testcontainers perspective:
Copy file is indeed preferred over mounting (and available as an API in other languages as well, in Java we even deprecated the mounting API, in favor of the copy API), exactly because of the reasons outlined by @guenhter. The copy approach is extremely portable across all kind of different Docker setup (e.g. socket mounting, or remote Docker daemon).

Copy link
Collaborator

@DDtKey DDtKey left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution and patience during the review process ❤️

@DDtKey DDtKey merged commit cf6f593 into testcontainers:main Sep 13, 2024
12 checks passed
@guenhter
Copy link
Contributor Author

Thanks for all the help. I realy appreciate your effort.

@guenhter guenhter deleted the copy-to-container branch September 14, 2024 07:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants