testcontainers-java icon indicating copy to clipboard operation
testcontainers-java copied to clipboard

Elasticsearch does not have a `-oss` artefact anymore

Open dadoonet opened this issue 4 years ago • 8 comments

Since version 7.11.0, there is no more -oss artefact anymore. This simplifies the code as we don't need to check anymore what is available and what is not.

dadoonet avatar Oct 12 '21 16:10 dadoonet

As a follow up for this PR, I'd like to:

  • Remove the default CTOR public ElasticsearchContainer()
  • Remove the TransportPort. It has been deprecated for a long time so people are now aware of it.
  • Use the new REST Client for tests which uses much lesser dependencies than the current one. This can be done only if we remove the TransportPort.
  • Add a new helper to ask for a specific type of license. Default is basic, but we can add support for a trial license in case someone wants to test some specific features like some ML features.
  • Provide by default a secured elasticsearch with a default test password. That will be the case with Elasticsearch 8.0 so we can prepare users to this.

But the first step IMO is this PR. Let me know if you need more info.

dadoonet avatar Oct 12 '21 16:10 dadoonet

Do you see any potential for this breaking for users that have pull-through caches configured and will therefore continue using old -oss suffixed images?

Actually, if they are updating TC, I believe that

dockerImageName.assertCompatibleWith(DEFAULT_IMAGE_NAME);

will fail.

Previous code was:

dockerImageName.assertCompatibleWith(DEFAULT_IMAGE_NAME, DEFAULT_OSS_IMAGE_NAME);

I could keep that old value around, but add a log.warn saying that -oss is now deprecated... WDYT?

dadoonet avatar Oct 12 '21 16:10 dadoonet

What would fail is if they used their own image that was configured as a compatible substitute for docker.elastic.co/elasticsearch/elasticsearch-oss.

Since we now change the container implementation to assume an image compatible to docker.elastic.co/elasticsearch/elasticsearch (meaning setting of password is allowed, since we also removed this check), I think changing the assert to break for users and thereby communicating explicitly the need to use a docker.elastic.co/elasticsearch/elasticsearch compatible image is correct.

kiview avatar Oct 12 '21 16:10 kiview

Is there anything else needed from my side to merge this?

dadoonet avatar Oct 13 '21 10:10 dadoonet

I don't think so, just another maintainer (@bsideup or @rnorth) needs to review it.

kiview avatar Oct 13 '21 11:10 kiview

Can I just make sure I understand this correctly: won't this break for users who are currently doing:

new ElasticsearchContainer("docker.elastic.co/elasticsearch/elasticsearch-oss:7.9.2")

(or any other tagged version)?

rnorth avatar Oct 14 '21 08:10 rnorth

You are right, we actually need to keep the checks for versions < 7.11.0. Unless we want to break compatibility and in this case, new assumptions are at least clearly communicated to users, since the image compatibility assert will fail for them.

kiview avatar Oct 14 '21 08:10 kiview

I reverted some parts of the change so we can still support -oss until 7.10.2 but we are warning the user that he should switch to the default distribution.

dadoonet avatar Oct 14 '21 12:10 dadoonet

I'd suggest to use 7.17.15 as the latest 7.x version: https://www.elastic.co/downloads/past-releases/elasticsearch-7-17-15

dadoonet avatar Nov 30 '23 16:11 dadoonet

@dadoonet thanks for the suggestion, but default constructor is deprecated and if by any chance there is someone still using it there will be a surprise by using a different image. Users must declare the image to use.

eddumelendez avatar Nov 30 '23 16:11 eddumelendez

Ha! Indeed. I misread the PR. You actually reverted the change to 7.15. So all good on my end. Thansk for moving forward this PR ;)

dadoonet avatar Nov 30 '23 17:11 dadoonet

Thanks for your contribution, @dadoonet ! Let's discuss via slack or using GH discussions the next steps proposed in the first comment. I think some of them were already addressed.

eddumelendez avatar Nov 30 '23 17:11 eddumelendez