Skip to content
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

Merged
merged 6 commits into from Nov 30, 2023

Conversation

dadoonet
Copy link
Contributor

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.

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 dadoonet marked this pull request as ready for review October 12, 2021 16:16
@dadoonet
Copy link
Contributor Author

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.

kiview
kiview previously approved these changes Oct 12, 2021
Copy link
Member

@kiview kiview left a 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?

@dadoonet
Copy link
Contributor Author

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?

@kiview
Copy link
Member

kiview commented Oct 12, 2021

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.

This test checks that setting `xpack.security.enabled` still works on this version although it is set by default.
@dadoonet
Copy link
Contributor Author

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

@kiview
Copy link
Member

kiview commented Oct 13, 2021

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

@rnorth
Copy link
Member

rnorth commented Oct 14, 2021

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)?

@kiview
Copy link
Member

kiview commented Oct 14, 2021

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 kiview self-requested a review October 14, 2021 08:58
@dadoonet
Copy link
Contributor Author

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.

@eddumelendez eddumelendez changed the title Elasticsearch does not have a -oss artefact anymore Elasticsearch does not have a -oss artifact anymore Mar 1, 2023
@eddumelendez eddumelendez removed the type/breaking-api-change Public APIs are being changed label Nov 29, 2023
@eddumelendez eddumelendez changed the title Elasticsearch does not have a -oss artifact anymore Deprecate support for docker.elastic.co/elasticsearch/elasticsearch-oss Nov 30, 2023
@dadoonet
Copy link
Contributor Author

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

@eddumelendez eddumelendez added this to the next milestone Nov 30, 2023
@eddumelendez
Copy link
Member

@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 eddumelendez requested a review from a team as a code owner November 30, 2023 16:47
@eddumelendez eddumelendez added the type/deprecation Public APIs are being deprecated but not changed yet label Nov 30, 2023
@eddumelendez eddumelendez merged commit 1bfb966 into testcontainers:main Nov 30, 2023
88 checks passed
@dadoonet
Copy link
Contributor Author

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 dadoonet deleted the pr/no-oss-anymore branch November 30, 2023 17:11
@eddumelendez
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
modules/elasticsearch type/deprecation Public APIs are being deprecated but not changed yet type/docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants