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

Improve docs for Elasticsearch 8 #8870

Merged
merged 11 commits into from
Aug 7, 2024

Conversation

philipp94831
Copy link
Contributor

The docs for connecting to Elasticsearch container only work for Elasticsearch v7. For Elasticsearch v8 either the SSLContext has to be configured for the HTTPClient or SSL needs to be disabled. This PR adds according documentation

@kiview kiview self-assigned this Jul 12, 2024
// }}
assertThat(response.getStatusLine().getStatusCode()).isEqualTo(200);
assertThat(EntityUtils.toString(response.getEntity())).contains("cluster_name");
// httpClientContainer8 {{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// httpClientContainer8 {{
// httpClientContainer8 {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In line 211 it is also {{. Which one is correct?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be a single curly brace.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested it and it only works with a single curly brace here. Docs preview looks good to me now

.build();

Response response = client.performRequest(new Request("GET", "/_cluster/health"));
// }}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// }}
// }

// }}
assertThat(response.getStatusLine().getStatusCode()).isEqualTo(200);
assertThat(EntityUtils.toString(response.getEntity())).contains("cluster_name");
// httpClientContainer8NoSSL {{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// httpClientContainer8NoSSL {{
// httpClientContainer8NoSSL {

@eddumelendez
Copy link
Member

Hi @philipp94831, thanks for your contribution. I have added some suggestions that will fix how the code is render in the documentation, see https://deploy-preview-8870--testcontainers.netlify.app/modules/elasticsearch/

Also, regarding to the new tests I think restClientClusterHealthElasticsearch8 is the same as
testElasticsearch8SecureByDefault but it is reusing a method, which doesn't make a good candidate to be exposed in the docs. However, let's avoid duplication.

@eddumelendez eddumelendez removed this from the next milestone Jul 17, 2024
@philipp94831
Copy link
Contributor Author

Hi @philipp94831, thanks for your contribution. I have added some suggestions that will fix how the code is render in the documentation, see deploy-preview-8870--testcontainers.netlify.app/modules/elasticsearch

Also, regarding to the new tests I think restClientClusterHealthElasticsearch8 is the same as testElasticsearch8SecureByDefault but it is reusing a method, which doesn't make a good candidate to be exposed in the docs. However, let's avoid duplication.

What is your suggestion wrt duplication? I mainly added the tests in order to document the connection to ES 8. Because of the method used in the existing tests I deemed them unsuitable for documentation purposes

@kiview
Copy link
Member

kiview commented Jul 23, 2024

I think if we want to have restClientClusterHealthElasticsearch8 for docs purposes, we can remove testElasticsearch8SecureByDefault, WDYT @eddumelendez?

@eddumelendez
Copy link
Member

Sorry for the delay. Let's proceed with @kiview suggestions. Also, you can check docs locally by running docker compose up or when submitting the PR, check netlify/testcontainers/deploy-preview

@eddumelendez eddumelendez added this to the next milestone Aug 7, 2024
eddumelendez
eddumelendez previously approved these changes Aug 7, 2024
@eddumelendez
Copy link
Member

@philipp94831 can you please run ./gradlew spotlessApply?

@eddumelendez eddumelendez merged commit 4b3d08e into testcontainers:main Aug 7, 2024
100 checks passed
@eddumelendez
Copy link
Member

Thanks @philipp94831 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants