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

Image substitution #3102

Merged
merged 18 commits into from
Nov 5, 2020
Merged

Image substitution #3102

merged 18 commits into from
Nov 5, 2020

Conversation

rnorth
Copy link
Member

@rnorth rnorth commented Aug 16, 2020

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 TestcontainersConfiguration approach (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 TestcontainersConfiguration class is intentional: specifying image overrides in testcontainers.properties should be removed in the future.

  • Add log deprecation warnings when testcontainers.properties image overrides are used. Defer to a future release?


DockerImageName getConfiguredSubstituteImage(DockerImageName original) {
for (final Map.Entry<DockerImageName, String> entry : CONTAINER_MAPPING.entrySet()) {
if (original.isCompatibleWith(entry.getKey())) {
Copy link
Member Author

Choose a reason for hiding this comment

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

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 rnorth mentioned this pull request Aug 17, 2020
12 tasks
Base automatically changed from image-overrides to master September 29, 2020 10:02
@rnorth rnorth force-pushed the image-substitutor branch 2 times, most recently from 289c4e5 to c776755 Compare September 30, 2020 10:58
@rnorth
Copy link
Member Author

rnorth commented Sep 30, 2020

I've roughly rebased, but want to go through it again to make sure there are merge conflicts that I resolved the wrong way.

* @return the found value, or null if not set
*/
@Nullable
public String getEnvVarOrProperty(final String propertyName) {
Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

yes, let's keep reuse.enabled as an environment property 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

Would you be happy with every other property also being settable with an env var?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've done this as proposed.

* TODO: Javadocs
*/
@Slf4j
public class DefaultImageNameSubstitutor extends ImageNameSubstitutor {
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

FTR for others looking at this PR:
this comment is outdated, see #3413 for the prefixing substitutor

@rnorth rnorth changed the title WIP preview for image substitution Image substitution Oct 11, 2020
@rnorth rnorth marked this pull request as ready for review October 15, 2020 19:22
@rnorth rnorth requested a review from kiview as a code owner October 15, 2020 19:22
@rnorth
Copy link
Member Author

rnorth commented Oct 15, 2020

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.

@rnorth rnorth self-assigned this Oct 15, 2020
@@ -0,0 +1,5 @@
description = "Testcontainers :: Image Name Substitutors :: Prefxing"
Copy link
Member

Choose a reason for hiding this comment

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

empty module?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ergh, accidental commit. Will remove.

@@ -0,0 +1,37 @@
# Image Registry rate limiting
Copy link
Member Author

Choose a reason for hiding this comment

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

This page is still WIP

Copy link
Member Author

Choose a reason for hiding this comment

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

Reduced in content to this current form, which I think is just enough for this PR.

}
}

@Test @Ignore
Copy link
Member

Choose a reason for hiding this comment

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

just a random thought:

can't we make this test pass by substituting registry.mycompany.com/mirror/mysql with mysql (aka reverse substitution)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Very good idea - will do.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

userProperties.setProperty("docker.client.strategy", "foo");
assertEquals("Docker client strategy is changed by user property", "foo", newConfig().getDockerClientStrategyClassName());

userProperties.remove("docker.client.strategy");
Copy link
Member

Choose a reason for hiding this comment

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

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)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

import static org.rnorth.visibleassertions.VisibleAssertions.assertEquals;
import static org.testcontainers.utility.PrefixingImageNameSubstitutor.PROPERTY_KEY;

public class PrefixingImageNameSubstitutorTest {
Copy link
Member

Choose a reason for hiding this comment

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

we should also add a test for images from custom registries (e.g. mcr.microsoft.com/mssql/server), to define the behaviour

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed from this PR


@Before
public void setUp() {
mockConfiguration = mock(TestcontainersConfiguration.class);
Copy link
Member

Choose a reason for hiding this comment

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

JFYI we also have MockTestcontainersConfigurationRule available :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated to use it!

}

@Test
public void testServiceLoaderFindsDefaultImplementation() {
Copy link
Member

@bsideup bsideup Oct 29, 2020

Choose a reason for hiding this comment

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

duplicate of ImageNameSubstitutorTest#simpleServiceLoadingTest?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

if (original.equals(DockerImageName.parse("registry.mycompany.com/mirror/mysql:8.0.22"))) {
return defaultImageNameSubstitutor.apply(DockerImageName.parse("mysql"));
} else {
return defaultImageNameSubstitutor.apply(original);
Copy link
Member

Choose a reason for hiding this comment

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

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 :)

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@bsideup bsideup left a comment

Choose a reason for hiding this comment

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

🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants