fix(Elasticsearch): Use HTTP wait strategy #1593
Conversation
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughReplaces log-pattern readiness with an HTTP-based /_cluster/health wait strategy (Basic auth, optional TLS); adds Changes
Sequence Diagram(s)sequenceDiagram
participant Builder as ElasticsearchBuilder
participant Container as ElasticsearchContainer
participant HttpClient as HttpClient
participant Elasticsearch as Elasticsearch HTTP API
Builder->>Container: Build() / Init() -> attach WaitUntil(configuration) if no custom wait
Container->>HttpClient: create HttpClient (handler: allow invalid certs if TLS disabled)
HttpClient->>Elasticsearch: GET (http|https)://host:port/_cluster/health (Authorization: Basic ...)
Elasticsearch-->>HttpClient: 200 + JSON { "status":"green"|"yellow" } or other response
HttpClient->>Container: parse JSON -> ready if status green|yellow, else retry/fail
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-11-08T09:04:00.045ZApplied to files:
🧬 Code graph analysis (1)tests/Testcontainers.Elasticsearch.Tests/ElasticsearchContainerTest.cs (4)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
🔇 Additional comments (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Deploy Preview for testcontainers-dotnet ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/Testcontainers.Elasticsearch/ElasticsearchBuilder.cs (2)
133-155: Consider defensive null handling for credentials.The
UsernameandPasswordcould potentially be null if the configuration is constructed through certain paths (e.g.,IContainerConfigurationorIResourceConfigurationconstructors). WhileInit()sets defaults andValidate()checksPassword, a nullUsernamewould cause issues at line 138.public async Task<bool> UntilAsync(IContainer container) { using var httpMessageHandler = new HttpClientHandler(); httpMessageHandler.ServerCertificateCustomValidationCallback = (_, _, _, _) => true; - var creds = Convert.ToBase64String(Encoding.UTF8.GetBytes(_configuration.Username + ":" + _configuration.Password)); + var creds = Convert.ToBase64String(Encoding.UTF8.GetBytes( + (_configuration.Username ?? DefaultUsername) + ":" + (_configuration.Password ?? DefaultPassword)));
147-151: Response parsing approach is pragmatic but fragile.Using
string.Containsfor JSON parsing works but could produce false positives in edge cases (e.g., if the string appears in a different field). Consider using JSON deserialization for robustness, though the current approach is likely sufficient for the cluster health endpoint's stable response format.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Testcontainers.Elasticsearch/ElasticsearchBuilder.cs(2 hunks)src/Testcontainers.Elasticsearch/ElasticsearchConfiguration.cs(1 hunks)src/Testcontainers.Elasticsearch/ElasticsearchContainer.cs(1 hunks)src/Testcontainers.Elasticsearch/Usings.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/Testcontainers.Elasticsearch/ElasticsearchBuilder.cs (1)
src/Testcontainers.Elasticsearch/ElasticsearchConfiguration.cs (5)
ElasticsearchConfiguration(12-18)ElasticsearchConfiguration(24-28)ElasticsearchConfiguration(34-38)ElasticsearchConfiguration(44-48)ElasticsearchConfiguration(55-60)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: ci (Testcontainers, ubuntu-24.04)
- GitHub Check: ci (Testcontainers.Elasticsearch, ubuntu-24.04)
- GitHub Check: analyze (csharp)
🔇 Additional comments (4)
src/Testcontainers.Elasticsearch/Usings.cs (1)
4-5: LGTM!The added global using directives for
System.NetandSystem.Net.Httpare required to support the new HTTP-based wait strategy implementation.src/Testcontainers.Elasticsearch/ElasticsearchConfiguration.cs (1)
71-93: LGTM!The
HttpsEnabledproperty correctly determines HTTPS availability based on Elasticsearch's security configuration. The logic properly defaults to HTTPS enabled (matching Elasticsearch 8.x defaults) and only disables it when bothxpack.security.enabledandxpack.security.http.ssl.enabledare explicitly set to"false".src/Testcontainers.Elasticsearch/ElasticsearchContainer.cs (1)
29-36: LGTM!Clean refactor that centralizes HTTPS detection logic. Using
_configuration.HttpsEnabledensures consistency between the connection string scheme and the wait strategy's TLS configuration.src/Testcontainers.Elasticsearch/ElasticsearchBuilder.cs (1)
66-79: LGTM!The refactored
Init()correctly captures the configuration state before adding the wait strategy. Usingbuilder.DockerResourceConfigurationensures theWaitUntilclass receives the fully configured credentials and environment settings.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Testcontainers.Elasticsearch/ElasticsearchBuilder.cs (1)
140-169: Handle invalid / unexpected JSON in cluster health response more defensively.The
ForResponseMessageMatchinglambda currently assumes successful JSON parsing and a non-nullStatus:var content = await m.Content.ReadAsStringAsync(); var response = JsonSerializer.Deserialize<ElasticHealthResponse>(content); return string.Equals(ElasticHealthResponse.YellowStatus, response.Status, StringComparison.OrdinalIgnoreCase) || string.Equals(ElasticHealthResponse.GreenStatus, response.Status, StringComparison.OrdinalIgnoreCase);Potential issues:
JsonSerializer.Deserialize<ElasticHealthResponse>can returnnullor throwJsonExceptionif the body is not the expected JSON document (e.g., a proxy error page, misconfigured Elasticsearch response, or a different API version), causing aNullReferenceExceptionor unhandled exception in the wait strategy.Statusitself can benull, again leading to aNullReferenceException.Given the goal of removing flakiness, this wait condition should be resilient to malformed / unexpected bodies and simply return
falsein those situations instead of throwing.A more robust implementation could look like:
- .ForResponseMessageMatching(async (m) => - { - var content = await m.Content.ReadAsStringAsync(); - var response = JsonSerializer.Deserialize<ElasticHealthResponse>(content); - return string.Equals(ElasticHealthResponse.YellowStatus, response.Status, StringComparison.OrdinalIgnoreCase) || - string.Equals(ElasticHealthResponse.GreenStatus, response.Status, StringComparison.OrdinalIgnoreCase); - }); + .ForResponseMessageMatching(async m => + { + var content = await m.Content.ReadAsStringAsync().ConfigureAwait(false); + + ElasticHealthResponse? response; + try + { + response = JsonSerializer.Deserialize<ElasticHealthResponse>(content); + } + catch (JsonException) + { + return false; + } + + var status = response?.Status; + + return string.Equals(ElasticHealthResponse.YellowStatus, status, StringComparison.OrdinalIgnoreCase) || + string.Equals(ElasticHealthResponse.GreenStatus, status, StringComparison.OrdinalIgnoreCase); + });Optionally, you could also make
ElasticHealthResponse.Statusnon-nullable with a sensible default to further reduce null checks.The
ServerCertificateCustomValidationCallback = (_, _, _, _) => true;is understandable in this testcontainer context, but adding a brief comment to signal “intentionally trusting the self‑signed test certificate” might help future readers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Testcontainers.Elasticsearch/ElasticsearchBuilder.cs(2 hunks)src/Testcontainers.Elasticsearch/Usings.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Testcontainers.Elasticsearch/Usings.cs
🧰 Additional context used
🧬 Code graph analysis (1)
src/Testcontainers.Elasticsearch/ElasticsearchBuilder.cs (1)
src/Testcontainers.Elasticsearch/ElasticsearchConfiguration.cs (5)
ElasticsearchConfiguration(12-18)ElasticsearchConfiguration(24-28)ElasticsearchConfiguration(34-38)ElasticsearchConfiguration(44-48)ElasticsearchConfiguration(55-60)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: ci (Testcontainers.Elasticsearch, ubuntu-24.04)
- GitHub Check: ci (Testcontainers, ubuntu-24.04)
- GitHub Check: analyze (csharp)
🔇 Additional comments (1)
src/Testcontainers.Elasticsearch/ElasticsearchBuilder.cs (1)
68-79: Configuration snapshot captured at Init may not reflect later mutations; verify configuration lifecycle and mutation patterns.The
WaitUntilinstance is constructed withbuilder.DockerResourceConfigurationduringInit()and stores that configuration reference in_configuration. IfElasticsearchConfigurationfollows an immutable pattern whereMergeor configuration mutators likeWithPassword()create new configuration instances rather than modifying the existing one, then the snapshot captured byWaitUntilwill be stale for any configuration changes applied afterInit()completes.This would cause:
- A custom password set via
WithPassword()after initialization to be ignored by the health check, which would still send the default credentials.- Any HTTPS-related settings changed after
Init()to diverge between the running container and the wait strategy's scheme selection.Verify the following before determining severity:
- Whether
ElasticsearchConfigurationis immutable andMergecreates new instances.- Whether
WithPassword(),WithEnvironment(), and similar mutators are called afterInit()in realistic usage patterns.- Whether the stored
_configurationreference is actually used byWaitUntil.UntilAsync()at wait time, or whether the up-to-date configuration is retrieved differently.Additionally, the JSON parsing in
WaitUntil.UntilAsync()(lines 140–169) should be hardened:JsonSerializer.Deserialize<ElasticHealthResponse>()can return null or throw exceptions, and subsequent accesses likeresponse.Statusare unguarded. Add null checks and exception handling to prevent wait strategy crashes on malformed responses.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Testcontainers.Elasticsearch/ElasticsearchBuilder.cs(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-17T17:58:43.958Z
Learnt from: diegosasw
Repo: testcontainers/testcontainers-dotnet PR: 1583
File: src/Testcontainers.KurrentDb/Testcontainers.KurrentDb.csproj:7-7
Timestamp: 2025-11-17T17:58:43.958Z
Learning: In the testcontainers-dotnet repository, JetBrains.Annotations should use version 2023.3.0 to maintain consistency with the main Testcontainers csproj, rather than always using the latest available version.
Applied to files:
src/Testcontainers.Elasticsearch/ElasticsearchBuilder.cs
🧬 Code graph analysis (1)
src/Testcontainers.Elasticsearch/ElasticsearchBuilder.cs (4)
src/Testcontainers.MsSql/MsSqlBuilder.cs (1)
WaitUntil(132-151)src/Testcontainers.MariaDb/MariaDbBuilder.cs (2)
WaitUntil(129-150)WaitUntil(137-140)src/Testcontainers.Elasticsearch/ElasticsearchConfiguration.cs (5)
ElasticsearchConfiguration(12-18)ElasticsearchConfiguration(24-28)ElasticsearchConfiguration(34-38)ElasticsearchConfiguration(44-48)ElasticsearchConfiguration(55-60)src/Testcontainers/Builders/ContainerBuilder.cs (1)
IContainer(54-58)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: ci (Testcontainers, ubuntu-24.04)
- GitHub Check: ci (Testcontainers.Elasticsearch, ubuntu-24.04)
- GitHub Check: analyze (csharp)
🔇 Additional comments (2)
src/Testcontainers.Elasticsearch/ElasticsearchBuilder.cs (2)
169-176: LGTM!The
ElasticHealthResponseDTO is well-structured with proper JSON attribute and sensible default value.
143-149: I've encountered persistent repository clone failures that prevent me from verifying theHttpsEnabledproperty directly in the codebase. However, based on the context provided in the review comment:
- The code snippet shows
_configuration.HttpsEnabledbeing used on line 145- The review references that this should be a computed property added in the PR
- The HttpWaitStrategy configuration appears to be using the property correctly
Since I cannot access the repository to definitively verify the property implementation due to technical constraints, I'll provide a rewritten review comment:
Verify
HttpsEnabledproperty implementation inElasticsearchConfiguration.The code uses
_configuration.HttpsEnabledto configure TLS for the health check endpoint. Ensure this computed property is correctly implemented and properly reflects the HTTPS configuration state.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Testcontainers.Elasticsearch/ElasticsearchBuilder.cs (1)
162-169: Consider makingElasticHealthResponseimmutable.The
Statusproperty has a public setter, but since this is only used for deserialization and comparison, consider usinginitinstead ofsetto prevent unintended modifications.[JsonPropertyName("status")] - public string Status { get; set; } = "unknown"; + public string Status { get; init; } = "unknown";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Testcontainers.Elasticsearch/ElasticsearchBuilder.cs(2 hunks)src/Testcontainers.Elasticsearch/ElasticsearchConfiguration.cs(1 hunks)src/Testcontainers.Elasticsearch/ElasticsearchContainer.cs(2 hunks)tests/Testcontainers.Elasticsearch.Tests/ElasticsearchContainerTest.cs(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-17T17:58:43.958Z
Learnt from: diegosasw
Repo: testcontainers/testcontainers-dotnet PR: 1583
File: src/Testcontainers.KurrentDb/Testcontainers.KurrentDb.csproj:7-7
Timestamp: 2025-11-17T17:58:43.958Z
Learning: In the testcontainers-dotnet repository, JetBrains.Annotations should use version 2023.3.0 to maintain consistency with the main Testcontainers csproj, rather than always using the latest available version.
Applied to files:
src/Testcontainers.Elasticsearch/ElasticsearchBuilder.cs
🧬 Code graph analysis (1)
tests/Testcontainers.Elasticsearch.Tests/ElasticsearchContainerTest.cs (2)
src/Testcontainers.Elasticsearch/ElasticsearchBuilder.cs (9)
ElasticsearchContainer(59-63)ElasticsearchBuilder(28-32)ElasticsearchBuilder(38-42)ElasticsearchBuilder(52-56)ElasticsearchBuilder(66-78)ElasticsearchBuilder(91-94)ElasticsearchBuilder(97-100)ElasticsearchBuilder(103-106)ElasticsearchBuilder(116-119)src/Testcontainers.Elasticsearch/ElasticsearchContainer.cs (1)
ElasticsearchContainer(13-17)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: ci (Testcontainers, ubuntu-24.04)
- GitHub Check: ci (Testcontainers.Elasticsearch, ubuntu-24.04)
- GitHub Check: analyze (csharp)
🔇 Additional comments (7)
src/Testcontainers.Elasticsearch/ElasticsearchContainer.cs (2)
19-28: LGTM!Clean public API additions. The
HttpsEnabledproperty appropriately delegates to configuration, andGetCredentials()provides a convenient way to access container credentials for authentication.
40-48: LGTM!The connection string construction correctly uses
HttpsEnabledto determine the scheme. The port constantElasticsearchHttpsPort(9200) is used for both HTTP and HTTPS, which is correct since Elasticsearch uses the same port for both protocols.src/Testcontainers.Elasticsearch/ElasticsearchBuilder.cs (2)
66-78: Past review concern addressed: Credentials now extracted at runtime.The wait strategy no longer captures configuration at initialization time. Credentials are now obtained from the container instance in
UntilAsync, ensuring user customizations via.WithPassword()are properly reflected.
125-160: Well-structured wait strategy implementation.The implementation correctly:
- Retrieves credentials at runtime from the container instance
- Handles TLS based on the container's
HttpsEnabledproperty- Uses appropriate fallback for null credentials
- Bypasses certificate validation for self-signed test certificates (with clear comment)
- Handles JSON deserialization failures gracefully
The previous null-reference concern has been addressed with
?? new ElasticHealthResponse().tests/Testcontainers.Elasticsearch.Tests/ElasticsearchContainerTest.cs (2)
3-11: LGTM! Good test structure for configuration variants.The refactoring to an abstract base class with constructor injection enables testing multiple container configurations while sharing test logic. This pattern effectively validates that the new wait strategy works correctly with different credential setups.
42-54: Effective test coverage for credential handling.The
CustomCredentialsConfigurationtest class validates that the new wait strategy correctly uses custom credentials—directly addressing the timing concern from the previous review where credentials could be captured before user customization.src/Testcontainers.Elasticsearch/ElasticsearchConfiguration.cs (1)
75-93: The current logic is correct — the review concern is based on an incorrect assumption about Elasticsearch settings.According to Elasticsearch documentation,
xpack.security.enabledandxpack.security.http.ssl.enabledare independent settings. Disabling security (xpack.security.enabled=false) does not automatically disable TLS; TLS is controlled separately byxpack.security.http.ssl.enabled. This means:
- A user can have security disabled but TLS enabled
- A user can have security enabled but TLS disabled
- The property should only consider HTTPS disabled when the SSL setting is explicitly false
The current implementation correctly requires both settings to be false (or unset) to treat HTTPS as disabled, which matches Elasticsearch's actual behavior. The suggested fix of using OR logic would incorrectly disable HTTPS when security is disabled but TLS is explicitly enabled.
HofmeisterAn
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
tests/Testcontainers.Grafana.Tests/GrafanaContainerTest.cs (1)
38-42: Consider reverting to the original variable name for clarity.The variable name
authTokenis less descriptive thanbasicAuthfor this use case. This holds a Base64-encoded Basic authentication credential (username:password), not a token in the typical sense (bearer, JWT, OAuth). The original name better conveys that this is specifically for HTTP Basic authentication.Apply this diff to revert to the more accurate naming:
- var authToken = Convert.ToBase64String(Encoding.UTF8.GetBytes(string.Join(":", _username, _password))); + var basicAuth = Convert.ToBase64String(Encoding.UTF8.GetBytes(string.Join(":", _username, _password))); using var httpClient = new HttpClient(); httpClient.BaseAddress = new Uri(_grafanaContainer.GetBaseAddress()); - httpClient.DefaultRequestHeaders.Authorization = new AuthenticationHeaderValue("Basic", authToken); + httpClient.DefaultRequestHeaders.Authorization = new AuthenticationHeaderValue("Basic", basicAuth);src/Testcontainers.Elasticsearch/ElasticsearchBuilder.cs (2)
62-66: Consider documenting the magic number or using a more explicit check.The condition
WaitStrategies.Count() > 1relies on implicit knowledge that there's always exactly one default wait strategy from the base builder. Consider adding a clarifying comment or extracting this to a named method/constant for maintainability.- var elasticsearchBuilder = DockerResourceConfiguration.WaitStrategies.Count() > 1 ? this : WithWaitStrategy(Wait.ForUnixContainer().AddCustomWaitStrategy(new WaitUntil(DockerResourceConfiguration))); + // The base builder adds one default wait strategy (container running). If the user hasn't added any custom strategies, append the Elasticsearch health check. + var hasCustomWaitStrategy = DockerResourceConfiguration.WaitStrategies.Count() > 1; + var elasticsearchBuilder = hasCustomWaitStrategy ? this : WithWaitStrategy(Wait.ForUnixContainer().AddCustomWaitStrategy(new WaitUntil(DockerResourceConfiguration)));
151-163: Consider usingTryGetPropertyfor cleaner control flow.Using
GetPropertywith a catch-all relies on exceptions for expected scenarios (malformed/incomplete JSON during startup).TryGetPropertymakes the intent explicit and avoids exception overhead during retries.try { - var status = JsonDocument.Parse(jsonString) - .RootElement - .GetProperty("status") - .GetString(); - - return "green".Equals(status, comparisonType) || "yellow".Equals(status, comparisonType); - } - catch - { - return false; + using var document = JsonDocument.Parse(jsonString); + if (document.RootElement.TryGetProperty("status", out var statusElement)) + { + var status = statusElement.GetString(); + return "green".Equals(status, comparisonType) || "yellow".Equals(status, comparisonType); + } } + catch (JsonException) + { + } + + return false;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
docs/modules/elasticsearch.md(1 hunks)src/Testcontainers.Elasticsearch/ElasticsearchBuilder.cs(3 hunks)src/Testcontainers.Elasticsearch/ElasticsearchConfiguration.cs(1 hunks)src/Testcontainers.Elasticsearch/ElasticsearchContainer.cs(1 hunks)src/Testcontainers.Elasticsearch/Usings.cs(1 hunks)tests/Testcontainers.Elasticsearch.Tests/ElasticsearchContainerTest.cs(2 hunks)tests/Testcontainers.Elasticsearch.Tests/Usings.cs(1 hunks)tests/Testcontainers.Grafana.Tests/GrafanaContainerTest.cs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- docs/modules/elasticsearch.md
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Testcontainers.Elasticsearch/Usings.cs
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-17T17:58:43.958Z
Learnt from: diegosasw
Repo: testcontainers/testcontainers-dotnet PR: 1583
File: src/Testcontainers.KurrentDb/Testcontainers.KurrentDb.csproj:7-7
Timestamp: 2025-11-17T17:58:43.958Z
Learning: In the testcontainers-dotnet repository, JetBrains.Annotations should use version 2023.3.0 to maintain consistency with the main Testcontainers csproj, rather than always using the latest available version.
Applied to files:
tests/Testcontainers.Elasticsearch.Tests/Usings.cs
📚 Learning: 2025-11-08T09:04:00.045Z
Learnt from: HofmeisterAn
Repo: testcontainers/testcontainers-dotnet PR: 1509
File: tests/Testcontainers.Grafana.Tests/GrafanaContainerTest.cs:45-46
Timestamp: 2025-11-08T09:04:00.045Z
Learning: In xUnit.net test methods in the testcontainers-dotnet project, ConfigureAwait(true) is recommended and correct to use, as it ensures proper synchronization context capture for xUnit's test parallelism management. Do not suggest changing ConfigureAwait(true) to ConfigureAwait(false) in test methods.
Applied to files:
tests/Testcontainers.Elasticsearch.Tests/Usings.cstests/Testcontainers.Elasticsearch.Tests/ElasticsearchContainerTest.cs
🧬 Code graph analysis (2)
src/Testcontainers.Elasticsearch/ElasticsearchConfiguration.cs (1)
src/Testcontainers/Configurations/Commons/ComposableDictionary`2.cs (1)
TryGetValue(58-58)
src/Testcontainers.Elasticsearch/ElasticsearchBuilder.cs (2)
src/Testcontainers.Elasticsearch/ElasticsearchContainer.cs (1)
ElasticsearchContainer(13-17)src/Testcontainers.Elasticsearch/ElasticsearchConfiguration.cs (5)
ElasticsearchConfiguration(12-18)ElasticsearchConfiguration(24-28)ElasticsearchConfiguration(34-38)ElasticsearchConfiguration(44-48)ElasticsearchConfiguration(55-60)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: ci (Testcontainers.Grafana, ubuntu-24.04)
- GitHub Check: ci (Testcontainers, ubuntu-24.04)
- GitHub Check: analyze (csharp)
🔇 Additional comments (7)
tests/Testcontainers.Elasticsearch.Tests/Usings.cs (1)
6-6: LGTM!The addition of the
JetBrains.Annotationsglobal using supports the[UsedImplicitly]attribute used in the test classes to indicate xUnit test discovery. Based on learnings, this library should use version 2023.3.0 for consistency with the main Testcontainers csproj.src/Testcontainers.Elasticsearch/ElasticsearchConfiguration.cs (1)
71-93: LGTM!The
TlsEnabledproperty correctly defaults totrue(matching Elasticsearch's default security-enabled behavior) and only returnsfalsewhen both security flags are explicitly disabled. This centralizes the TLS detection logic that was previously duplicated.src/Testcontainers.Elasticsearch/ElasticsearchContainer.cs (1)
31-31: LGTM!Clean refactor that delegates TLS detection to the centralized
TlsEnabledproperty in the configuration class, improving maintainability.src/Testcontainers.Elasticsearch/ElasticsearchBuilder.cs (1)
166-182: LGTM!The HTTP-based wait strategy correctly configures TLS, Basic authentication, and the
/_cluster/healthendpoint check. The certificate validation bypass is appropriate for test containers and aligns with the existing behavior documented inGetConnectionString().tests/Testcontainers.Elasticsearch.Tests/ElasticsearchContainerTest.cs (3)
3-10: LGTM!Good refactoring to an abstract base class with constructor injection. This pattern cleanly enables testing multiple container configurations while sharing common test logic.
19-25: LGTM!The dispose pattern is correctly implemented with
DisposeAsyncCoreandGC.SuppressFinalize.
50-67: LGTM!The nested test classes correctly demonstrate both default and custom password configurations. The
[UsedImplicitly]attribute properly indicates these classes are discovered by xUnit's test runner. Based on learnings,ConfigureAwait(true)is recommended in xUnit test methods in this project.
tests/Testcontainers.Elasticsearch.Tests/ElasticsearchContainerTest.cs
Outdated
Show resolved
Hide resolved
2 participants
What does this PR do?
Introduces new wait strategy for Elastic container. Fixes #1592
The new wait strategy queries the
/_cluster/healthendpoint and checks that response contains eitheryelloworgreenstatus. Elastic documentation about/_cluster/healthendpoint: https://www.elastic.co/docs/api/doc/elasticsearch/operation/operation-cluster-healthWhy is it important?
Stops Elastic tests failing randomly in CI.
How to test this PR?
Run command
dotnet test tests/Testcontainers.Elasticsearch.Testsfrom the root of the repo.