feat(wait): add human-readable String() methods to all wait strategies #3461
+159
−2
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
✅ Deploy Preview for testcontainers-go ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Summary by CodeRabbit
WalkthroughAdds String() methods to many wait strategies to produce human-readable descriptions, updates lifecycle readiness logging to use those descriptions, and changes WaitUntilReady invocation to pass the DockerContainer receiver. (≤50 words) Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Hook as readinessHook
participant Logger as logger
participant Strat as WaitStrategy
participant DC as DockerContainer
Hook->>Strat: if strategy != nil
note right of Strat #EEF6FF: use fmt.Stringer.String() if implemented
Strat-->>Hook: strategyDesc
Hook->>Logger: log "Waiting for container to be ready" (containerID, image, strategyDesc)
Hook->>DC: DC.WaitUntilReady(ctx, strategy)
DC->>Strat: WaitUntilReady(ctx, container)
Strat-->>DC: result / error
DC-->>Hook: done / error
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (20)
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 |
mdelapenya
commented
Oct 31, 2025
| @echo "Running $* tests..." | ||
| gotestsum \ | ||
| --format short-verbose \ | ||
| --rerun-fails=5 \ |
Member
Author
There was a problem hiding this comment.
Added by mistake, reverting. I'll submit a separated PR for this.
mdelapenya
commented
Oct 31, 2025
There was a problem hiding this comment.
Pull Request Overview
This PR implements the fmt.Stringer interface for all wait strategies by adding String() methods that return human-readable descriptions of each strategy's configuration. This enhances debugging and logging capabilities by providing clear, descriptive output for each wait strategy.
Key changes:
- Added
String()methods to all strategy types (HTTP, SQL, TLS, HostPort, Log, File, Exec, Exit, Health, Nop, and MultiStrategy) - Imported necessary packages (
fmt,strings) where needed - Removed
--rerun-fails=5flag from test configuration
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| wait/all.go | Implements String() for MultiStrategy with recursive formatting of nested strategies |
| wait/exec.go | Implements String() for ExecStrategy showing the command being executed |
| wait/exit.go | Implements String() for ExitStrategy with static description |
| wait/file.go | Implements String() for FileStrategy showing file path and optional matcher |
| wait/health.go | Implements String() for HealthStrategy with static description |
| wait/host_port.go | Implements String() for HostPortStrategy with port and check type details |
| wait/http.go | Implements String() for HTTPStrategy showing protocol, method, port, and path |
| wait/log.go | Implements String() for LogStrategy showing log type and occurrence count |
| wait/nop.go | Implements String() for NopStrategy with static description |
| wait/sql.go | Implements String() for waitForSQL showing port, driver, and optional query |
| wait/tls.go | Implements String() for TLSStrategy showing certificate and key file paths |
| commons-test.mk | Removes --rerun-fails=5 flag from test runner configuration |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Member
Author
|
@copilot add a commit to this pull request to apply changes based on the comments in this thread |
Add String() method to all wait strategy types to provide human-readable
descriptions that are displayed in container lifecycle logs. This improves
the user experience by showing clear wait conditions instead of raw struct
output.
Changes:
- Add String() method to HTTPStrategy, HealthStrategy, LogStrategy,
HostPortStrategy, FileStrategy, ExecStrategy, ExitStrategy,
waitForSQL, MultiStrategy, NopStrategy, and TLSStrategy
- Update lifecycle logging to use strategy String() output
- Each strategy provides contextual information (ports, paths, conditions)
Example output changes:
Before: Waiting for: &{timeout:<nil> deadline:0x140001c7190 Strategies:[0x1400032e1d0]}
After: Waiting for container to be ready strategy="HTTP GET request on port 8080 path /health"
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
mdelapenya
force-pushed
the
wait-strategy-stringer
branch
from
9e7fc5c to
ca83ad1
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
commons-test.mk(0 hunks)wait/all.go(2 hunks)wait/exec.go(2 hunks)wait/exit.go(1 hunks)wait/file.go(1 hunks)wait/health.go(1 hunks)wait/host_port.go(1 hunks)wait/http.go(1 hunks)wait/log.go(1 hunks)wait/nop.go(1 hunks)wait/sql.go(1 hunks)wait/tls.go(2 hunks)
💤 Files with no reviewable changes (1)
- commons-test.mk
🚧 Files skipped from review as they are similar to previous changes (8)
- wait/http.go
- wait/all.go
- wait/exit.go
- wait/file.go
- wait/health.go
- wait/tls.go
- wait/host_port.go
- wait/sql.go
🧰 Additional context used
🧬 Code graph analysis (1)
wait/log.go (1)
logconsumer.go (1)
Log(14-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). (19)
- GitHub Check: lint (modules/weaviate) / lint: modules/weaviate
- GitHub Check: lint (modules/influxdb) / lint: modules/influxdb
- GitHub Check: lint (modules/milvus) / lint: modules/milvus
- GitHub Check: lint (modules/mongodb) / lint: modules/mongodb
- GitHub Check: lint (modules/dind) / lint: modules/dind
- GitHub Check: lint (modules/scylladb) / lint: modules/scylladb
- GitHub Check: lint (modules/yugabytedb) / lint: modules/yugabytedb
- GitHub Check: lint (modules/inbucket) / lint: modules/inbucket
- GitHub Check: lint (modules/databend) / lint: modules/databend
- GitHub Check: lint (modules/gcloud) / lint: modules/gcloud
- GitHub Check: lint (modules/k3s) / lint: modules/k3s
- GitHub Check: lint (modules/neo4j) / lint: modules/neo4j
- GitHub Check: lint (modules/mssql) / lint: modules/mssql
- GitHub Check: lint (modules/qdrant) / lint: modules/qdrant
- GitHub Check: lint (modules/pinecone) / lint: modules/pinecone
- GitHub Check: lint (modules/arangodb) / lint: modules/arangodb
- GitHub Check: lint (modules/couchbase) / lint: modules/couchbase
- GitHub Check: lint (modules/dockermodelrunner) / lint: modules/dockermodelrunner
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
wait/log.go (1)
126-138: LGTM!The String() implementation is clear and provides useful context. The conditional logic for "log message" vs "log pattern" and the optional occurrence count make the output informative without being verbose.
wait/nop.go (1)
36-38: LGTM!The generic description "custom wait condition" is appropriate for NopStrategy since the actual wait behavior is user-defined via the
waitUntilReadyfunction parameter.wait/exec.go (1)
5-5: LGTM!The
fmtimport is necessary for the new String() method implementation.
Modify ExecStrategy.String() to only show the command name and argument count instead of full command with all arguments. This prevents sensitive information (passwords, API tokens, credentials) from being exposed in container lifecycle logs. Changes: - Show only command name when no arguments present - Show command name with argument count when arguments present - Never expose actual argument values Example output: - Before: exec command ["mysql" "-u" "root" "-pSecretPass"] - After: exec command "mysql" with 3 arguments 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Update container readiness logging to use structured logging format and
leverage the new String() methods on wait strategies. This provides
human-readable strategy descriptions in logs instead of raw Go struct
output.
Changes:
- Extract strategy to variable for type assertion
- Check if strategy implements fmt.Stringer interface
- Use strategy.String() for description when available
- Fall back to "unknown strategy" if String() not implemented
- Convert to structured logging with key-value pairs
- Improves log readability and parseability
Example output:
Before: ⏳ Waiting for container id abc123 image: nginx. Waiting for: &{timeout:<nil> deadline:0x140001c7190 Strategies:[0x1400032e1d0]}
After: Waiting for container to be ready containerID=abc123 image=nginx strategy="HTTP GET request on port 8080 path /health"
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
Remove special case that would omit the "all of:" prefix when only one strategy remains after filtering nils. Always include the prefix to make it clear that a MultiStrategy (ForAll) is being used, improving clarity and consistency in logs. Changes: - Remove single-strategy special case from MultiStrategy.String() - Always return "all of: [...]" format regardless of strategy count - Add comment explaining the rationale Example output: Before: log message 'ready' (when only 1 strategy) After: all of: [log message 'ready'] This makes it explicit that ForAll() is being used, which helps with debugging and understanding wait behavior. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lifecycle.go(1 hunks)wait/all.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- wait/all.go
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: mdelapenya
Repo: testcontainers/testcontainers-go PR: 3319
File: modules/arangodb/arangodb.go:46-57
Timestamp: 2025-09-29T13:57:14.636Z
Learning: In testcontainers-go ArangoDB module, the wait strategy combines port listening check with HTTP readiness check using wait.ForAll - both strategies are required and complementary, not redundant.
📚 Learning: 2025-09-29T13:57:14.636Z
Learnt from: mdelapenya
Repo: testcontainers/testcontainers-go PR: 3319
File: modules/arangodb/arangodb.go:46-57
Timestamp: 2025-09-29T13:57:14.636Z
Learning: In testcontainers-go ArangoDB module, the wait strategy combines port listening check with HTTP readiness check using wait.ForAll - both strategies are required and complementary, not redundant.
Applied to files:
lifecycle.go
🧬 Code graph analysis (1)
lifecycle.go (1)
log/logger.go (1)
Printf(47-49)
⏰ 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). (20)
- GitHub Check: lint (modules/cassandra) / lint: modules/cassandra
- GitHub Check: lint (modules/couchbase) / lint: modules/couchbase
- GitHub Check: lint (modules/nebulagraph) / lint: modules/nebulagraph
- GitHub Check: lint (modules/dind) / lint: modules/dind
- GitHub Check: lint (modules/openfga) / lint: modules/openfga
- GitHub Check: lint (modules/pinecone) / lint: modules/pinecone
- GitHub Check: lint (modules/mssql) / lint: modules/mssql
- GitHub Check: lint (modules/socat) / lint: modules/socat
- GitHub Check: lint (modules/yugabytedb) / lint: modules/yugabytedb
- GitHub Check: lint (modules/mysql) / lint: modules/mysql
- GitHub Check: lint (modules/openldap) / lint: modules/openldap
- GitHub Check: lint (modules/dolt) / lint: modules/dolt
- GitHub Check: lint (modules/aerospike) / lint: modules/aerospike
- GitHub Check: lint (modules/mockserver) / lint: modules/mockserver
- GitHub Check: lint (modules/minio) / lint: modules/minio
- GitHub Check: lint (modules/compose) / lint: modules/compose
- GitHub Check: lint (modules/k3s) / lint: modules/k3s
- GitHub Check: lint (modules/artemis) / lint: modules/artemis
- GitHub Check: lint (modules/chroma) / lint: modules/chroma
- GitHub Check: Analyze (go)
Use proper singular/plural grammar when displaying argument count: - "exec command 'foo'" when no arguments - "exec command 'foo' with 1 argument" when one argument - "exec command 'foo' with N arguments" when multiple arguments Also adds comment explaining security consideration to avoid exposing sensitive data in command arguments. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Add standard Go documentation comments to all String() method implementations across wait strategies for better code documentation and godoc generation. Changes: - Add "String returns a human-readable description of the wait strategy" comment to all String() methods in: - ExecStrategy - HealthStrategy - LogStrategy - HostPortStrategy - FileStrategy - ExitStrategy - NopStrategy - waitForSQL - TLSStrategy - MultiStrategy 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
wait/tls.go (1)
103-120: LGTM with a minor formatting suggestion.The String() method correctly builds a description from the TLS configuration elements. File paths are appropriately included for debugging purposes.
Minor suggestion: Consider using consistent formatting for file paths. Currently line 108 uses
%vfor rootFiles while line 112 uses%qfor cert/key files. Using%qconsistently would provide uniform output, though the current approach is acceptable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
wait/all.go(2 hunks)wait/exec.go(2 hunks)wait/exit.go(1 hunks)wait/file.go(1 hunks)wait/health.go(1 hunks)wait/host_port.go(1 hunks)wait/log.go(1 hunks)wait/nop.go(1 hunks)wait/sql.go(1 hunks)wait/tls.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- wait/sql.go
- wait/host_port.go
- wait/exit.go
- wait/nop.go
- wait/file.go
- wait/log.go
- wait/all.go
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: mdelapenya
Repo: testcontainers/testcontainers-go PR: 3319
File: modules/arangodb/arangodb.go:46-57
Timestamp: 2025-09-29T13:57:14.636Z
Learning: In testcontainers-go ArangoDB module, the wait strategy combines port listening check with HTTP readiness check using wait.ForAll - both strategies are required and complementary, not redundant.
⏰ 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). (10)
- GitHub Check: test (1.24.x, modulegen) / test: modulegen/1.24.x
- GitHub Check: test (1.24.x, modulegen) / test: modulegen/1.24.x
- GitHub Check: test (1.24.x, modulegen) / test: modulegen/1.24.x
- GitHub Check: test (1.24.x, modulegen) / test: modulegen/1.24.x
- GitHub Check: test (1.24.x, modulegen) / test: modulegen/1.24.x
- GitHub Check: test (1.24.x, modulegen) / test: modulegen/1.24.x
- GitHub Check: test (1.24.x, modulegen) / test: modulegen/1.24.x
- GitHub Check: test (1.24.x, modulegen) / test: modulegen/1.24.x
- GitHub Check: test (1.24.x, modulegen) / test: modulegen/1.24.x
- GitHub Check: test (1.24.x, modulegen) / test: modulegen/1.24.x
🔇 Additional comments (2)
wait/health.go (1)
63-66: LGTM!The String() method provides a clear, concise description of the health wait strategy.
wait/exec.go (1)
80-94: LGTM - Good security consideration!The String() method correctly avoids exposing potentially sensitive argument values by only displaying the command name and argument count. This addresses the security concern from the previous review.
mdelapenya
added a commit
that referenced
this pull request
Nov 3, 2025
* main: chore(deps): bump github.com/docker/docker from 28.3.3+incompatible to 28.5.1+incompatible (#3464) feat(wait): add human-readable String() methods to all wait strategies (#3461) chore(deps): bump mkdocs-include-markdown-plugin from 7.1.6 to 7.2.0 (#3463) chore(deps): bump actions/setup-go from 5.4.0 to 6.0.0 (#3462)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
String()method to all wait strategy types for human-readable descriptionsChanges
This PR adds
String()methods to all wait strategy implementations:Example Output
Before:
Waiting for container to be ready containerID=0fc8e4de4995 image=nginx:alpine Waiting for: &{timeout: deadline:0x140001c7190 Strategies:[0x1400032e1d0]}
After:
Waiting for container to be ready containerID=0fc8e4de4995 image=nginx:alpine strategy='HTTP GET request on port 8080 path /health'
Test plan
🤖 Generated with Claude Code