-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Relocate modules #607
Relocate modules #607
Conversation
Also enable parallel running of Jdbc Driver tests for speed reasons
114cf45
to
b97c19a
Compare
} | ||
|
||
// Do not break here, but step into the next iteration, where it will be verified with listImagesCmd(). | ||
// see https://github.com/docker/docker/issues/10708 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This issue is resolved as of Jan 2016 so I don't think we need to worry about it any more.
Accordingly, I've tidied the code a bit. Crucially the control flow now breaks out early if the image has been successfully pulled (line 96), instead of re-fetching the image listing.
This is actually important in letting us pull by sha256 hash: the listing doesn't include the same hash as the repository, so we can't use the listing to check for its presence (possibly something we can fix upstream in docker-java for better performance)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we also add a test for pulling with sha256 hash?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. While I was at it I tidied the image name handling code and added more tests relating to image names/image pulling.
modules/dynalite/README.md
Outdated
<dependency> | ||
<groupId>org.testcontainers</groupId> | ||
<artifactId>dynalite</artifactId> | ||
<version>1.4.3</version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to do something about these version numbers, preferably in an automated way...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or simply add a link to Maven Central with a note to check for the latest version there (Docker does it like this for docker-compose AFAIK).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done 👍
@@ -20,7 +21,7 @@ | |||
import static org.rnorth.visibleassertions.VisibleAssertions.assertEquals; | |||
import static org.rnorth.visibleassertions.VisibleAssertions.assertTrue; | |||
|
|||
@RunWith(Parameterized.class) | |||
@RunWith(ParallelParameterized.class) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ParallelParameterized
from junit-toolbox
lets us run these parameterised tests in parallel.
Before merging I'd like to have a bit of a discussion about whether this is a route we should take, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no experience with ParallelParameterized
, so no real opinion here 🙂
{"jdbc:tc:mariadb://hostname/databasename?useUnicode=yes&characterEncoding=utf8", false, true, false}, | ||
{"jdbc:tc:mariadb://hostname/databasename?TC_INITSCRIPT=somepath/init_mariadb.sql", true, false, false}, | ||
{"jdbc:tc:mariadb://hostname/databasename?TC_INITFUNCTION=org.testcontainers.jdbc.JDBCDriverTest::sampleInitFunction", true, false, false}, | ||
{"jdbc:tc:mariadb:10.1.16://hostname/databasename?TC_MY_CNF=somepath/mariadb_conf_override", false, false, true} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't added Oracle here due to different SQL syntax needed for validation (further consolidation/refactoring could follow).
private static final int APEX_HTTP_PORT = 8080; | ||
|
||
public OracleContainer() { | ||
super(IMAGE + "@sha256:15ff9ef50b4f90613c9780b589c57d98a8a9d496decec854316d96396ec5c085"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the Oracle image we use is owned by a third party and thus hard to reliably pin, I'd like to use the sha256 hash to lock down the version. Changes in RemoteDockerImage
permit this to work.
Why lock down the version? As we're importing this module into the main respository our stability, and ability to release, rely on all the modules builds staying repeatable. I'd like to do this with other dependencies too, but this one in particular is a crucial one.
b97c19a
to
9cd58d8
Compare
@StefanHufschmidt @mikeoswald I'd be grateful if you could check you're happy with this PR, and leave comments if there's anything you'd like to see changed! Thank you. |
Also enable use of sha256 hashes instead of image tags, for situations where image tags are not easily pinnable Extend pull/startup timeout for Oracle container
8261c01
to
9f1ca7b
Compare
Intended to eliminate `ORA-12516, TNS:listener could not find available handler` error
@@ -82,19 +83,20 @@ protected final String resolve() { | |||
} | |||
|
|||
if (attempts++ >= 3) { | |||
logger.error("Retry limit reached while trying to pull image: " + dockerImageName + ". Please check output of `docker pull " + dockerImageName + "`"); | |||
throw new ContainerFetchException("Retry limit reached while trying to pull image: " + dockerImageName); | |||
logger.error("Retry limit reached while trying to pull image: {}" + dockerImageName + ". Please check output of `docker pull {}`", dockerImageName, dockerImageName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the dockerImageName
concatenation in the middle is redundant now?
modules/dynalite/README.md
Outdated
<dependency> | ||
<groupId>org.testcontainers</groupId> | ||
<artifactId>dynalite</artifactId> | ||
<version>1.4.3</version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or simply add a link to Maven Central with a note to check for the latest version there (Docker does it like this for docker-compose AFAIK).
@@ -20,7 +21,7 @@ | |||
import static org.rnorth.visibleassertions.VisibleAssertions.assertEquals; | |||
import static org.rnorth.visibleassertions.VisibleAssertions.assertTrue; | |||
|
|||
@RunWith(Parameterized.class) | |||
@RunWith(ParallelParameterized.class) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no experience with ParallelParameterized
, so no real opinion here 🙂
@@ -148,7 +147,9 @@ public Connection createConnection(String queryString) throws SQLException { | |||
final Driver jdbcDriverInstance = getJdbcDriverInstance(); | |||
|
|||
try { | |||
return Unreliables.retryUntilSuccess(120, TimeUnit.SECONDS, () -> jdbcDriverInstance.connect(url, info)); | |||
return Unreliables.retryUntilSuccess(120, TimeUnit.SECONDS, () -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this 120 also go into a protected
method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call - done.
} | ||
|
||
// Do not break here, but step into the next iteration, where it will be verified with listImagesCmd(). | ||
// see https://github.com/docker/docker/issues/10708 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we also add a test for pulling with sha256 hash?
Add further tests for docker image name processing
.filter(Objects::nonNull) | ||
.flatMap(Stream::of) | ||
.map(DockerImageName::new) | ||
.forEach(AVAILABLE_IMAGE_NAME_CACHE::add); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
forEach
here is inefficient, let's use:
.collect(Collectors.toCollection(() -> AVAILABLE_IMAGE_NAME_CACHE));
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL yet another stream API usage :D
} | ||
|
||
private void doPullStartAndStop(String s) { | ||
final GenericContainer container = new GenericContainer<>(s); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please use try-with-resources here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
doPullStartAndStop("quay.io/coreos/etcd@sha256:39a30367cd1f3186d540a063ea0257353c8f81b0d3c920c87c7e0f602bb6197c"); | ||
} | ||
|
||
private void doPullStartAndStop(String s) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would use more explicit variable name like image
or something
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
|
||
@Test | ||
public void pullLatestImageFromPublicRegistryTest() { | ||
doPullStartAndStop("quay.io/coreos/etcd:latest"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we use this image already? If not, to improve the tests speed, we should use already used images (less downloads)
import org.junit.Test; | ||
import org.testcontainers.containers.GenericContainer; | ||
|
||
public class ImagePullTest { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this test seems to miss Parameterized runner a lot ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactored :D
private final GenericContainer delegate; | ||
|
||
public DynaliteContainer() { | ||
this("richnorth/dynalite:v1.2.1-1"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it's time to move this image to quay.io/testcontainers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea...
.map(Service::getPort) | ||
.collect(Collectors.toSet()).toArray(new Integer[]{}); | ||
|
||
delegate = new GenericContainer("atlassianlabs/localstack:0.6.0") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since this image is deprecated, maybe we should not add this module with it and replace with a recommended one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, this is done.
|
||
delegate = new GenericContainer("atlassianlabs/localstack:0.6.0") | ||
.withExposedPorts(portsList) | ||
.withFileSystemBind("/var/run/docker.sock", "/var/run/docker.sock", READ_WRITE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you will change this class, add double slash here (Windows issue)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
protected void configure() { | ||
|
||
addExposedPort(MS_SQL_SERVER_PORT); | ||
addEnv("ACCEPT_EULA", "Y"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it's legal or not, to set it automatically. We might have to make it explicit to avoid legal cases with M$
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very good point - I've added a simple license check mechanism (see new LicenseAcceptance
class). Acceptance is by adding a file to the classpath, so hopefully not too much of a burden.
My initial plan was to add an 'acceptEula' parameter to the constructor, but that falls apart when it comes to JDBC URL usage.
final URL url = Resources.getResource(ACCEPTANCE_FILE_NAME); | ||
final List<String> acceptedLicences = Resources.readLines(url, Charsets.UTF_8); | ||
|
||
if (acceptedLicences.stream().anyMatch(imageName::equals)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would also trim it
|
||
@Test | ||
public void test() { | ||
try (final GenericContainer __ = new GenericContainer<>(image)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we decide to stop pulling in constructor, this test will become a noop. We should call container.start()
inside the block to ensure that it actually starts with the provided image
|
||
public MSSQLServerContainer() { | ||
this(IMAGE + ":latest"); | ||
LicenseAcceptance.assertLicenseAccepted(IMAGE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should move this line right before addEnv("ACCEPT_EULA", "Y")
to make it even more explicit and also avoid ignoring the file when user passes a version with MSSQLServerContainer("microsoft/mssql-server-linux:1.2.3")
(the check is not included in MSSQLServerContainer(final String dockerImageName)
constructor)
|
||
Testcontainers module for the Oracle XE database. | ||
|
||
## Usage example |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also add here a note about licences?
steps: | ||
- checkout | ||
- run: ./gradlew check -x testcontainers:check -x selenium:check | ||
- run: | ||
command: ./gradlew jdbc-test:check |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bsideup I've split the CI build further so we now have four concurrent build jobs. I'm just wondering if I've missed anything: you'd excluded certain check tasks, whereas I've gone on an inclusive basis. This looks OK to me in local builds (i.e. it's compiling but not running tests on the core dependency, which seems correct).
circle.yml
Outdated
- run: | ||
command: ./gradlew jdbc-test:check | ||
environment: | ||
TZ: "/usr/share/zoneinfo/ETC/UTC" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding TZ
env var so that the Oracle JDBC driver doesn't cause an error. This is the first time we're testing the Oracle module on Circle CI 2.0, and it seems the first time that we've had to do work around this problem.
We could include the timezone setting as a workaround inside the OracleContainer
class, but I really think this would be the wrong place, as it's the driver that needs a timezone to be available. People using Oracle ought to be aware of this already or capable of resolving it.
final int slashIndex = name.indexOf('/'); | ||
|
||
String remoteName; | ||
if (slashIndex == -1 || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW this logic, and the regex patterns, were copied/translated from an official Docker library - I didn't figure these out myself 😂
... create a connection and run test as normal | ||
``` | ||
|
||
> *Note:* Due to licencing restrictions you are required to accept an EULA for this container image. To indicate that you accept the MS SQL Server image EULA, Please place a file at the root of the classpath named `container-license-acceptance.txt`, e.g. at `src/test/resources/container-license-acceptance.txt`. This file should contain the line: `microsoft/mssql-server-linux:latest` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@StefanHufschmidt I hope this is OK with you: we felt uncomfortable with setting the EULA acceptance env var automatically, so we've created this mechanism to allow devs to indicate their acceptance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's right I've noticed this as well but I had not such a nice idea with this license file. I think it is a nice solution! Well done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might throw an Exception in this module if the EULA is not accepted here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Throwing the exception is already implemented.
@bsideup I think I've addressed all your comments - WDYT now? |
circle.yml
Outdated
- run: | ||
command: ./gradlew testcontainers:check | ||
environment: | ||
TZ: "/usr/share/zoneinfo/ETC/UTC" |
There was a problem hiding this comment.
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 TZ env is needed anywhere but jdbc tests. Since some users check our CircleCI config for the inspiration, maybe we should remove it to keep it as small as possible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I was really in two minds about it. I put it in all for consistency, but instead I'll trim it down and have a comment explaining why it's there,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rnorth Approved with a minor comment about CircleCI
Oracle module was moved back to this repo in #607.
Bumps [amqp-client](https://github.com/rabbitmq/rabbitmq-java-client) from 5.6.0 to 5.7.0. <details> <summary>Release notes</summary> *Sourced from [amqp-client's releases](https://github.com/rabbitmq/rabbitmq-java-client/releases).* > ## 5.7.0 > # Changes between 5.6.0 and 5.7.0 > > This is a minor release with usability improvements and dependency upgrades. All users of the 5.x.x series are encouraged to upgrade to this version. > > ## Improve logging for TLS connections > > GitHub issue: [#441](https://github-redirect.dependabot.com/rabbitmq/rabbitmq-java-client/issues/441) > > ## Don't panic when cancelling an unknown consumer > > GitHub issue: [#525](https://github-redirect.dependabot.com/rabbitmq/rabbitmq-java-client/issues/525) > > ## Bump dependencies > > GitHub issue: [#607](https://github-redirect.dependabot.com/rabbitmq/rabbitmq-java-client/issues/607) > > ## 5.7.0.RC1 > # Changes between 5.6.0 and 5.7.0.RC1 > > This is a pre-release for 5.7.0, a maintenance release with usability improvements and dependency upgrades. All users of the 5.x.x series are encouraged to test this version. > > ## Improve logging for TLS connections > > GitHub issue: [#441](https://github-redirect.dependabot.com/rabbitmq/rabbitmq-java-client/issues/441) > > ## Don't panic when cancelling an unknown consumer > > GitHub issue: [#525](https://github-redirect.dependabot.com/rabbitmq/rabbitmq-java-client/issues/525) > > ## Bump dependencies > > GitHub issue: [#607](https://github-redirect.dependabot.com/rabbitmq/rabbitmq-java-client/issues/607) </details> <details> <summary>Changelog</summary> *Sourced from [amqp-client's changelog](https://github.com/rabbitmq/rabbitmq-java-client/blob/v5.7.0/release-versions.txt).* > RELEASE_VERSION="5.7.0" > DEVELOPMENT_VERSION="5.8.0-SNAPSHOT" </details> <details> <summary>Commits</summary> - [`57aa724`](rabbitmq/rabbitmq-java-client@57aa724) [maven-release-plugin] prepare release v5.7.0 - [`1928ce8`](rabbitmq/rabbitmq-java-client@1928ce8) Set release version to 5.7.0 - [`9f2dffe`](rabbitmq/rabbitmq-java-client@9f2dffe) [maven-release-plugin] prepare for next development iteration - [`e79ed4d`](rabbitmq/rabbitmq-java-client@e79ed4d) [maven-release-plugin] prepare release v5.7.0.RC1 - [`bb8f6ed`](rabbitmq/rabbitmq-java-client@bb8f6ed) Bump test dependencies - [`953d255`](rabbitmq/rabbitmq-java-client@953d255) Bump dependencies - [`e49f66d`](rabbitmq/rabbitmq-java-client@e49f66d) Add comment to explain decision tree - [`365dd22`](rabbitmq/rabbitmq-java-client@365dd22) Add test for Channel#basicCancel with unknown consumer tag - [`6081628`](rabbitmq/rabbitmq-java-client@6081628) Log warning when receiving basic.cancel for unknown consumer - [`be57b26`](rabbitmq/rabbitmq-java-client@be57b26) Merge pull request [#548](https://github-redirect.dependabot.com/rabbitmq/rabbitmq-java-client/issues/548) from spring-operator/polish-urls-apache-license-5.x.x... - Additional commits viewable in [compare view](rabbitmq/rabbitmq-java-client@v5.6.0...v5.7.0) </details> <br /> [![Dependabot compatibility score](https://api.dependabot.com/badges/compatibility_score?dependency-name=com.rabbitmq:amqp-client&package-manager=gradle&previous-version=5.6.0&new-version=5.7.0)](https://dependabot.com/compatibility-score.html?dependency-name=com.rabbitmq:amqp-client&package-manager=gradle&previous-version=5.6.0&new-version=5.7.0) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) If all status checks pass Dependabot will automatically merge this pull request. [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot ignore this [patch|minor|major] version` will close this PR and stop Dependabot creating any more for this minor/major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) - `@dependabot badge me` will comment on this PR with code to add a "Dependabot enabled" badge to your readme Additionally, you can set the following in the `.dependabot/config.yml` file in this repo: - Update frequency (including time of day and day of week) - Automerge options (never/patch/minor, and dev/runtime dependencies) - Pull request limits (per update run and/or open at any time) - Out-of-range updates (receive only lockfile updates, if desired) - Security updates (receive only security updates, if desired) Finally, you can contact us by mentioning @dependabot. </details>
Implements #564.
In general, this is a straight lift-and-shift of module code, and conversion of maven dependencies into Gradle.
For MS SQL Server and Vault modules I've migrated the licence and authors files, and tried to be very clear with attributions.
For the Oracle module I've implemented pull-by-sha rather than using the
latest
tag, for stability.With the database modules I've combined some of the tests into
jdbc-test
but this is a general cleanup exercise I'd like to come back to after this PR.