FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fix getDockerHostIpAddress within docker container by qoomon · Pull Request #648 · testcontainers/testcontainers-java · GitHub

/ testcontainers-java Public

fix getDockerHostIpAddress within docker container - #648

Merged
bsideup merged 1 commit into
testcontainers:masterfrom
qoomon:patch-1
Apr 19, 2018
ConversationCommits (1)ChecksFiles changed
Merged

fix getDockerHostIpAddress within docker container#648
bsideup merged 1 commit into
testcontainers:masterfrom
qoomon:patch-1

Conversation

qoomon commented Apr 18, 2018

Copy link
Copy Markdown
Contributor

only determine default gateway within docker container if docker host differ from default, otherwise it was set manually and it should be used as it was set

bsideup requested changes Apr 19, 2018
View reviewed changes
Comment thread core/src/main/java/org/testcontainers/dockerclient/DockerClientConfigUtils.java
// See https://github.com/docker/docker/blob/a9fa38b1edf30b23cae3eade0be48b3d4b1de14b/daemon/initlayer/setup_unix.go#L25
public static final boolean IN_A_CONTAINER = new File("/.dockerenv").exists();

@Getter(lazy = true)

bsideup Apr 19, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Any reason why you dropped lazily cached result? This change will make it run every time somebody queries the host ip, and I don't see how does it fix it.
Also, was it broken?

qoomon Apr 19, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

my bad you are right i will refactor it to lazy cache again.
and yes it is broken if we run it in GitLab CI directly in the build container with the DinD service it resolves to the default gateway, however it should resolve to tcp://docker:2375 or just docker

bsideup Apr 19, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

oh wow, nice finding!

bsideup reviewed Apr 19, 2018
View reviewed changes
Comment thread core/src/main/java/org/testcontainers/dockerclient/DockerClientConfigUtils.java
))
.map(StringUtils::trimToEmpty)
.filter(StringUtils::isNotBlank);
private static final Optional<String> defaultGateway = Optional

bsideup Apr 19, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I know that the indentation was incorrect, but could you please revert the change to the actual change? We will fix the indentation afterwards, but it will help to understand what was modified :)

bsideup Apr 19, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think the whole change is as simple as getDefaultGateway().orElse("localhost") in getDockerHostIpAddress

qoomon Apr 19, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

okay will fixed

bsideup added this to the next milestone Apr 19, 2018
bsideup reviewed Apr 19, 2018
View reviewed changes
Comment thread core/src/main/java/org/testcontainers/dockerclient/DockerClientConfigUtils.java
public static final boolean IN_A_CONTAINER = new File("/.dockerenv").exists();

@Getter(lazy = true)
private static final Optional<String> detectedDockerHostIp = Optional

bsideup Apr 19, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Since this getter is public, I don't think we can just change the behaviour of it :( Could you please make this change backward compatible?

qoomon Apr 19, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

ah I see

qoomon commented Apr 19, 2018

Copy link
Copy Markdown
Contributor Author

so finally it is backward compatible so getDetectedDockerHostIp() acts as before
and I marked getDetectedDockerHostIp() with @deprecated

bsideup reviewed Apr 19, 2018
View reviewed changes
Comment thread core/src/main/java/org/testcontainers/dockerclient/DockerClientConfigUtils.java Outdated

@Deprecated
@Getter(lazy = true)
private static final Optional<String> detectedDockerHostIp = IN_A_CONTAINER ? getDefaultGateway() : Optional.empty();

bsideup Apr 19, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

could you please put this method upper where it was, so that the diff will be smaller?

Comment thread core/src/main/java/org/testcontainers/dockerclient/DockerClientConfigUtils.java
}
}
))
.map(StringUtils::trimToEmpty)

bsideup Apr 19, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

TBH I would prefer to keep this chain

qoomon commented Apr 19, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

now the diff look quite neat

bsideup merged commit 58a24f9 into testcontainers:master Apr 19, 2018

bsideup commented Apr 19, 2018

Copy link
Copy Markdown
Member

@qoomon merged, thanks!

bsideup commented Apr 19, 2018

Copy link
Copy Markdown
Member

@qoomon oh snap, I forgot the changelog :D Will add it myself, thanks for your contribution 👍

bsideup added a commit that referenced this pull request Apr 19, 2018
add #648 to CHANGELOG.md
e2b408f
rnorth pushed a commit that referenced this pull request Apr 24, 2018
only determine default gateway within docker container if docker host…
f54dc86 
… differ from default (#648)
rnorth pushed a commit that referenced this pull request Apr 24, 2018
add #648 to CHANGELOG.md
d1b131a
rnorth pushed a commit that referenced this pull request Apr 24, 2018
add #648 to CHANGELOG.md
eb8fd8d
bsideup mentioned this pull request Apr 24, 2018
Closed
miketwo mentioned this pull request Apr 29, 2018
Closed
rnorth pushed a commit that referenced this pull request Apr 30, 2018
add #648 to CHANGELOG.md
9844ea6
rnorth pushed a commit that referenced this pull request Apr 30, 2018
add #648 to CHANGELOG.md
af5210d
rnorth pushed a commit that referenced this pull request Apr 30, 2018
add #648 to CHANGELOG.md
9734f5f
rnorth pushed a commit that referenced this pull request May 13, 2018
add #648 to CHANGELOG.md
fee8ed2
rnorth pushed a commit that referenced this pull request May 15, 2018
add #648 to CHANGELOG.md
e6b95c4
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

bsideup bsideup approved these changes
kiview Awaiting requested review from kiview
rnorth Awaiting requested review from rnorth

Assignees

No one assigned

Labels

client/in-container type/bug

Projects

None yet

Milestone

1.7.1

Development

Successfully merging this pull request may close these issues.

2 participants

Back | FazBrowse Home | New Git URL