Fix default network is not initialize when get gateway ip #349
Conversation
|
@mrproliu thanks for this contribution! I'd be great if you can share with us logs, screenshots, useful links where we can check the real problem, or even better to contribute a test reproducing the issue (although I know that replicating the DinD + GH action scenario could be difficult). wdyt? |
We are already trying to run in GitHub Action through DinD. Here is how we use GitHub Action to implement DinD by sharing socks. When we use docker-compose, we need to bind the ports to the waiting policy to ensure that these ports can be accessed. I hope this content is enough, If you think what else I am missing, please feel free to let me know :) |
|
Just say hi to @bsideup :P We are deeply integrating testcontainer for our e2e test infra project, https://github.com/apache/skywalking-infra-e2e We will incubate the project in the SkyWalking and ASF. Who ever uses the skywalking-infra-e2e, they have 50% chance(or more) are going to use test container too. |
Codecov Report
@@ Coverage Diff @@
## master #349 +/- ##
==========================================
+ Coverage 61.32% 61.92% +0.59%
==========================================
Files 15 15
Lines 993 998 +5
==========================================
+ Hits 609 618 +9
+ Misses 292 285 -7
- Partials 92 95 +3
Continue to review full report at Codecov.
|
| @@ -1506,6 +1506,18 @@ func TestContainerWithReaperNetwork(t *testing.T) { | |||
| assert.NotNil(t, cnt.NetworkSettings.Networks[networks[1]]) | |||
| } | |||
|
|
|||
| func TestGetGatewayIP(t *testing.T) { | |||
There was a problem hiding this comment.
Thanks for contributing a unit test! This is super!
I'd like to ask about the assertions part: according to the PR description:
the IP address in the Host Port wait strategy is incorrect
Is it enough to verify that no errors are raised, or do we need to verify any other thing?
BTW It's not a blocker on the review, I just want to make sure we are covering the new behaviour with the tests.
There was a problem hiding this comment.
Is it enough to verify that no errors are raised, or do we need to verify any other thing?
We are not sure about the specific value of the IP address in the GHA test or host test, so we can only verify that the IP address is not empty and there is no error. Do you think we also need to create a container with an export port to verify that this address is available?
There was a problem hiding this comment.
Yeah, if reproducing GHA behaviour is complex I'd keep it like it is now adding the verification of the IP address is not empty. If you can give it a try with a real container, best. But as I said, let's not block this unless needed.
There was a problem hiding this comment.
Have added the not empty check :)
There was a problem hiding this comment.
offtopic: it seems the @dalekliuhan commiter user does not have associated an email on github
There was a problem hiding this comment.
If the CI passes, this LGTM :)
There was a problem hiding this comment.
offtopic: it seems the
@dalekliuhancommiter user does not have associated an email on github
email address on GitHub? I have set it. Do I miss anything? :)
There was a problem hiding this comment.
Look at the commit in the list: it does not have an image associated to it. And when you click on the commit itself, the user is not clickable
There was a problem hiding this comment.
Oh, sorry about that, I submit by a different user, not the GitHub user so it cannot be clickable.
There was a problem hiding this comment.
@mdelapenya Does this repo support squash merge? If so, GitHub will take care of this issue automatically.
|
Thanks a lot! |
When I try to use docker in docker through the share UNIX socket on GitHub Action, the IP address in the Host Port wait strategy is incorrect.
I found when we trying to get the target host, the default network may not initialize:
testcontainers-go/docker.go
Line 895 in 5963181