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.

Sign up for GitHub

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

Jump to bottom

Elasticsearch: Add test and documentation for secured cluster, bump default Elasticsearch version (deprecated constructor) from 6.4.1 to 7.9.2 #2320

Merged
merged 1 commit into from Oct 15, 2020
Merged

Elasticsearch: Add test and documentation for secured cluster, bump default Elasticsearch version (deprecated constructor) from 6.4.1 to 7.9.2 #2320

merged 1 commit into from Oct 15, 2020

Conversation

dadoonet
Copy link
Contributor

@dadoonet dadoonet commented Feb 3, 2020 •
edited

This can be now activated for free with the default basic license.

@dadoonet dadoonet mentioned this pull request Feb 3, 2020
Merged
rnorth
rnorth reviewed Feb 7, 2020
View reviewed changes
...les/elasticsearch/src/main/java/org/testcontainers/elasticsearch/ElasticsearchContainer.java Outdated
@@ -34,15 +34,15 @@
/**
* Elasticsearch Default version
*/
protected static final String ELASTICSEARCH_DEFAULT_VERSION = "6.4.1";
Copy link
Member

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...

Copy link
Contributor Author

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.

@stale
Copy link

stale bot commented May 7, 2020

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.

@stale stale bot added the stale label May 7, 2020
@rnorth
Copy link
Member

rnorth commented May 8, 2020

Not stale. Sorry for being slow to come back to this PR.

@stale stale bot removed the stale label May 8, 2020
@rnorth rnorth mentioned this pull request Jun 5, 2020
Merged
7 tasks
@rnorth
Copy link
Member

rnorth commented Jun 18, 2020

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 rnorth closed this Jun 18, 2020
@dadoonet
Copy link
Contributor Author

@rnorth I disagree because the change I proposed is bringing much more value than just updating the default version.
It brings new tests, new documentation, etc...

So please consider reopening it.

I 100% agree of removing the default constructor though but this is another story IMO.

dadoonet
dadoonet commented Jun 26, 2020
View reviewed changes
...les/elasticsearch/src/main/java/org/testcontainers/elasticsearch/ElasticsearchContainer.java Outdated
@@ -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() {
Copy link
Contributor Author

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

dadoonet
dadoonet commented Jun 26, 2020
View reviewed changes
...elasticsearch/src/test/java/org/testcontainers/elasticsearch/ElasticsearchContainerTest.java Outdated
@@ -67,14 +77,34 @@ public void elasticsearchDefaultTest() throws IOException {
}
}

@Test
public void elasticsearchSecuredTest() throws IOException {
try (ElasticsearchContainer container = new ElasticsearchContainer()
Copy link
Contributor Author

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

@rnorth rnorth reopened this Jul 3, 2020
@rnorth
Copy link
Member

rnorth commented Jul 3, 2020

@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.

@dadoonet
Copy link
Contributor Author

dadoonet commented Jul 3, 2020

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.

@dadoonet dadoonet changed the title Update Elasticsearch versions (7.5.2 and 6.8.6) Add a test for secured cluster Jul 7, 2020
@dadoonet dadoonet changed the title Add a test for secured cluster Add test and documentation for secured cluster Jul 7, 2020
@dadoonet
Copy link
Contributor Author

dadoonet commented Jul 7, 2020

@rnorth I revisited the PR and removed all calls to the default CTOR and mark it as Deprecated so I think it's aligned with what you want to merge in master with #2839.

I'd be happy to hear your feedback.

@dadoonet dadoonet requested a review from rnorth July 7, 2020 13:01
@dadoonet dadoonet mentioned this pull request Jul 15, 2020
Closed
@dadoonet
Copy link
Contributor Author

I rebased my work on master. Could you give another look?

@dadoonet dadoonet force-pushed the update-es-versions branch 2 times, most recently from 6ddb945 to deb1245 Compare October 15, 2020 05:24
@dadoonet
Add a test for secured cluster
04e5366 
This can be now activated for free with the default basic license.
Also change latest version to 7.9.2.
@dadoonet
Copy link
Contributor Author

I believe the branch is now ready for review as CI is happy :)

rnorth
rnorth approved these changes Oct 15, 2020
View reviewed changes
Copy link
Member

@rnorth rnorth left a 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!

@rnorth rnorth changed the title Add test and documentation for secured cluster Elasticsearch: Add test and documentation for secured cluster, bump default Elasticsearch version (deprecated constructor) from 6.4.1 to 7.5.2 Oct 15, 2020
@rnorth rnorth added this to the next milestone Oct 15, 2020
@rnorth rnorth changed the title Elasticsearch: Add test and documentation for secured cluster, bump default Elasticsearch version (deprecated constructor) from 6.4.1 to 7.5.2 Elasticsearch: Add test and documentation for secured cluster, bump default Elasticsearch version (deprecated constructor) from 6.4.1 to 7.9.2 Oct 15, 2020
@rnorth rnorth merged commit 85f1e5e into testcontainers:master Oct 15, 2020
@dadoonet dadoonet deleted the update-es-versions branch October 16, 2020 08:41
@dadoonet dadoonet mentioned this pull request Oct 22, 2020
Closed
@eddumelendez eddumelendez mentioned this pull request Aug 24, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rnorth rnorth rnorth approved these changes

@bsideup bsideup Awaiting requested review from bsideup

@kiview kiview Awaiting requested review from kiview

Assignees
No one assigned
Labels
modules/elasticsearch resolution/acknowledged type/docs
Projects
None yet
Milestone
1.15.0
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@dadoonet @rnorth