Dev Services - Mark configured images as compatible with default ones #28198
Conversation
quarkus-bot
bot
added
area/elasticsearch
area/kafka
area/reactive-messaging
area/smallrye
labels
|
/cc @loicmathieu, @yrodiere |
There was a problem hiding this comment.
LGTM, at least for the Elasticsearch part, though I have a few additional suggestions.
Also, it's missing the hardest part: tests. They were the reason I didn't take 2 minutes to tackle this in the first place: it'll take way more than 2 minutes downloading the various images :)
| @@ -197,7 +197,7 @@ private DevServicesResultBuildItem.RunningDevService startElasticsearch( | |||
| // Starting the server | |||
| final Supplier<DevServicesResultBuildItem.RunningDevService> defaultElasticsearchSupplier = () -> { | |||
| ElasticsearchContainer container = new ElasticsearchContainer( | |||
| DockerImageName.parse(config.imageName)); | |||
| DockerImageName.parse(config.imageName).asCompatibleSubstituteFor("elasticsearch/elasticsearch")); | |||
There was a problem hiding this comment.
We probably also need elasticsearch here (or is it _/elasticsearch? I mean the one with no prefix, see https://hub.docker.com/_/elasticsearch/ vs. https://hub.docker.com/elastic/elasticsearch/).
Ideally we'd also allow OpenSearch, since our stack is compatible with it (except for the high-level client, maybe). But that may be a bit more adventurous. Relevant images:
opensearchproject/opensearch(see https://hub.docker.com/r/opensearchproject/opensearch)public.ecr.aws/opensearchproject/opensearch(see https://opensearch.org/docs/latest/opensearch/install/docker/)
There was a problem hiding this comment.
I'm not so sure. The whole purpose of it is to have ElasticsearchContainer to not complain about the custom image. And it is actually just checking that it's compatible with the default one.
We just want to pass:
dockerImageName.assertCompatibleWith(DEFAULT_IMAGE_NAME, DEFAULT_OSS_IMAGE_NAME);
There was a problem hiding this comment.
Ok, well if you think that's enough I'll trust you.
There was a problem hiding this comment.
That worked for me:
DockerImageName.parse(config.imageName).asCompatibleSubstituteFor("docker.elastic.co/elasticsearch/elasticsearch"));
Just My 2 Cents.
|
Also, did you check that |
|
I wanted to run our tests but they are not using dev services so I went with the quickstart and it worked so I think we are safe. |
Fixes #27862