Allow customization of test containers image name #266

andythsu · 2024-02-27T21:53:37Z

Problem: Some tests download trino docker images from public dockerhub. For example:

trino-gateway/gateway-ha/src/test/java/io/trino/gateway/ha/TestGatewayHaMultipleBackend.java

Line 55 in e2aaa0e

adhocTrino = new TrinoContainer("trinodb/trino");

However, this will fail when running behind a proxy. Our in-house images have different names from the names on dockerhub. Therefore, we need a way to override trinodb/trino to a completely different name when necessary. This will eventually expand to other public images as they are added to the codebase.

cla-bot · 2024-02-27T21:53:40Z

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Andy Su (Apps).
This is most likely caused by a git client misconfiguration; please make sure to:

check if your git client is configured with an email to sign commits git config --list | grep email
If not, set it up using git config --global user.email [email protected]
Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

cla-bot · 2024-02-27T21:54:52Z

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Andy Su (Apps).
This is most likely caused by a git client misconfiguration; please make sure to:

check if your git client is configured with an email to sign commits git config --list | grep email
If not, set it up using git config --global user.email [email protected]
Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

gateway-ha/src/test/java/io/trino/gateway/CustomImageNameSubstitutor.java

ebyhr · 2024-02-27T22:11:35Z

gateway-ha/src/test/java/io/trino/gateway/CustomImageNameSubstitutor.java

