-
-
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 withPassword(String) method for secure access #2321
Conversation
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 |
d987545
to
877aa4f
Compare
public class ElasticsearchContainerTest { | ||
|
||
/** | ||
* Elasticsearch version which should be used for the Tests | ||
*/ | ||
private static final String ELASTICSEARCH_VERSION = Version.CURRENT.toString(); | ||
private static final String ELASTICSEARCH_VERSION = "7.8.0"; |
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.
Ideally we should get this from the Gradle definition. Is that possible?
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.
Depending on the module, it's not always the case that there's a 1:1 mapping between driver and image versions, but if there is for Elasticsearch that's quite nice. It could help keep our tests up to date.
We do this for Selenium, for which there's historically been a very tight coupling between driver version and required image version.
For the Selenium module, the code that does it is here:
testcontainers-java/modules/selenium/src/main/java/org/testcontainers/containers/SeleniumUtils.java
Line 17 in 923472f
public final class SeleniumUtils { |
If you wanted to use this for inspiration (maybe change SeleniumUtils
to a general-purpose class inside our test-support
module), perhaps it could work. We'd only want to use this for Testcontainers' internal tests, though - i.e. deprecation of the default-args constructor still applies.
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.
Right. I think that this should come within another PR so we don't overcomplicate things.
BTW I realized lately that I should have commented in #2320 and not here :(
8d3bbf3
to
a6c00c5
Compare
3e17a3e
to
e681a97
Compare
#2320 is merged, so this can be rebased now 🙂 |
Instead of providing env settings manually, we can simplify the usage of Elasticsearch in the context of TestContainers by just asking a password. Behind the scene, we do provide the needed env settings. We also check that we can not define the password on OSS image.
e681a97
to
23878f9
Compare
I added a missing test. I think it's now ready for review. :) |
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.
Looks good to me!
Does this need another review to be merged? |
@dadoonet I was just waiting to clear an issue with our master branch Windows CI executor. Should be good to go now. Thank you! |
Instead of providing env settings manually, we can simplify the usage of Elasticsearch in the context of TestContainers by just asking a password.
Behind the scene, we do provide the needed env settings.
This PR comes after #2320 and should be probably rebased on master when 2320 will be merged.