use testcontainers directly instead of docker-testkit #4241
Conversation
andrew-nowak
force-pushed
the
an/modern-testcontainers
branch
from
7ec35a1 to
afc5fe0
Compare
|
I can see nothing that would present a problem for BBC devs. Thanks |
twrichards
left a comment
twrichards
left a comment
There was a problem hiding this comment.
LGTM - nice one @andrew-nowak 🙌
(note, I rebased #4287 on top to get it to build 🎉 - thank you)
| val esContainer: Option[ElasticsearchContainer] = if (useEsDocker) { | ||
| { | ||
| val container = new ElasticsearchContainer("docker.elastic.co/elasticsearch/elasticsearch:7.16.2") | ||
| .withExposedPorts(9200) | ||
| .withAccessToHost(true) | ||
| .withEnv(Map( | ||
| "cluster.name" -> "media-service", | ||
| "xpack.security.enabled" -> "false", | ||
| "discovery.type" -> "single-node", | ||
| "network.host" -> "0.0.0.0" | ||
| ).asJava) | ||
| .waitingFor(Wait.forHttp("/") | ||
| .forPort(9200) | ||
| .forStatusCode(200) | ||
| .withStartupTimeout(180.seconds.toJava) | ||
| ) | ||
| container.start() | ||
| Some(container) | ||
| } | ||
| } else None | ||
|
|
||
| val esPort = esContainer.map(_.getMappedPort(9200)).getOrElse(9200) | ||
| val es6TestUrl = Properties.envOrElse("ES6_TEST_URL", s"http://localhost:$esPort") | ||
|
|
||
| override protected def afterAll(): Unit = { | ||
| super.afterAll() | ||
|
|
||
| esContainer foreach { _.stop() } | ||
| } |
There was a problem hiding this comment.
any chance we could DRY this out and have this and thrall equivalent extend something in common? (appreciate it's not rule of 3, but seems like a lot of code with various config options therein which presents scope for divergence).
|
I had a similar issue with path-manager, which I fixed in guardian/path-manager#61 by adding an explicit dependency on a newer version of jnr-unixsocket |
Thanks that's a good find! However the docker lib is still unmaintained and these sorts of problems are gonna continue ramping up - so I think it's still worth replacing with the fresher/slimmer lib |
twrichards
left a comment
twrichards
left a comment
There was a problem hiding this comment.
nice, thanks for DRYing
Seems reasonable. If you were feeling keen and wanted to apply the same approach to path-manager, I'd be very grateful. |
|
Seen on auth, usage, metadata-editor, thrall, leases, cropper, collections, media-api, kahuna (merged by @andrew-nowak 9 minutes and 36 seconds ago) Please check your changes! |
|
Seen on image-loader (merged by @andrew-nowak 9 minutes and 48 seconds ago) Please check your changes! |
What does this change?
@Conalb97 raised that docker-testkit has some sort of incompatibility with M1 macs on certain JDKs. docker-testkit isn't receiving maintenance updates, and neither is the underlying docker client, so this is a good opportunity to replace it with a direct usage of testcontainers (mainly taking inspiration from Ophan https://github.com/guardian/ophan/blob/2ecb80da40915ce097e4a1de0e29b352b396323e/shared-lib/src/test/scala/app/BaseElasticTest.scala#L153 though we share the container between the tests, instead of a container per test, for a mild performance boost)