Use "copy" strategy in withClasspathResourceMapping where appropriate

[bsideup]

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 than NONE) 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 of READ_ONLY) and the minority of such usages, I would say that's okay.

[bsideup]

/azp run "Windows 10 - Docker for Windows"

[azure-pipelines]

No pipelines are associated with this pull request.

[bsideup]

/AzurePipelines run Windows 10 - Docker for Windows

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[vpavic]

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 Container API less transparent IMO.

To me, Container#withClasspathResourceMapping suggests this will result in a mounting operation. With this change, the operation depends on an input and is thus less obvious. I'd prefer if users were guided to use Container#withCopyFileToContainer themselves instead.

Would you accept a PR to provide something like Container#withCopyClasspathResourceToContainer, that would free users from having to deal with MountableFile as in Container#withCopyFileToContainer?

[bsideup]

To me, Container#withClasspathResourceMapping suggests this will result in a mounting operation. With this change, the operation depends on an input and is thus less obvious

the operation depends on an input

Sorry, not sure I follow the "depends" part.

To me, Container#withClasspathResourceMapping suggests this will result in a mounting operation.

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. Container methods provide an abstraction for common operations, and it is up to the implementation how to perform such operations, but we try to make them work in 100% of cases.

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 👍

[bsideup]

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 👍

[vpavic]

See here:
https://github.com/spring-projects/spring-session/blob/2f79da00dc4ae66facbd0ff669681f4115199106/spring-session-hazelcast/src/integration-test/java/org/springframework/session/hazelcast/ClientServerHazelcastIndexedSessionRepositoryITests.java#L49-L52

unlike the file system mounting, the copy API requires the folders structure to be presented too.

Right, that was the problem in case of this test.

Sorry, not sure I follow the "depends" part.

The resulting operation (copy vs mount) directly depends on the input parameters of Container#withClasspathResourceMapping:

testcontainers-java/core/src/main/java/org/testcontainers/containers/GenericContainer.java

Lines 966 to 977 in 94c903d

	@Override
	public SELF withClasspathResourceMapping(final String resourcePath, final String containerPath, final BindMode mode, final SelinuxContext selinuxContext) {
	final MountableFile mountableFile = MountableFile.forClasspathResource(resourcePath);
	if (mode == BindMode.READ_ONLY && selinuxContext == SelinuxContext.NONE) {
	withCopyFileToContainer(mountableFile, containerPath);
	} else {
	addFileSystemBind(mountableFile.getResolvedPath(), containerPath, mode, selinuxContext);
	}
	return self();
	}

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 Container#withCopyClasspathResourceToContainer and have Container#withClasspathResourceMapping with BindMode of READ_ONLY behave as it did before.

[bsideup]

I see now. I just fixed the regression:
#1956

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 if {} else {}... bad? I mean... if we know for sure that we can do the copy (after the fix :D), why would you care?

[vpavic]

Thanks for the prompt fix. 👍

Regarding my comments on the resulting operation, I've always liked the Container API because it was simple and transparent, so that you would always know which operation would a given method result in. This conditional logic breaks that, and also introduces complexity (as you can see from the fact it caused a regression 😉).

[fedinskiy]

@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: .withClasspathResourceMapping("from", "to", BindMode.READ_ONLY), right?

[1]

testcontainers-java/core/src/main/java/org/testcontainers/containers/GenericContainer.java

Line 417 in eea61bd

copyToFileContainerPathMap.forEach(this::copyFileToContainer);

[bsideup]

@fedinskiy when container is created, not started. The files will be there by the time container's command is executed.