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.
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.
🚀
| contextDeadlineExceeded := fmt.Errorf("failed to start container: context deadline exceeded") | ||
| nodeStartUpCompleted := "node startup completed" | ||
| suite.Run("Expected Failure To Run", func() { | ||
|
|
| suite.Require().NoError(err) | ||
| } | ||
| }) | ||
|
|
| suite.Require().NoError(err) | ||
| } | ||
| }) | ||
|
|
There was a problem hiding this comment.
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.
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.
| if req.WaitingFor == nil { | ||
| req.WaitingFor = defaultStrategy | ||
| } else { | ||
| req.WaitingFor = wait.ForAll(req.WaitingFor, sqlWait) |
There was a problem hiding this comment.
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.
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.
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.
They are not run in parallel, so FIFO
There was a problem hiding this comment.
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.
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?
|
@mdelapenya Is this good to go, the last failing test were unrelated |
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
SQLstrategy. This has to be merged into the user provided strategy otherwise you will timeout.