+    @Override
+    public DockerImageName apply(DockerImageName dockerImageName)
+    {
+        if (dockerImageName.asCanonicalNameString().equals("trinodb/trino:latest") && System.getenv(TESTCONTAINERS_TRINO_IMAGE_SUBSTITUTE) != null) {


Remove the 1st condition.

first condition makes sure we are overriding a specific image. If we don't specify it will just substitute all images

See #266 (comment)

I understand you want to change only Trino image names. I suppose we should tweak the condition to exclude the version.

we can probably use this getUnversionedPart() from https://javadoc.io/doc/org.testcontainers/testcontainers/1.15.1/org/testcontainers/utility/DockerImageName.html#getUnversionedPart--

gateway-ha/src/test/java/io/trino/gateway/CustomImageNameSubstitutor.java

ebyhr · 2024-02-27T22:13:12Z

gateway-ha/src/test/java/io/trino/gateway/CustomImageNameSubstitutor.java

+    {
+        return "custom image name substitutor";
+    }
+}


testcontainers.properties isn't required?

i was going to use env variables to configure this. https://java.testcontainers.org/features/configuration/
It looks like we can just choose one

If we use env variables, why this class is required? I'm looking the following sentence:

Setting environment variables TESTCONTAINERS_HUB_IMAGE_NAME_PREFIX=registry.mycompany.com/mirror/
https://java.testcontainers.org/features/image_name_substitution/#automatically-modifying-docker-hub-image-names

this may not work because it needs image name to be predictable

I was looking at this https://java.testcontainers.org/features/image_name_substitution/#developing-a-custom-function-for-transforming-image-names-on-the-fly

it needs image name to be predictable

I don't understand this. Can you elaborate on that?

If we use env variables, why this class is required? I'm looking the following sentence:

Setting environment variables TESTCONTAINERS_HUB_IMAGE_NAME_PREFIX=registry.mycompany.com/mirror/
https://java.testcontainers.org/features/image_name_substitution/#automatically-modifying-docker-hub-image-names

The link you are looking at is specifically adding prefix to image name. It has an assumption:

Consider this if:

Your private registry has copies of images from Docker Hub where the names are predictable, and just adding a prefix is enough. For example, registry.mycompany.com/mirror/mysql:8.0.36 can be derived from the original Docker Hub image name (mysql:8.0.36) with a consistent prefix string: registry.mycompany.com/mirror/

Also, it looks like even though we can change the registry, the names of the image have to match. In our case, we have different image names from the dockerhub.

Thanks for your explanation.

we have different image names from the dockerhub.

This is the important fact I didn't know. Please leave the code comment.

added.

I can't find it.

ebyhr · 2024-02-27T23:17:03Z

gateway-ha/src/test/java/io/trino/gateway/CustomImageNameSubstitutor.java

+import org.testcontainers.utility.DockerImageName;
+import org.testcontainers.utility.ImageNameSubstitutor;
+
+public class CustomImageNameSubstitutor


The class name is misleading. Please rename to CustomTrinoImageNameSubstitutor.

From the doc I suppose we can only have one ImageNameSubstitutor, then we use TESTCONTAINERS_IMAGE_SUBSTITUTOR=com.mycompany.testcontainers.ExampleImageNameSubstitutor to tell testcontainer to use this class. Therefore, I think this class should have a generic name as this is the only substitutor that testcontainer will look at.

I still disagree with the current name. Even CustomImageNameSubstitutor doesn't explain whether TESTCONTAINERS_IMAGE_SUBSTITUTOR supports single substitutor or several substitutors either.

I don't understand your concern to be honest. There's only one class extending ImageNameSubstitutor in this repository. Who will try setting several substitutors?

If the class CustomImageNameSubstitutor is used for other images, CustomImageNameSubstitutor seems right but since this is only applied to trino image, I think it's better off with what ebyhr saids. We can change the name to a more general one if there are other images to substitute in future

I still disagree with the current name. Even CustomImageNameSubstitutor doesn't explain whether TESTCONTAINERS_IMAGE_SUBSTITUTOR supports single substitutor or several substitutors either.

I don't understand your concern to be honest. There's only one class extending ImageNameSubstitutor in this repository. Who will try setting several substitutors?

The concern is we can only have one ImageSubstitutor. Naming it to CustomTrinoImageNameSubstitutor is misleading because it might lead to thinking it's only for substituting Trino image. However, this class should be flexible to substitute other images as well. I agree with @Chaho12's suggestion. We can change the name to be generic later as we need.

andythsu · 2024-02-28T14:27:30Z

We probably want to add how to use this class to the documentation. @mosabua could you suggest a good place for adding this doc?

ebyhr · 2024-02-28T14:57:22Z

We probably want to add how to use this class to the documentation

Most people won't use this feature. Please leave javadoc in the class.

ebyhr · 2024-02-28T14:59:00Z

gateway-ha/src/test/java/io/trino/gateway/CustomTrinoImageNameSubstitutor.java

+    @Override
+    public DockerImageName apply(DockerImageName dockerImageName)
+    {
+        String trinoImage = "trinodb/trino";


Move to a constant.

ebyhr · 2024-02-28T14:59:07Z

gateway-ha/src/test/java/io/trino/gateway/CustomTrinoImageNameSubstitutor.java

+    @Override
+    protected String getDescription()
+    {
+        return "custom image name substitutor";


Suggested change

return "custom image name substitutor";

return "custom Trino image name substitutor";

gateway-ha/src/test/java/io/trino/gateway/CustomTrinoImageNameSubstitutor.java

wendigo · 2024-02-28T20:49:05Z

0:16:09.097 [main] INFO org.testcontainers.utility.ImageNameSubstitutor -- Found configured ImageNameSubstitutor: custom Trino image name substitutor
20:16:09.098 [main] INFO org.testcontainers.utility.ImageNameSubstitutor -- Image name substitution will be performed by: Chained substitutor of 'DefaultImageNameSubstitutor (composite of 'ConfigurationFileImageNameSubstitutor' and 'PrefixingImageNameSubstitutor')' and then 'custom Trino image name substitutor'

wendigo · 2024-02-28T20:49:35Z

gateway-ha/src/test/java/io/trino/gateway/CustomTrinoImageNameSubstitutor.java

+    {
+        String trinoImageSubstitute = System.getenv(TESTCONTAINERS_TRINO_IMAGE_SUBSTITUTE);
+        if (dockerImageName.getUnversionedPart().equals(TRINO_IMAGE) && trinoImageSubstitute != null) {
+            String imageSubstitute = System.getenv(TESTCONTAINERS_TRINO_IMAGE_SUBSTITUTE);


redundant variable. You have it above

wendigo

Small change requested

andythsu force-pushed the image_name_substitutor branch from 3cd79ca to aa99a08 Compare February 27, 2024 21:54

andythsu force-pushed the image_name_substitutor branch from aa99a08 to b4a5de7 Compare February 27, 2024 21:56

cla-bot bot added the cla-signed label Feb 27, 2024

andythsu force-pushed the image_name_substitutor branch 2 times, most recently from ba9e9cd to 3fb6096 Compare February 27, 2024 22:10

ebyhr reviewed Feb 27, 2024

View reviewed changes

andythsu force-pushed the image_name_substitutor branch from 3fb6096 to 46a14fe Compare February 27, 2024 22:20

ebyhr added the no-release-notes This pull request does not require release notes entry label Feb 27, 2024

ebyhr reviewed Feb 27, 2024

View reviewed changes

andythsu force-pushed the image_name_substitutor branch from 46a14fe to 4f404b6 Compare February 28, 2024 14:26

ebyhr reviewed Feb 28, 2024

View reviewed changes

wendigo reviewed Feb 28, 2024

View reviewed changes

gateway-ha/src/test/java/io/trino/gateway/CustomTrinoImageNameSubstitutor.java Show resolved Hide resolved

wendigo reviewed Feb 28, 2024

View reviewed changes

gateway-ha/src/test/java/io/trino/gateway/CustomTrinoImageNameSubstitutor.java Outdated Show resolved Hide resolved

andythsu force-pushed the image_name_substitutor branch 2 times, most recently from 249f71d to 28fb73f Compare February 28, 2024 20:14

wendigo reviewed Feb 28, 2024

View reviewed changes

wendigo requested changes Feb 28, 2024

View reviewed changes

Allow customization of test containers image name

8455dd6

andythsu force-pushed the image_name_substitutor branch from 28fb73f to 8455dd6 Compare February 28, 2024 20:56

wendigo approved these changes Feb 28, 2024

View reviewed changes

wendigo merged commit 5fff194 into trinodb:main Feb 28, 2024
2 checks passed

github-actions bot added this to the 7 milestone Feb 28, 2024

andythsu changed the title ~~allow customization of test containers image name~~ Allow customization of test containers image name Feb 29, 2024

mosabua mentioned this pull request Mar 18, 2024

Add release notes for Trino Gateway 7 #286

Merged

andythsu deleted the image_name_substitutor branch September 18, 2024 20:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow customization of test containers image name #266

Allow customization of test containers image name #266

andythsu commented Feb 27, 2024 •

edited

Loading

cla-bot bot commented Feb 27, 2024

cla-bot bot commented Feb 27, 2024

ebyhr Feb 27, 2024

andythsu Feb 27, 2024

ebyhr Feb 27, 2024

ebyhr Feb 27, 2024

andythsu Feb 28, 2024

ebyhr Feb 27, 2024

andythsu Feb 27, 2024

ebyhr Feb 27, 2024

andythsu Feb 27, 2024

andythsu Feb 27, 2024

ebyhr Feb 27, 2024

andythsu Feb 27, 2024 •

edited

Loading

ebyhr Feb 27, 2024

andythsu Feb 28, 2024

ebyhr Feb 28, 2024

ebyhr Feb 27, 2024

andythsu Feb 28, 2024

ebyhr Feb 28, 2024 •

edited

Loading

Chaho12 Feb 28, 2024

andythsu Feb 28, 2024

andythsu commented Feb 28, 2024 •

edited

Loading

ebyhr commented Feb 28, 2024

ebyhr Feb 28, 2024

ebyhr Feb 28, 2024

wendigo commented Feb 28, 2024

wendigo Feb 28, 2024

wendigo left a comment •

edited

Loading

	return "custom image name substitutor";
	return "custom Trino image name substitutor";

Allow customization of test containers image name #266

Allow customization of test containers image name #266

Conversation

andythsu commented Feb 27, 2024 • edited Loading

cla-bot bot commented Feb 27, 2024

cla-bot bot commented Feb 27, 2024

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

andythsu Feb 27, 2024 • edited Loading

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

ebyhr Feb 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andythsu commented Feb 28, 2024 • edited Loading

ebyhr commented Feb 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wendigo commented Feb 28, 2024

Choose a reason for hiding this comment

wendigo left a comment • edited Loading

Choose a reason for hiding this comment

andythsu commented Feb 27, 2024 •

edited

Loading

andythsu Feb 27, 2024 •

edited

Loading

ebyhr Feb 28, 2024 •

edited

Loading

andythsu commented Feb 28, 2024 •

edited

Loading

wendigo left a comment •

edited

Loading