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

Normalize image names in prefix substitutor #8509

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

findepi
Copy link
Contributor

@findepi findepi commented Apr 4, 2024

Optionally normalize docker image names to use library/ prefix when they don't have any namespace at all. This allows for the PrefixingImageNameSubstitutor to be used to redirect Docker Hub requests to a private registry without having to apply otherwise redundant changes to the test code.

@findepi findepi requested a review from a team as a code owner April 4, 2024 12:56
@eddumelendez
Copy link
Member

@findepi thanks for your contribution, can you please add some docs?

@eddumelendez eddumelendez added this to the next milestone Apr 25, 2024
@findepi
Copy link
Contributor Author

findepi commented Apr 26, 2024

@eddumelendez thanks for review!
sure, where this should be documented?

@eddumelendez
Copy link
Member

@findepi findepi force-pushed the findepi/normalize-image-names-in-prefix-substitutor-af94c9 branch from c002c29 to 78ce06e Compare April 29, 2024 11:20
@findepi findepi force-pushed the findepi/normalize-image-names-in-prefix-substitutor-af94c9 branch from 78ce06e to 64e9271 Compare April 29, 2024 11:21
Comment on lines +80 to +86
If you want your registry to handle both official Docker Hub images (e.g `postgres`)
as well as images from other registries (e.g `mycompany/postgres`), you can use the
`TESTCONTAINERS_HUB_IMAGE_NAME_NORMALIZE` environment variable or the `hub.image.name.normalize`
configuration option. When set to `true`, Testcontainers will normalize the official Docker Hub
image names to start with `library/`. When this option is used, Testcontainers will additionally
disable image compatibility checks done by some containers, so it the compatibility responsibility
is on the user.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added this. cc @eddumelendez.

Did i get the env variable name right?
There is automatic conversion from TESTCONTAINERS_HUB_IMAGE_NAME_NORMALIZE to hub.image.name.normalize, right?

Copy link
Member

Choose a reason for hiding this comment

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

yes, the name is correct.

When this option is used, Testcontainers will additionally disable image compatibility checks done by some containers, so it the compatibility responsibility is on the user.

I don't think this is the case because the check compatibility is executed first. Can you please add a test similar to

@Test
public void testWorksWithoutConfiguredImplementation() {
Mockito.doReturn(null).when(TestcontainersConfiguration.getInstance()).getImageSubstitutorClassName();
final ImageNameSubstitutor imageNameSubstitutor = ImageNameSubstitutor.instance();
DockerImageName result = imageNameSubstitutor.apply(DockerImageName.parse("original"));
assertThat(result.asCanonicalNameString())
.as("the image has been substituted by default then configured implementations")
.isEqualTo("substituted-image:latest");
}
in order to cover it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think this is the case because the check compatibility is executed first.

that makes sense!

however, when testing this PR internally, i run into some issues until i added "as compatible substitute" call.
is it possible that there are some additional checks somewhere else? sadly, don't have stacktrace now

@findepi findepi force-pushed the findepi/normalize-image-names-in-prefix-substitutor-af94c9 branch from 64e9271 to 4539b80 Compare April 29, 2024 15:09
@findepi
Copy link
Contributor Author

findepi commented Apr 29, 2024

The build failed (core)

Gradle Test Executor 10 > org.testcontainers.junit.DockerComposeContainerScalingTest > simpleTest FAILED
    org.junit.ComparisonFailure: [Each redis instance is separate] expected:<"[1]"> but was:<"[2]">
        at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
        at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
        at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
        at org.testcontainers.junit.DockerComposeContainerScalingTest.simpleTest(DockerComposeContainerScalingTest.java:54)

I will need some guidance, I don't know how to fix it.

@eddumelendez
Copy link
Member

@findepi can you run ./gradlew spotlessApply --no-daemon, please?

@findepi findepi force-pushed the findepi/normalize-image-names-in-prefix-substitutor-af94c9 branch from 4539b80 to caee590 Compare May 7, 2024 08:26
@findepi
Copy link
Contributor Author

findepi commented May 7, 2024

@eddumelendez done!

@findepi
Copy link
Contributor Author

findepi commented May 7, 2024

CI green this time. Sorry for missing spotless on the previous push.

@eddumelendez eddumelendez removed this from the next milestone May 8, 2024
@findepi findepi force-pushed the findepi/normalize-image-names-in-prefix-substitutor-af94c9 branch from caee590 to e69a9ce Compare May 9, 2024 18:51
@findepi
Copy link
Contributor Author

findepi commented May 10, 2024

I see test failure https://app.circleci.com/pipelines/github/testcontainers/testcontainers-java/11624/workflows/67da8da7-fbdc-42d2-94ab-c6e59c101324/jobs/43677

     Caused by:
            com.github.dockerjava.api.exception.DockerClientException: Could not pull image: [DEPRECATION NOTICE] Docker Image Format v1 and Docker Image manifest version 2, schema 1 support is disabled by default and will be removed in an upcoming release. Suggest the author of docker.io/library/redis:3.0.2 to upgrade the image to the OCI Format or Docker Image manifest v2, schema 2. More information at https://docs.docker.com/go/deprecated-image-specs/

in https://github.com/trinodb/trino we solved similar failures by bumping the image version, but I am not sure this is the way to go here.

@eddumelendez please let me know what do you think about this

@findepi findepi closed this May 17, 2024
@findepi findepi reopened this May 17, 2024
Optionally normalize docker image names to use `library/` prefix when
they don't have any namespace at all. This allows for the
`PrefixingImageNameSubstitutor` to be used to redirect Docker Hub
requests to a private registry without having to apply otherwise
redundant changes to the test code.
@findepi findepi force-pushed the findepi/normalize-image-names-in-prefix-substitutor-af94c9 branch from e69a9ce to 3baf043 Compare May 17, 2024 20:51
@findepi findepi closed this Jun 20, 2024
@findepi findepi reopened this Jun 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants