Skip to content
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.

Sign up for GitHub

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 default network is not initialize when get gateway ip #349

Merged
merged 4 commits into from
Aug 24, 2021
Merged

Fix default network is not initialize when get gateway ip #349

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
602d86c
Fix default network is not initialize when get gateway ip
Aug 19, 2021
5dfa612
fix issue
Aug 19, 2021
0963bdd
Add UT
Aug 20, 2021
46cf72a
verify the ip address is not empty
Aug 20, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions docker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -892,6 +892,13 @@ func (p *DockerProvider) GetNetwork(ctx context.Context, req NetworkRequest) (ty

func (p *DockerProvider) GetGatewayIP(ctx context.Context) (string, error) {
// Use a default network as defined in the DockerProvider
if p.defaultNetwork == "" {
var err error
p.defaultNetwork, err = getDefaultNetwork(ctx, p.client)
if err != nil {
return "", err
}
}
nw, err := p.GetNetwork(ctx, NetworkRequest{Name: p.defaultNetwork})
if err != nil {
return "", err
Expand Down
12 changes: 12 additions & 0 deletions docker_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1506,6 +1506,18 @@ func TestContainerWithReaperNetwork(t *testing.T) {
assert.NotNil(t, cnt.NetworkSettings.Networks[networks[1]])
}

func TestGetGatewayIP(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have added the not empty check :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

offtopic: it seems the @dalekliuhan commiter user does not have associated an email on github

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the CI passes, this LGTM :)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

offtopic: it seems the @dalekliuhan commiter user does not have associated an email on github

email address on GitHub? I have set it. Do I miss anything? :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, sorry about that, I submit by a different user, not the GitHub user so it cannot be clickable.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mdelapenya Does this repo support squash merge? If so, GitHub will take care of this issue automatically.

// When using docker-compose with DinD mode, and using host port or http wait strategy
// It's need to invoke GetGatewayIP for get the host
provider, err := NewDockerProvider()
if err != nil {
t.Fatal(err)
}
if _, err := provider.GetGatewayIP(context.Background()); err != nil {
t.Fatal(err)
}
}

func randomString() string {
rand.Seed(time.Now().UnixNano())
chars := []rune("ABCDEFGHIJKLMNOPQRSTUVWXYZ" +
Expand Down