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

Deprecate file/volume mapping APIs #7652

Merged
merged 1 commit into from
Oct 23, 2023
Merged

Conversation

eddumelendez
Copy link
Member

Using Copy Files API will make tests portable cross docker environments.
Docs are removed too.

Using `Copy Files API` will make tests portable cross docker environments.
Docs are removed too.
@eddumelendez eddumelendez requested a review from a team as a code owner October 13, 2023 17:53
@eddumelendez eddumelendez added this to the next milestone Oct 14, 2023
Copy link
Member

@kiview kiview left a comment

Choose a reason for hiding this comment

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

While withCopyToContainer is not a 100% replacement of all variants of using withFileSystemBind (e.g. READ_WRITE, or SelinuxContext), for the intents of the general Testcontainers use cases, the deprecation makes IMO sense.

So I'd say, let's go ahead with it and see if there are any users missing these features (and don't remove the deprecated methods for some time).

@eddumelendez eddumelendez merged commit 34853fe into main Oct 23, 2023
87 checks passed
@eddumelendez eddumelendez deleted the deprecate-file-volume-mapping branch October 23, 2023 14:16
@eddumelendez eddumelendez added the type/deprecation Public APIs are being deprecated but not changed yet label Oct 23, 2023
@idontusenumbers
Copy link

Can the documentation be updated with a comparison of MountableFile and Transferable.of? I see both examples but it's not clear why one would use each over the other. Looking at the code, it seems a Transferable.of(String) creates a file with the contents set to the string passed in, though the example given is ambiguous of that might be a local file path.

@slovdahl
Copy link
Contributor

We run quite a lot of MariaDB-based tested, and some of them create and drop databases over and over again. Doing this on a partition backed by a real disk is not that fast, so we have optimized it by mounting a RAM disk as /var/lib/mysql inside the container. Using the RAM disk speeds up test runs with ~50% locally, and in contended CI even more.

dbContainer.withFileSystemBind( RAMDISK_LOCATION + '/' + dbTypeAndVersion, "/var/lib/mysql" );

Correct me if I'm wrong, but I don't think this is doable with withCopyToContainer?

@slovdahl
Copy link
Contributor

Ah, after reading testcontainers/testcontainers-go#1907 I found out that this might work:

dbContainer.withCreateContainerCmdModifier(
        cmd -> cmd.getHostConfig()
                .withBinds( Bind.parse( RAMDISK_LOCATION + '/' + dbTypeAndVersion + ":/var/lib/mysql:rw" ) )
);

Is this something that will continue working going forward? Maybe it could be mentioned in the docs somewhere in that case.

@idontusenumbers
Copy link

As for cases where this might end up being worse, I was using a bind to get a handful of files in and handful of files out. The 1 line bind is now ~8 or so copy-in/copy-out.

@perlun
Copy link
Contributor

perlun commented Nov 27, 2023

Is this something that will continue working going forward? Maybe it could be mentioned in the docs somewhere in that case.

Yes, very much agree on this one. 👍 Even though the Copy files API is "recommended [...] for portability cross-docker environments", there are still cases like the one described above where bind-mounting is preferable. Would be great to see this mentioned in the docs, and being explicitly supported by Testcontainers. 🙇

perlun added a commit to perlun/testcontainers-java that referenced this pull request Nov 29, 2023
This should have been included in testcontainers#7652,
but was likely forgotten.
@perlun
Copy link
Contributor

perlun commented Nov 29, 2023

Also @eddumelendez, I think you missed at least two deprecations. This, and another one in the same class. 👇

@Override
public SELF withFileSystemBind(String hostPath, String containerPath, BindMode mode) {
addFileSystemBind(hostPath, containerPath, mode);
return self();
}

I submitted this PR to fix it: #7895

(It would also be nice to get the other questions answered, but until then let's at least be consistent. 😄)

@perlun
Copy link
Contributor

perlun commented Nov 30, 2023

The above was apparently deliberate. Let's continue the discussion in #7895.

@bsideup
Copy link
Member

bsideup commented Nov 30, 2023

@slovdahl you can just use withTmpFs instead 👍

(FTR this is a good example of when "there are still cases like the one described above where bind-mounting is preferable" isn't so true and there is a better API for the job)

@slovdahl
Copy link
Contributor

slovdahl commented Dec 1, 2023

@slovdahl you can just use withTmpFs instead 👍

Cool, TIL, thanks for sharing! Definitely need to investigate that approach.

@big-andy-coates
Copy link
Contributor

big-andy-coates commented Dec 24, 2023

I'm currently using Container.withFileSystemBind with READ_WRITE. The suggested alternative in the deprecated notes of the Container class is GenericContainer.withCopyToContainer.

  1. (minor) Why is this not Container.withCopyToContainer, given the Container interface has such a method. Seems strange for an interface to be directing people to a single implementation.
  2. withCopyToContainer is only an alternative for READ_ONLY usage. It would be nice if an alternative is also given for READ_WRITE usage.
  3. What is the alternative for READ_WRITE usage? If I can't mount a writable mount, then I'll have to write a file within the image and then somehow copy it out of the container once the main process has terminated... is this possible?

(Let me know if this is not the right place for this discussion...)

@slovdahl
Copy link
Contributor

@big-andy-coates did you try #7652 (comment)?

@big-andy-coates
Copy link
Contributor

I'll take a look...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/deprecation Public APIs are being deprecated but not changed yet type/docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants