-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Use "copy" strategy in withClasspathResourceMapping
where appropriate
#1814
Conversation
/azp run "Windows 10 - Docker for Windows" |
No pipelines are associated with this pull request. |
core/src/test/java/org/testcontainers/junit/CopyFileToContainerTest.java
Show resolved
Hide resolved
/AzurePipelines run Windows 10 - Docker for Windows |
Azure Pipelines successfully started running 1 pipeline(s). |
This change broke one of our integration tests in Spring Session. Although it wasn't a problem to adopt to the change, I'm not a fan of this change as it makes To me, Would you accept a PR to provide something like |
Sorry, not sure I follow the "depends" part.
We're trying to not use the mounting where possible, because it does not work with remote Daemons and/or in-container scenarios. This is why we made this change. I will try to debug why it broke your test tho. We would appreciate in case you have any info to share on what it broke 👍 |
I did some debugging and discovered that, unlike the file system mounting, the copy API requires the folders structure to be presented too. This is clearly a regression, I will fix it 👍 |
Right, that was the problem in case of this test.
The resulting operation (copy vs mount) directly depends on the input parameters of testcontainers-java/core/src/main/java/org/testcontainers/containers/GenericContainer.java Lines 966 to 977 in 94c903d
With the current state, you're making it harder to do a simple read only mount. That's why as a user I'd prefer to have something like |
I see now. I just fixed the regression: once merged (and released of course), there should not be any difference to the user which method is used, and the end result will be exactly the same. I believe the problem was that we missed the "auto create folders" part. If not that regression, you would update the version without any problem and not notice the change, but your tests would become more flexible (in regards of the target Docker environment). That was the goal :) Or do you still find this |
Thanks for the prompt fix. 👍 Regarding my comments on the resulting operation, I've always liked the |
@bsideup if I understand this correctly, "copy strategy" copies files only after the container has started[1]. And this means, that if some file is required for start-up, then it cannot be copied like this: [1] testcontainers-java/core/src/main/java/org/testcontainers/containers/GenericContainer.java Line 417 in eea61bd
|
@fedinskiy when container is created, not started. The files will be there by the time container's command is executed. |
Context
The current implementation of
withClasspathResourceMapping
uses file mounting.While it works fine most of the time, there are some environments where the file mounting does not work (simple example: remote Docker daemon).
The change
For read-only classpath mappings without Selinux we can use the copy strategy.
Testing
I added tests for different combinations (read-only, read-only with
Selinux.NONE
, read-write, read-only with Selinux other thanNONE
) to make sure that we only use the strategy where we know for sure.What can go wrong
Some users may use
withClasspathResourceMapping
for large files. This won't break anything, but may make it slightly slower for them.Given that there is a workaround (e.g. use
READ_WRITE
instead ofREAD_ONLY
) and the minority of such usages, I would say that's okay.