feat: add yugabytedb module#2825
Conversation
✅ Deploy Preview for testcontainers-go ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
stevenh
left a comment
stevenh
left a comment
There was a problem hiding this comment.
Thanks for the PR, here's some initial feedback
Co-authored-by: Steven Hartland <[email protected]>
Co-authored-by: Steven Hartland <[email protected]>
Co-authored-by: Steven Hartland <[email protected]>
henripqt
force-pushed
the
feat/add-yugabytedb-module
branch
from
f4f49e9 to
cad9e6a
Compare
modules/yugabytedb/examples_test.go
Outdated
| var ( | ||
| i int | ||
| row = db.QueryRowContext(ctx, "SELECT 1") | ||
| ) | ||
|
|
||
| if err := row.Scan(&i); err != nil { |
There was a problem hiding this comment.
suggestion: a more idiomatic style, which is less verbose for this is, you could also fold into a single line if you prefer.
| var ( | |
| i int | |
| row = db.QueryRowContext(ctx, "SELECT 1") | |
| ) | |
| if err := row.Scan(&i); err != nil { | |
| var i int | |
| row := db.QueryRowContext(ctx, "SELECT 1") | |
| if err := row.Scan(&i); err != nil { |
Co-authored-by: Steven Hartland <[email protected]>
There was a problem hiding this comment.
Hey @henripqt, thanks for this new module! I added a few comments after testing this PR locally. It seems there is a race condition that causes the tests to fail if I run them, but they always pass if I debug them step-by-step. So my first impression is that we need a more consistent wait strategy. I have no experience with yugabyte, so I'd appreciate if you could do the research on how to fix that. Can you double check how the Java implementation is building them (here and here)?
Thanks!
Co-authored-by: Manuel de la Peña <[email protected]>
Co-authored-by: Manuel de la Peña <[email protected]>
mdelapenya
left a comment
mdelapenya
left a comment
There was a problem hiding this comment.
I left just one final comment regarding the usage of the yugabyte client package. Other than that, and after a thorough review from @stevenh (🙇), this PR LGTM, and it's ready to be merged after that discussion is resolved.
Thank you for your time, and for this new module 🙏
"refers to unknown field or method: Container.YCQLConfigureClusterConfig"
|
@henripqt I added two commits to avoid you the burden of doing it: they just run I think once the CI passes, we are good to go. Thank you! |
mdelapenya
left a comment
mdelapenya
left a comment
There was a problem hiding this comment.
LGTM, thanks for your patience during the review 🙇 and for improving the library with this new module 🚀
* main: feat: add yugabytedb module (testcontainers#2825) fix: update module container struct name and missing imports (testcontainers#2831)
* main: fix(reaper): refactor to allow retries and fix races (testcontainers#2728) chore: update ryuk to 0.10.2 (testcontainers#2833) feat: add yugabytedb module (testcontainers#2825) fix: update module container struct name and missing imports (testcontainers#2831) chore: replace 'assert' with 'require' (testcontainers#2827) chore: replace 'assert' with 'require' for critical checks (testcontainers#2824) chore: bump ryuk to latest release (testcontainers#2818) feat: add require for critical checks (testcontainers#2812)
3 participants
What does this PR do?
This PR introduces the yugabytedb go module
Why is it important?
There is currently no official yugabytedb test container module for golang
Related issues
No related issue
How to test this PR