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

Allow to disable configuration of EnvironmentAndSystemPropertyClientProviderStrategy through Testcontainers #4387

Merged
merged 10 commits into from
Sep 16, 2021

Conversation

kiview
Copy link
Member

@kiview kiview commented Aug 20, 2021

#4118 added the ability to configure EnvironmentAndSystemPropertyClientProviderStrategy through the Testcontainers configuration mechanism. However, in some advanced use cases, it can be desirable to keep the legacy behavior for configuring Docker host detection only.

This PR allows switching this behavior by configuring dockerconfig.source as either testcontainers (current behavior) or docker (legacy behavior).

In addition, EnvironmentAndSystemPropertyClientProviderStrategy is not considered a persistable strategy anymore. In most cases, this should effectively make no difference since it is the strategy with the highest priority of the strategies that are included in Testcontainers.

This high priority strategy property is also the reason, why this PR removes the code introduced by #4175, since it still will be selected by the discovery mechanism.

@kiview
Copy link
Member Author

kiview commented Aug 24, 2021

Once the new docker-java version is released, we can change the ENV check to the new getter from docker-java/docker-java#1692.

@rnorth
Copy link
Member

rnorth commented Aug 24, 2021

Shouldn't we add some unit tests for this? Our configuration/environment detection is pretty complex and regressions are especially painful to detect.

@kiview
Copy link
Member Author

kiview commented Aug 24, 2021

I was only thinking about TestcontainersConfigurationTest, which would not have captured this change, but we can add a test to EnvironmentAndSystemPropertyClientProviderStrategyTest (using Mocks), that makes sense 👍

@kiview
Copy link
Member Author

kiview commented Aug 24, 2021

@rnorth I added unit tests, that captures the behavior. But TBH, I am not sure if this will save us from regressions since they are extremely whitebox and IMO the regressions are most likely to be introduced through a combination of EnvironmentAndSystemPropertyClientProviderStrategy, TestcontainersConfiguration and DockerClientProviderStrategy in combination with the environment. This is also reflected by the fact that this unit test (with this corresponding implementation) was an overspecification that was already effectively enforced through the interaction of those components.

Another indicator for this test is too tightly connected to the implementation come from the fact, that we have to change the test (i.e. the mocking) once we switch to using the new getter in docker-java.

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.

👍

@bsideup
Copy link
Member

bsideup commented Sep 16, 2021

@kiview the code looks good. There are some CI failures tho - PTAL

@kiview
Copy link
Member Author

kiview commented Sep 16, 2021

With ❤️ from the Gradle Plugins server 🙄

@kiview kiview added this to the next milestone Sep 16, 2021
@kiview kiview merged commit 6cde564 into master Sep 16, 2021
@delete-merged-branch delete-merged-branch bot deleted the project-override-strategy branch September 16, 2021 10:45
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.

None yet

3 participants