-
-
Notifications
You must be signed in to change notification settings - Fork 466
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
fix(cockroachdb): Fixes cockroachdb wait strategy handling #2456
fix(cockroachdb): Fixes cockroachdb wait strategy handling #2456
Conversation
✅ Deploy Preview for testcontainers-go ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
LGTM thank you for taking the time to fix this!
@bearrito I added a few minor comments regarding consistency around blank lines as separation for different code blocks, so once they are addressed, this is ready to be merged.
🚀
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.
LGTM, there are a few non-blocking cosmetic comments here, but I think the PR can be merged if everybody agree.
I'll wait for you @bearrito and @AhmedNSidd for the open discussions, so at the moment they are closed, I'll merge this one.
Thank you both for your patience and for contributing to testcontainers-go!
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.
Changes look good to me. Please feel free to merge this one in @mdelapenya
My pleasure re: contributing to testcontainers-go
:)
Thank you for maintaining this library.
modules/cockroachdb/cockroachdb.go
Outdated
if req.WaitingFor == nil { | ||
req.WaitingFor = defaultStrategy | ||
} else { | ||
req.WaitingFor = wait.ForAll(req.WaitingFor, sqlWait) |
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.
Do we want to exclude the wait.ForHTTP here? IIUC, if there is a waitingFor, we are adding the SQL wait plus the defined one. If there is no waitingFor, we are adding the default wait (http + sql). Is it intended to remove the http one in the case the user provides a waitingFor? I think we should append the user-defined one to the default. Wdyt?
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.
Are you suggesting we do this ?
defaultStrategy := wait.ForSQL( ... elided ...)
if req.WaitingFor == nil {
req.WaitingFor = wait.ForAll(wait.ForHTTP("/health").WithPort(defaultAdminPort),defaultStrategy)
} else {
req.WaitingFor = wait.ForAll(req.WaitingFor, defaultStrategy)
}
I think you end up with the same thing.
Case 1: No Waiting for defined -> You get HTTP and SQL Waits
Case 2: A Wait is defined -> You get whatever the user passes in and SQL.
If that approach reads better I'm good with it.
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.
Also, I can look in the source, but I might as well ask is wait.ForAll
linear-ish eg if is FIFO or LIFO or in parallel ?
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.
They are not run in parallel, so FIFO
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.
Case 2: A Wait is defined -> You get whatever the user passes in and SQL.
That's exactly my point: we lose here the original waitFor.HTTP, right? Is this intended?
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.
That's what I originally had, but removed it. I have no problem switching it back.
Two arguments for/against:
- If as a user I pass in a wait strategy I would expect that wait strategy to be exclusively used. However, the SQL is different as it has to be awaited for connection functionality.
- Alternatively, we should always wait with predetermined strategies until we have a usable container to provide the user. But then why allow the user to override at all?
It almost seems like what would be better would something like
testcontainers.WithDeadline(time.Millisecond*20))
Where we could just add the deadline to the existing wait strategy (assuming it exists).
WDYT?
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.
Hey @bearrito sorry for the radio silence, May was a month in which I combined PTO and full-focus on an eventual v1 release of testcontainers-go. My apologies.
The wait.ForAll strategy comes with a deadline to be applied to all its children, so you could leverage that.
@mdelapenya Is this good to go, the last failing test were unrelated |
@mdelapenya Anything else on this? |
@mdelapenya I reverted to always honor the default strategy. I don't think can do much more with cockroach, namely because how it's doing it's own thing with the |
* main: (48 commits) Fix race condition when looking up reaper (ryuk) container (testcontainers#2508) chore: bring golangci-lint back (testcontainers#2571) docs(compose): Fix typo docker compose docs (testcontainers#2565) Handle error properly during port forwarding initialization. (testcontainers#2550) chore: pin vearch version (testcontainers#2568) feat: add vearch module (testcontainers#2560) chore: run tests against latest Docker engine, nightly (testcontainers#2566) chore(deps): bump mkdocs-include-markdown-plugin from 6.0.4 to 6.0.7 (testcontainers#2562) Fix network accessor for port-forwarding feature (testcontainers#2551) --- (testcontainers#2549) fix: update search bar eval in mkdocs (testcontainers#2547) docs: improve contributing docs for code snippets (testcontainers#2546) chore: use a virtualenv for working with the docs site (testcontainers#2545) docs: document test session semantics (testcontainers#2544) feat(ryuk): allow to configure ryuk timeouts using env variables (testcontainers#2541) docs: fix CircleCI docs (testcontainers#2539) fix: add import to module generation (testcontainers#2537) chore: prepare for next minor development cycle (0.32.0) chore: use new version (v0.31.0) in modules and examples feat(mongodb): add replica set support via opts (testcontainers#2469) ...
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.
LGTM, thanks!
* Fixes cockroachdb wait strategy handling * Clarify test intent * Merge in default strategy to get connection * Use subtests * Separate out Connection logic * Feedback from PR * Honor default settings * fix: make lint --------- Co-authored-by: bstrausser <bstrausser@locusrobotics.com> Co-authored-by: Manuel de la Peña <mdelapenya@gmail.com>
* main: docs: document disabling ryuk with properties (testcontainers#2603) fix: allow compose files and readers to be used together (testcontainers#2598) chore(deps): bump urllib3 from 2.2.1 to 2.2.2 (testcontainers#2590) docs: example for NATS cluster (testcontainers#2591) docs: fix deprecation for material extensions emoji (testcontainers#2592) gha: update golangci-lint to v1.59.1 (testcontainers#2588) network: WithCheckDuplicate: don't set CheckDuplicate value (testcontainers#2589) fix: proper synchronisation for start/stop log production (testcontainers#2576) chore(deps): bump github.com/docker/docker from v25.0.5 to v26.1.4 (testcontainers#2584) docs: update contributing guide (testcontainers#2586) feat: add testcontainers labels to the built images (testcontainers#2579) feat(mongodb): Make the replica set name configurable (testcontainers#2538) chore: skip reaper tests if ryuk is disabled (testcontainers#2582) chore: increase check time to 60 secs instead of 20 (testcontainers#2581) fix: prepend substitutors for built images (testcontainers#2577) docs: update colima docs (testcontainers#2578) fix(cockroachdb): Fixes cockroachdb wait strategy handling (testcontainers#2456) Fix race condition when looking up reaper (ryuk) container (testcontainers#2508) chore: bring golangci-lint back (testcontainers#2571) docs(compose): Fix typo docker compose docs (testcontainers#2565)
What does this PR do?
Fixes wait strategy handling. Previously cockroachdb was overwriting wait strategies.
Why is it important?
Custom wait strategies were not being respected
Related issues
How to test this PR
I added tests.
There looks to be some form of connection logic in the
SQL
strategy. This has to be merged into the user provided strategy otherwise you will timeout.