testcontainers-java
testcontainers-java copied to clipboard
Elasticsearch does not have a `-oss` artefact anymore
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.
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.
Do you see any potential for this breaking for users that have pull-through caches configured and will therefore continue using old
-osssuffixed 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?
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.
Is there anything else needed from my side to merge this?
I don't think so, just another maintainer (@bsideup or @rnorth) needs to review it.
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)?
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.
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.
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 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.
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 ;)
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.