-
-
Notifications
You must be signed in to change notification settings - Fork 449
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
Add host for sql waiter URL function. #166
Conversation
Add host for sql waiter
Thanks @a-urth This is a BC break but I actually like it. @mdelapenya what do yo think? |
Hey @gianarb is there any progress for this PR? Is it going to be merged? |
Update from source
this is way easier to merge if we can figure out a way to avoid a bc-break. I think we can using the a with function. Like a |
@@ -1,2 +1,3 @@ | |||
.idea |
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.
Can you revert this? it should be placed in your global gitignore
The wait.ForSQL wait strategy does not have a testcase and I forgot about how to use it as well. I decided to write it in preparation for the work/discussion ongoing here: #166
The wait.ForSQL wait strategy does not have a testcase and I forgot about how to use it as well. I decided to write it in preparation for the work/discussion ongoing here: #166
The wait.ForSQL wait strategy does not have a testcase and I forgot about how to use it as well. I decided to write it in preparation for the work/discussion ongoing here: #166
Yes, I like the idea of not forcing a breaking change too 👍 |
I'd waitForMerge("#214") wdyt? |
Hey guys, sorry to bring this back from the dead, but I was running into a similar situation. I'd love to get this merged so I can avoid having to revert to what testcontainers-java is doing (waiting for log messages). If we can change this PR to preserve backwards compatibility, can we merge? I'm willing to invest some time into this unless @a-urth wants to apply the suggested changes? |
I just took a little stab at a backward-compatible way in #310. The reason I didn't just create a |
I'm closing this PR, as it's already been implemented in #524 Thanks! |
Issue:
In case container is started in docker which is not in localhost (ci environment with tests already running in container), mapped port is not enough to establish connection to a database.
Solution:
Add host parameter for url function, which is part of wait.ForSQL strategy, so it will be possible to construct connection url with docker host and mapped port.