-
-
Notifications
You must be signed in to change notification settings - Fork 503
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
feat: add yugabytedb module #2825
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. |
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.
Thanks for the PR, here's some initial feedback
Co-authored-by: Steven Hartland <stevenmhartland@gmail.com>
Co-authored-by: Steven Hartland <stevenmhartland@gmail.com>
Co-authored-by: Steven Hartland <stevenmhartland@gmail.com>
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 <stevenmhartland@gmail.com>
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 @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 <social.mdelapenya@gmail.com>
Co-authored-by: Manuel de la Peña <social.mdelapenya@gmail.com>
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.
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! |
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 for your patience during the review 🙇 and for improving the library with this new module 🚀
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!
* 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)
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