-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Elasticsearch: Add test and documentation for secured cluster, bump default Elasticsearch version (deprecated constructor) from 6.4.1 to 7.9.2 #2320
Conversation
@@ -34,15 +34,15 @@ | |||
/** | |||
* Elasticsearch Default version | |||
*/ | |||
protected static final String ELASTICSEARCH_DEFAULT_VERSION = "6.4.1"; |
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.
Unfortunately I think we have to be careful about this; anybody using the plain ElasticsearchContainer()
constructor in their tests will get an unexpected major version bump.
We have a similar situation with other modules where the default version is getting old, and I would like to advance versions - but don't want to cause unexpected breakage.
@bsideup, @kiview let's discuss the most responsible course of action...
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.
Fortunately the documentation invites the user to use a specific version.
As we did not document the default constructor (which we should may be ban) I'd not be super concerned by this breaking change.
My 2 cents.
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you believe this is a mistake, please reply to this comment to keep it open. If there isn't one already, a PR to fix or at least reproduce the problem in a test case will always help us get back on track to tackle this. |
Not stale. Sorry for being slow to come back to this PR. |
With #2839 we're intending to deprecate the default constructor for all modules and start encouraging all users to specify a version explicitly. This is primarily to avoid the risk of us breaking users' builds by changing the docker image unexpectedly. As such, I'll close this PR, as I'd expect that we will leave default image tags where they are until they eventually get removed altogether. |
@rnorth I disagree because the change I proposed is bringing much more value than just updating the default version. So please consider reopening it. I 100% agree of removing the default constructor though but this is another story IMO. |
@@ -34,15 +34,15 @@ | |||
/** | |||
* Elasticsearch Default version | |||
*/ | |||
protected static final String ELASTICSEARCH_DEFAULT_VERSION = "6.4.1"; | |||
protected static final String ELASTICSEARCH_DEFAULT_VERSION = "7.5.2"; | |||
|
|||
public ElasticsearchContainer() { |
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.
Let's just mark this as @Deprecated
@@ -67,14 +77,34 @@ public void elasticsearchDefaultTest() throws IOException { | |||
} | |||
} | |||
|
|||
@Test | |||
public void elasticsearchSecuredTest() throws IOException { | |||
try (ElasticsearchContainer container = new ElasticsearchContainer() |
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.
We should just replace this default constructor method
@dadoonet, sorry, I missed your reply. I realise now what you mean - yes, we should bring the other elements of this PR in. Can we refactor this once #2839 is merged, so that the docs refer to the right (new) way of doing things? We should still leave the default version unchanged and deprecated, but there's absolutely nothing to stop the tests and docs referring to newer versions. Sorry for my misunderstanding. |
Thanks! I subscribed to the PR and will update the PR when merged. Although I could have done it before your merge. But no worries. |
991057c
to
a605c74
Compare
a605c74
to
f3345e3
Compare
f3345e3
to
c5be8dd
Compare
I rebased my work on master. Could you give another look? |
6ddb945
to
deb1245
Compare
deb1245
to
4ee7170
Compare
This can be now activated for free with the default basic license. Also change latest version to 7.9.2.
4ee7170
to
04e5366
Compare
I believe the branch is now ready for review as CI is happy :) |
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.
@dadoonet this looks great, thank you. Thanks for your patience on bumping the version of the docker image as well - I'm very happy with how this has turned out!
This can be now activated for free with the default basic license.