-
-
Notifications
You must be signed in to change notification settings - Fork 548
fix: workaround for moby/moby#50133 when reusing container #3197
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
fix: workaround for moby/moby#50133 when reusing container #3197
Conversation
✅ Deploy Preview for testcontainers-go ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
471d296
to
fc48a6e
Compare
Hi @mdelapenya, Could you please trigger CI build for this PR sooner to check if this PR helps CI? 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! Just added a comment to improve the comment for the workaround, but approving it anyway to make sure we are going to merge this soon.
Thanks!
docker.go
Outdated
@@ -1364,11 +1364,17 @@ func (p *DockerProvider) ReuseOrCreateContainer(ctx context.Context, req Contain | |||
lifecycleHooks: []ContainerLifecycleHooks{combineContainerHooks(defaultHooks, req.LifecycleHooks)}, | |||
} | |||
|
|||
// Workaround for https://github.com/moby/moby/issues/50133 |
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: could you explain in the comment the need for the refresh of the state? I think that, even when the moby bug is related, the execution of our code could take millis that could impact it, even with the bug eventually resolved, so we'd like to have it explained there for future reference. I could imagine that the moby bug is resolved and we go back to this code and remove it just because of the comment. Please correct me if I'm wrong 🙏
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.
explain in the comment the need for the refresh of the state
Added details about workaround. Please review this PR one more time.
the moby bug is resolved and we go back to this code and remove it
I doubt it will be easy, because it will add requirement on Docker Engine version for Testcontainers for Go. Such requirement can be hard to meet (Docker Engine is pretty heavy tool for installation and not every user of Testcontainers for Go will be able to get required version of Docker Engine, especially if it is part of CI).
fc48a6e
to
76476ef
Compare
…tainers.GenericContainer. Signed-off-by: Marat Abrarov <[email protected]>
76476ef
to
bf5b134
Compare
* main: fix: workaround for moby/moby#50133 when reusing container in testcontainers.GenericContainer. (testcontainers#3197) feat(kafka,redpanda): support for waiting for mapped ports without external checks (testcontainers#3165)
Changes
fix: workaround for moby/moby#50133 when reusing container in
testcontainers.GenericContainer
.What does this PR do?
This PR implements workaround for moby/moby#50133 which fixes #3177.
Why is it important?
These changes should make PR builds more stable in this repository.
Related issues
How to test this PR
go test -count 1 -v -run TestGenericContainer_stop_start_withReuse
Follow-ups
Possible fix for moby/moby#50133 can be found in mabrarov/moby@master...mabrarov:moby:50133-container_stop_state_sync.