Conversation
|
|
||
| DockerImageName getConfiguredSubstituteImage(DockerImageName original) { | ||
| for (final Map.Entry<DockerImageName, String> entry : CONTAINER_MAPPING.entrySet()) { | ||
| if (original.isCompatibleWith(entry.getKey())) { |
There was a problem hiding this comment.
Risk: if so-called tiny image (alpine:3.5) is overridden in config, all usages of that image will be replaced. Substitution is now along image-identity lines rather than semantic lines.
rnorth
force-pushed
the
image-substitutor
branch
2 times, most recently
from
289c4e5 to
c776755
Compare
|
I've roughly rebased, but want to go through it again to make sure there are merge conflicts that I resolved the wrong way. |
core/src/main/java/org/testcontainers/containers/SocatContainer.java
Outdated
Show resolved
Hide resolved
| * @return the found value, or null if not set | ||
| */ | ||
| @Nullable | ||
| public String getEnvVarOrProperty(final String propertyName) { |
There was a problem hiding this comment.
This is currently only used by the PrefixingImageNameSubstitutor, but I'm very tempted to use it for most of the other properties that are configurable.
I think most of the properties would benefit from having an equivalent environment variable.
I know we discussed having the reuse.enabled as a config file property instead of an env var some time back, so perhaps we'd leave that as is?
There was a problem hiding this comment.
yes, let's keep reuse.enabled as an environment property 👍
There was a problem hiding this comment.
Would you be happy with every other property also being settable with an env var?
There was a problem hiding this comment.
I've done this as proposed.
| * TODO: Javadocs | ||
| */ | ||
| @Slf4j | ||
| public class DefaultImageNameSubstitutor extends ImageNameSubstitutor { |
There was a problem hiding this comment.
I've made this as a default out-of-the-box substitutor that delegates to both the config file settings and a simple 'prefix' substitutor which can apply a common prefix to all image names.
I suspect this might be enough for 80% of situations where people need a substitutor, which is why I think it's worth including it by default.
There was a problem hiding this comment.
FTR for others looking at this PR:
this comment is outdated, see #3413 for the prefixing substitutor
|
Marking as ready for review now despite docs not being done yet - I'll make a proper effort at the documentation when we've agreed any changes to the API/behaviour. |
| @@ -0,0 +1,5 @@ | |||
| description = "Testcontainers :: Image Name Substitutors :: Prefxing" | |||
There was a problem hiding this comment.
Ergh, accidental commit. Will remove.
| @@ -0,0 +1,37 @@ | |||
| # Image Registry rate limiting | |||
There was a problem hiding this comment.
Reduced in content to this current form, which I think is just enough for this PR.
rnorth
force-pushed
the
image-substitutor
branch
from
a60b5ff to
da114db
Compare
modules/oracle-xe/src/main/java/org/testcontainers/containers/OracleContainer.java
Show resolved
Hide resolved
| } | ||
| } | ||
|
|
||
| @Test @Ignore |
There was a problem hiding this comment.
just a random thought:
can't we make this test pass by substituting registry.mycompany.com/mirror/mysql with mysql (aka reverse substitution)?
There was a problem hiding this comment.
Very good idea - will do.
| userProperties.setProperty("docker.client.strategy", "foo"); | ||
| assertEquals("Docker client strategy is changed by user property", "foo", newConfig().getDockerClientStrategyClassName()); | ||
|
|
||
| userProperties.remove("docker.client.strategy"); |
There was a problem hiding this comment.
WDYT about either adding an extra assert after the property is removed (to ensure that it is not cached, for example) or splitting the test into 2 (user properties, environment)?
| import static org.rnorth.visibleassertions.VisibleAssertions.assertEquals; | ||
| import static org.testcontainers.utility.PrefixingImageNameSubstitutor.PROPERTY_KEY; | ||
|
|
||
| public class PrefixingImageNameSubstitutorTest { |
There was a problem hiding this comment.
we should also add a test for images from custom registries (e.g. mcr.microsoft.com/mssql/server), to define the behaviour
|
|
||
| @Before | ||
| public void setUp() { | ||
| mockConfiguration = mock(TestcontainersConfiguration.class); |
There was a problem hiding this comment.
JFYI we also have MockTestcontainersConfigurationRule available :)
| } | ||
|
|
||
| @Test | ||
| public void testServiceLoaderFindsDefaultImplementation() { |
There was a problem hiding this comment.
duplicate of ImageNameSubstitutorTest#simpleServiceLoadingTest?
core/src/main/java/org/testcontainers/utility/PrefixingImageNameSubstitutor.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/testcontainers/utility/DefaultImageNameSubstitutor.java
Show resolved
Hide resolved
.../examples/junit4/generic/src/test/java/generic/support/TestSpecificImageNameSubstitutor.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/testcontainers/utility/ConfigurationFileImageNameSubstitutor.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/testcontainers/utility/DefaultImageNameSubstitutor.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/testcontainers/utility/ConfigurationFileImageNameSubstitutor.java
Outdated
Show resolved
Hide resolved
core/src/test/java/org/testcontainers/utility/FakeImageSubstitutor.java
Outdated
Show resolved
Hide resolved
core/src/test/java/org/testcontainers/utility/FakeImageSubstitutor.java
Outdated
Show resolved
Hide resolved
| if (original.equals(DockerImageName.parse("registry.mycompany.com/mirror/mysql:8.0.22"))) { | ||
| return defaultImageNameSubstitutor.apply(DockerImageName.parse("mysql")); | ||
| } else { | ||
| return defaultImageNameSubstitutor.apply(original); |
There was a problem hiding this comment.
is it still needed, with the latest change where we always apply the default?
It is a nit but, since we refer to it from the docs, serves as an important example :)
There was a problem hiding this comment.
I just realized... DefaultImageNameSubstitutor is internal, so we definitely should avoid using it in this example :) I would even move it to some other package to ensure that everyone can write a custom substitutor
There was a problem hiding this comment.
Argh, good point. Removed, which also means that I can move this class back out of the org.testcontainers.utility package because there's no longer a dependency on the default substitutor.
…move duplicate use of default substitutor
2 participants
Builds upon #3021:
adds a pluggable image substitution mechanism using ServiceLoader, enabling users to perform custom substitution/auditing of images being used by their tests
provides a default implementation that behaves similarly to legacy
TestcontainersConfigurationapproach (testcontainers.properties)Notes:
behaviour is similar but not quite identical to
TestcontainersConfiguration: use of a configured custom image for, e.g. Kafka/Pulsar that does not have a tag specified causes the substitution to take effect for all usages. It seems very unlikely that people would be using a mix of the config file image overrides in some places and specific images specified in code in others.Duplication of default image names in modules vs
TestcontainersConfigurationclass is intentional: specifying image overrides intestcontainers.propertiesshould be removed in the future.Add log deprecation warnings whenDefer to a future release?testcontainers.propertiesimage overrides are used.