-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Deprecate support for docker.elastic.co/elasticsearch/elasticsearch-oss #4574
Conversation
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:
But the first step IMO is this PR. Let me know if you need more info. |
...les/elasticsearch/src/main/java/org/testcontainers/elasticsearch/ElasticsearchContainer.java
Outdated
Show resolved
Hide resolved
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 very nice and simplifies the module as well as UX for Testcontainers and Elasticsearch users. 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 |
What would fail is if they used their own image that was configured as a compatible substitute for Since we now change the container implementation to assume an image compatible to |
This test checks that setting `xpack.security.enabled` still works on this version although it is set by default.
Is there anything else needed from my side to merge this? |
Can I just make sure I understand this correctly: won't this break for users who are currently doing:
(or any other tagged version)? |
You are right, we actually need to keep the checks for versions < 7.11.0. |
I reverted some parts of the change so we can still support |
-oss
artefact anymore-oss
artifact anymore
...les/elasticsearch/src/main/java/org/testcontainers/elasticsearch/ElasticsearchContainer.java
Outdated
Show resolved
Hide resolved
...les/elasticsearch/src/main/java/org/testcontainers/elasticsearch/ElasticsearchContainer.java
Outdated
Show resolved
Hide resolved
...les/elasticsearch/src/main/java/org/testcontainers/elasticsearch/ElasticsearchContainer.java
Outdated
Show resolved
Hide resolved
...elasticsearch/src/test/java/org/testcontainers/elasticsearch/ElasticsearchContainerTest.java
Outdated
Show resolved
Hide resolved
...elasticsearch/src/test/java/org/testcontainers/elasticsearch/ElasticsearchContainerTest.java
Outdated
Show resolved
Hide resolved
-oss
artifact anymore
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. |
…ss (testcontainers#4574) Since version 7.11.0, there is no more `docker.elastic.co/elasticsearch/elasticsearch-oss` artifact anymore. --------- Co-authored-by: Eddú Meléndez Gonzales <eddu.melendez@gmail.com>
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.