Deprecate ambiguous constructors #2839

rnorth · 2020-06-05T14:44:32Z

Our constructors for GenericContainer and all the classes that inherit from it tend to be a bit inconsistent. In some places, a no-args constructor is favoured, in other places a String parameter is favoured - which can be either a full image name or a tag.

This raises confusion, and in the case of an image tag being the parameter, it restricts which images people can easily use.

Another problem this raises is when the container class specifies a default tag version: this frequently falls out of date. We're stuck for options: some users (and image maintainers) would like to see newer image versions being used, but this could be a breaking change for existing users. This is especially risky when a major version bump is proposed.

For example, the following open PRs suffer varying degrees of this risk: #2825, #2773, #2320, #2436.

This PR aims to:

Deprecate 'ambiguous' constructors for containers - any no-args or String-arg constructor gets this treatment.
Introduce a new constructor which accepts a DockerImageName object. This class is preferred to a String because its meaning is unambiguous.
Enhance DockerImageName with a static factory method and a convenience withTag method so that users can easily create and derive new instances.
Update all modules to follow suit
Update Javadocs throughout to make the change clear
Update our own test methods, leaving a small vanguard of tests for the old way to guard against regression
Update documentation and examples to show the new preferred approach

This PR will not introduce any breaking changes, but is likely to be an 'annoying' change. I'm considering whether a script could be written to quickly migrate code, especially for people with large codebases.

Note that I'm not touching the Future<String> constructor. I would have liked to, but type erasure makes adding an adjacent Future<DockerImageName> constructor impossible. As such, I don't think we can do anything about this particular parameter without a breaking change. I'd be open to suggestions, though!

bsideup · 2020-06-05T15:15:48Z

