-
Notifications
You must be signed in to change notification settings - Fork 31
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
Support docker compose #19
Conversation
mrproliu
commented
Jun 13, 2021
•
edited
Loading
edited
- Support using docker-compose to setup and cleanup
- Using Environment to expose the port for query or trigger
- Add example config
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.
Thank you @mrproliu very much for jumping in 🙇, very appreciate, some comments of mine are inline, please take a look
waitTimeout = NewTimeout(timeNow, waitTimeout) | ||
timeNow = time.Now() | ||
|
||
// wait port and export |
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.
We don't need to wait and export, just exporting it is OK, the healthiness check should be done by docker-compose.yml
, because the healthiness check should only make sense by the one who write docker-compose.yml
, we can cover every possibility of how to check healthiness.
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.
Sure, but the healthcheck
is only working on the service that has depends_on
. If we need to use webapp
service on trigger
or query
, it hasn't service dependent on this, so we may not access it. Do I miss anything?
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.
OK, it makes sense to me, can you reuse compose.WithExposedService(service, port, wait.ForListeningPort("<port>/tcp"))
to replace waitTCPPortStarted
so that we don't replicate the codes? Others generally look good to me, thanks again ❤️
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 have tried compose.WithExposedService(service, port, wait.ForListeningPort("<port>/tcp"))
method, but currently the second parameter only supports the specified port, so we cannot use it. This error has already been reported and a new PR has been created. I will pay attention to the next release version of the testcontainers-go
project. If this bug is fixed, I will use 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.
Thanks for the information, can you paste the related issue / PR here?
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.
Sorry I forget. here: testcontainers/testcontainers-go#330
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.
A new release has been pushed, fixing this. Hope it helps!
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.
A new release has been pushed, fixing this. Hope it helps!
Wow, thx a lot. I will continue to work on this.
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