fix(Nats): Use healthz API for readiness probe#1679
fix(Nats): Use healthz API for readiness probe#1679HofmeisterAn merged 3 commits intotestcontainers:developfrom
Conversation
✅ Deploy Preview for testcontainers-dotnet ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughReplaced the NATS container readiness check from a log-based wait for "Server is ready" to an HTTP-based wait that polls Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/Testcontainers.Nats/NatsBuilder.cs (1)
113-113: Use the existing port constant in the HTTP wait strategy.This works, but replacing the literal
8222withNatsHttpManagementPortavoids duplication and future drift.♻️ Suggested change
- .WithWaitStrategy(Wait.ForUnixContainer().UntilHttpRequestIsSucceeded(request => request.ForPort(8222).ForPath("/healthz"))); + .WithWaitStrategy(Wait.ForUnixContainer().UntilHttpRequestIsSucceeded(request => request.ForPort(NatsHttpManagementPort).ForPath("/healthz")));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Testcontainers.Nats/NatsBuilder.cs` at line 113, Replace the hard-coded port 8222 in the HTTP wait strategy with the existing port constant NatsHttpManagementPort: locate the call chain in NatsBuilder where WithWaitStrategy(Wait.ForUnixContainer().UntilHttpRequestIsSucceeded(request => request.ForPort(8222).ForPath("/healthz"))) is used and change request.ForPort(8222) to request.ForPort(NatsHttpManagementPort) so the wait strategy uses the shared constant.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/Testcontainers.Nats/NatsBuilder.cs`:
- Line 113: Replace the hard-coded port 8222 in the HTTP wait strategy with the
existing port constant NatsHttpManagementPort: locate the call chain in
NatsBuilder where
WithWaitStrategy(Wait.ForUnixContainer().UntilHttpRequestIsSucceeded(request =>
request.ForPort(8222).ForPath("/healthz"))) is used and change
request.ForPort(8222) to request.ForPort(NatsHttpManagementPort) so the wait
strategy uses the shared constant.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f397a78b-abd2-43fa-a5c0-12e30e910838
📒 Files selected for processing (1)
src/Testcontainers.Nats/NatsBuilder.cs
Cheers! And thanks for applying the code standard, sloppy of me. Seems we need a second reviewer now though. |
What does this PR do?
This PR changes the wait strategy to await a 200 OK for the /healthz endpoint, rather than simply checking the logs on the container.
Why is it important?
When using the Nats Testcontainer, the
StartAsync()returns before the container is ready to accept connections, resulting inNATS.Client.Core.NatsException: can not connect uris: nats://127.0.0.1:<<port>>. This is a problem on OSX with Rancher Desktop, not sure if it's a problem with Docker Desktop. Talking to NATS Server (see issue below) they recommend waiting differently.Related issues
Summary by CodeRabbit