core/src/main/java/org/testcontainers/containers/FixedHostPortGenericContainer.java

 */
 public class FixedHostPortGenericContainer<SELF extends FixedHostPortGenericContainer<SELF>> extends GenericContainer<SELF> {
+    @Deprecated


Suggested change

*/

public class FixedHostPortGenericContainer<SELF extends FixedHostPortGenericContainer<SELF>> extends GenericContainer<SELF> {

@Deprecated

*/

@Deprecated

public class FixedHostPortGenericContainer<SELF extends FixedHostPortGenericContainer<SELF>> extends GenericContainer<SELF> {

😂 😂 😂

Deprecated without replacement - because it's evil.

bsideup · 2020-06-05T15:17:03Z

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

+        this.image = new RemoteDockerImage(dockerImageName);
+    }
+
+    @Deprecated


what about GenericContainer(Future)? Since it can be replaced with new GenericContainer(new RemoteDockerImage(future))

Ooh, interesting suggestion - I'll explore that.

I've not done this yet.

I've had a quick try, but I'm not sure about this.

This example from existing code:

new GenericContainer(new ImageFromDockerfile() ...

would become

new GenericContainer(new RemoteDockerImage(new ImageFromDockerfile() ...

which I think is quite ugly - it's really not a 'remote docker image', so naming-wise it's a bit of a stretch.

I wonder if, really, the way to make this work would be to rename RemoteDockerImage to FutureDockerImageName, which more accurately reflects what it's used for. So the example would become:

new GenericContainer(new FutureDockerImageName(new ImageFromDockerfile() ...

We'd of course keep RemoteDockerImage around, deprecated, as no more than an alias to FutureDockerImageName.

Two problems I see with this that I don't think we can magic away. I think I could live with these:

the three levels of constructors involved are a bit clunky, but it's not a big deal

it's such as shame that type erasure prevents us from overloading the GenericContainer(Future<String>) constructor, because otherwise FutureDockerImageName could just be Future<DockerImageName> 😂 😭

Yeah, I agree that "Remote" is misleading here.

But the example you provided made me think whether we should even wrap ImageFromDockerfile with RemoteDockerImage, since it is definitely local, does not need an auth resolve, etc etc...
what if we extract a common interface from RemoteDockerImage and make ImageFromDockerfile implement it?

With the interface being DockerImage?

As discussed on Slack, we'll defer this to a separate PR.

It feels like more extensive and complicated changes are needed to refactor the Remote/Future images code, and I feel it would be sensible to not add that complexity on top of this, already massive, PR.

…-constructors # Conflicts: # core/src/test/java/org/testcontainers/images/RemoteDockerImageTest.java # modules/kafka/src/main/java/org/testcontainers/containers/KafkaContainer.java # modules/kafka/src/test/java/org/testcontainers/containers/KafkaContainerTest.java # modules/pulsar/src/main/java/org/testcontainers/containers/PulsarContainer.java # modules/selenium/src/test/java/org/testcontainers/junit/ChromeRecordingWebDriverContainerTest.java

core/src/main/java/org/testcontainers/DockerClientFactory.java

bsideup · 2020-07-03T09:32:57Z

core/src/main/java/org/testcontainers/containers/SocatContainer.java

+    /**
+     * @deprecated use {@link SocatContainer(DockerImageName)} instead
+     */
+    @Deprecated


Just thinking... what if we keep the default constructors in "utility" classes like this one, where the users do not / should not care about the version used? it is a bit contradictory to the idea of this whole change, but I think a few exceptions can be made

Yeah, I was thinking the same. If these had not been essentially in our public API it would have been a no-brainer.

But I'm fine with doing it. I'll move to deprecate these classes away from view (e.g. if we can eventually get them to be package-private then this becomes moot).

well, I would not hide/deprecate them, because they are helpful for building complex 3rd party containers

bsideup · 2020-07-03T09:35:43Z

core/src/main/java/org/testcontainers/images/RemoteDockerImage.java

+        this.imageNameFuture = CompletableFuture.completedFuture(dockerImageName);
+    }
+
+    @Deprecated


Since the contract was always that RemoteDockerImage accepts a full image name, perhaps we can keep this and (String,String) constructors?

Otherwise, it becomes a bit too long:

new GenericContainer(new RemoveDockerImage(new DockerImageName("missed-me:1"))

Will defer as per #2839 (comment)

bsideup · 2020-07-03T09:38:13Z

core/src/main/java/org/testcontainers/utility/DockerImageName.java

@@ -124,8 +144,13 @@ public String getRegistry() {
        return registry;
    }

+    public DockerImageName withTag(final String newTag) {


Love it!

Made me thinking whether we should even have a fully featured builder as well:

DockerImageName.builder() .registry("repo.example.com") .image("services/foo") .tag("123") .build();

I feel like the halfway house of Dockerfilename.of("repo.example.com/services/foo").withTag("123") is probably enough - I'd like to try that first.

FTR the builder idea is just an idea, don't need to be in this PR and can easily be a follow up :)

bsideup · 2020-07-03T09:41:36Z

core/src/test/java/org/testcontainers/containers/ReusabilityUnitTests.java

@@ -61,8 +67,8 @@
        @Parameterized.Parameters(name = "{0}")
        public static Object[][] data() {
            return new Object[][] {
-                { "generic", new GenericContainer(IMAGE_FUTURE), true },
-                { "anonymous generic", new GenericContainer(IMAGE_FUTURE) {}, true },
+                { "generic", new GenericContainer<>(new RemoteDockerImage(IMAGE_FUTURE)), true },


FTR if I remember correctly, I had to use IMAGE_FUTURE here to avoid the side effect we had in the constructor, but now it can be as simple as new GenericContainer<>(TINY_IMAGE)

bsideup · 2020-07-03T09:43:18Z

core/src/test/java/org/testcontainers/images/ImagePullPolicyTest.java

 public class ImagePullPolicyTest {

    @ClassRule
-    public static GenericContainer<?> registry = new GenericContainer<>("registry:2")
+    public static GenericContainer<?> registry = new GenericContainer<>(new DockerImageName("registry:2.7.0"))


I liked the idea of TestImages, btw. Perhaps we can/should move such references to that class as well

Sure, I missed this one!

bsideup · 2020-07-03T09:43:59Z

core/src/test/java/org/testcontainers/images/ImagePullPolicyTest.java

@@ -74,7 +74,7 @@ public void setUp() {
    @Test
    public void pullsByDefault() {
        try (
-            GenericContainer<?> container = new GenericContainer<>(imageName)
+            GenericContainer<?> container = new GenericContainer(new DockerImageName(imageName))


nit: imageName field can be changed to DockerImageName, to make tests' bodies a little bit smaller

bsideup · 2020-07-03T09:50:50Z

examples/mongodb-container/src/test/java/org/testcontainers/containers/MongoDbContainer.java

     */
+    @Deprecated


a bit unrelated, but... don't we have a mongodb module now? :D

Yeah, we should discuss nuking this example...

bsideup · 2020-07-03T12:16:08Z

modules/mongodb/src/test/java/org/testcontainers/containers/MongoDBContainerTest.java

    /**
     * Taken from <a href="https://docs.mongodb.com/manual/core/transactions/">https://docs.mongodb.com</a>
     */
    @Test
    public void shouldExecuteTransactions() {
        try (
            // creatingMongoDBContainer {
-            final MongoDBContainer mongoDBContainer = new MongoDBContainer()
+            final MongoDBContainer mongoDBContainer = new MongoDBContainer(MONGO_IMAGE)


perhaps we should inline the image here, as it will be in the docs

modules/selenium/src/main/java/org/testcontainers/containers/BrowserWebDriverContainer.java

…-constructors

core/src/test/java/org/testcontainers/images/ImagePullPolicyTest.java

…st.java Co-authored-by: Sergei Egorov <bsideup@gmail.com>

aguibert · 2020-07-20T15:31:06Z

@bsideup just saw your tweet for testcontainers 1.15-rc1 and I wanted to -1 the deprecation of new GenericContainer(String), this is a very convenient constructor and isn't at all ambiguous unless a user doesn't bother the read the javadoc and see that the parameter is already named dockerImageName.

An eventual removal of new GenericContainer(String) will break pretty much every single extension I've written (and a lot of others have written). For examle:

public class MyContainer extends GenericContainer<MyContainer> {
  public MyContainer() {
    super("mycontainer:1.0");
  }
  // ...
}

Deprecating this in favor of: new GenericContainer(new DockerImageName(String)) seems like over-typing, especially since DockerImageName doesn't provide any real utility to users. From a user POV this is just a wrapper around String and the methods like assertValid or getRegistry only are only useful to Testcontainers internally.

Instead of deprecating the constructor, can we simply change it to this?

    public GenericContainer(@NonNull final String dockerImageName) {
        this(new DockerImageName(dockerImageName));
    }

rnorth · 2020-07-20T16:12:45Z

@aguibert it's my PR so I'll respond first ;) One of the problems we're trying to undo is the inconsistent meaning of stringly-typed parameters; in some container classes it's a full image name, and in other places it's a tag name (meant to be concatenated against a provided image name).

(String tag) constructors started because some classes have a strong preference for a particular vendor-provided image, so it made sense for us to have a fairly hard default for the image name, overridable via a config file.

We're just as keen to get rid of these constructors as we are to get rid of the default no-args constructors, though:

lack of consistency between what the (String ?) constructor means is confusing for users, despite presence of Javadocs and the name of the parameter
the mechanism we have for overriding the non-tag part of these images is clunky and unwieldy

For these cases we can't just flip the meaning of a single string argument to be a full docker image name - I wish we could, but it would be an immediate breaking change for everyone who has used (String tag) constructors.

So, we don't think it is over-typing, as one of the problems we're trying to tackle is the tech debt of having an ambiguous type 😀

As for breakage, we're only deprecating the constructors, not removing them. We intend to give a significant period of time for people to migrate. You can see from our previous deprecation-removal cycles that we're pretty glacial about this, so I wouldn't expect an actual breaking change here in 2020.

bsideup · 2020-07-20T16:14:37Z

@rnorth btw, I was thinking about this one:

For these cases we can't just flip the meaning of a single string argument to be a full docker image name - I wish we could, but it would be an immediate breaking change for everyone who has used (String tag) constructors.

since we have the parsing logic, theoretically, we could detect tag vs image and handle both :)

rnorth · 2020-07-20T16:16:31Z

since we have the parsing logic, theoretically, we could detect tag vs image and handle both

Yeah, I thought about it but it feels like inviting spectacular unforeseen consequences into our lives 😆

bsideup · 2020-07-20T16:18:15Z

:D true. Although I think in this case the rules are "strict enough":
https://docs.docker.com/engine/reference/commandline/tag/

A tag name must be valid ASCII and may contain lowercase and uppercase letters, digits, underscores, periods and dashes. A tag name may not start with a period or a dash and may contain a maximum of 128 characters.

sounds like we can simply check for : presence... wdyt?

rnorth · 2020-07-21T09:34:40Z

We’ve come up with what we think is a better compromise:

We’ll undo deprecation of (String imageName) constructors
We’ll keep deprecation of (String tag) constructors, and continue to push people to use (DockerImageName) constructors in these cases
At a later date we will change all (String tag) constructors to (String imageName) and un-deprecate. This will be a breaking change, but we’ll have given people plenty of warning in advance.

Thanks for raising your concerns @aguibert - it’s made us rethink this and find a (hopefully) better solution.

aguibert · 2020-07-21T13:31:49Z

That sounds like a good compromise, thanks as always for your consideration and all you do for this project @rnorth and @bsideup !

bsideup reviewed Jun 5, 2020

View reviewed changes

WIP

68ee3bc

rnorth force-pushed the deprecate-ambiguous-constructors branch from b60d19c to 68ee3bc Compare June 25, 2020 13:04

rnorth added 7 commits June 25, 2020 14:18

WIP

46c7e1b

WIP

054b637

WIP

196f1a9

Tidy up test image references

f3fe807

Update docs re contrainer creation and specifying versions

296f9f7

Update Javadocs with deprecation warnings.

aa94289

rnorth marked this pull request as ready for review July 2, 2020 14:50

rnorth requested a review from kiview as a code owner July 2, 2020 14:50

kiview reviewed Jul 2, 2020

View reviewed changes

core/src/main/java/org/testcontainers/DockerClientFactory.java Outdated Show resolved Hide resolved

Add asCanonicalNameString() method on DockerImageName

77aa418

bsideup reviewed Jul 3, 2020

View reviewed changes

modules/selenium/src/main/java/org/testcontainers/containers/BrowserWebDriverContainer.java Show resolved Hide resolved

rnorth added 6 commits July 4, 2020 09:08

Use DockerImageName instead of String for image name

fc7f625

Inline Mongo image name parsing

cd7e13b

Fix constructor delegation

a35b3c4

Undo deprecation of SocatContainer no-args constructor

868f962

Update references to DockerImageName.parse in example code

faf1217

Merge remote-tracking branch 'origin/master' into deprecate-ambiguous…

ee45003

…-constructors

rnorth added the type/deprecation Public APIs are being deprecated but not changed yet label Jul 5, 2020

Merge remote-tracking branch 'origin/master' into deprecate-ambiguous…

8960ceb

…-constructors

rnorth mentioned this pull request Jul 9, 2020

Update to latest LocalStack container and support unified port mode #2825

Merged

rnorth requested review from bsideup and kiview July 15, 2020 11:52

Merge remote-tracking branch 'origin/master' into deprecate-ambiguous…

f4e314d

…-constructors

bsideup reviewed Jul 18, 2020

View reviewed changes

core/src/test/java/org/testcontainers/images/ImagePullPolicyTest.java Outdated Show resolved Hide resolved

Update core/src/test/java/org/testcontainers/images/ImagePullPolicyTe…

cc4190d

…st.java Co-authored-by: Sergei Egorov <bsideup@gmail.com>

rnorth added this to the next milestone Jul 19, 2020

bsideup approved these changes Jul 19, 2020

View reviewed changes

bsideup added the type/enhancement label Jul 19, 2020

Merge branch 'master' into deprecate-ambiguous-constructors

0eee5d8

bsideup merged commit c3f53b3 into master Jul 19, 2020

delete-merged-branch bot deleted the deprecate-ambiguous-constructors branch July 19, 2020 12:32

silaev added a commit to silaev/testcontainers-java that referenced this pull request Jul 28, 2020

Use new constructor as per testcontainers#2839

d02daf7

rnorth mentioned this pull request Oct 24, 2020

Developer experience: recommending a default container image #3381

Open

bsideup mentioned this pull request Nov 10, 2020

gcloud module default to a container image #3456

Closed

Deprecate ambiguous constructors #2839

Deprecate ambiguous constructors #2839

Conversation

rnorth commented Jun 5, 2020 • edited

Choose a reason for hiding this comment

rnorth Jun 5, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aguibert commented Jul 20, 2020

rnorth commented Jul 20, 2020

bsideup commented Jul 20, 2020

rnorth commented Jul 20, 2020

bsideup commented Jul 20, 2020

rnorth commented Jul 21, 2020

aguibert commented Jul 21, 2020

rnorth commented Jun 5, 2020 •

edited

rnorth Jun 5, 2020 •

edited