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.

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

Update docker-java to 3.0.12 #393

Merged
merged 5 commits into from
Jul 7, 2017
Merged

Update docker-java to 3.0.12 #393

merged 5 commits into from
Jul 7, 2017

Conversation

bsideup
Copy link
Member

@bsideup bsideup commented Jul 6, 2017

Removes custom Netty factory and tcp-to-unixsocket proxy.

Removes custom Netty factory and tcp-to-unixsocket proxy.
@bsideup bsideup added this to the 1.4.0 milestone Jul 6, 2017
@bsideup bsideup requested review from rnorth and kiview July 6, 2017 09:53
@@ -21,7 +25,12 @@

@Override
protected boolean isApplicable() {
return SystemUtils.IS_OS_LINUX;
return SystemUtils.IS_OS_LINUX || SystemUtils.IS_OS_MAC && new File(DOCKER_SOCK_PATH).exists();
Copy link
Member

Choose a reason for hiding this comment

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

Maybe save result of || in local variable unixLike?

Copy link
Member Author

Choose a reason for hiding this comment

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

isn't it self-descriptive already?

Copy link
Member

Choose a reason for hiding this comment

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

Fine either way, I think it's okay for 3 arguments.

Copy link
Member Author

Choose a reason for hiding this comment

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

actually, just IS_OS_UNIX is enough. Changed. Also, there was a bug with || :D

Copy link
Member

Choose a reason for hiding this comment

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

Great change 💃

Copy link
Member

@rnorth rnorth left a comment

Choose a reason for hiding this comment

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

Seems fine to me. For what it's worth, I've tested this with Docker for Mac 17.06.0-ce-mac18 and it's working well.
Thanks!

<groupId>org.rnorth</groupId>
<artifactId>tcp-unix-socket-proxy</artifactId>
<version>1.0.1</version>
</dependency>
Copy link
Member

Choose a reason for hiding this comment

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

At last this can die 😆

Copy link
Member Author

Choose a reason for hiding this comment

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

It served a great job! 🥇 :)

@@ -16,6 +16,9 @@
*/
@Slf4j
public class DockerMachineClientProviderStrategy extends DockerClientProviderStrategy {

public static final int PRIORITY = EnvironmentAndSystemPropertyClientProviderStrategy.PRIORITY - 10;
Copy link
Member

Choose a reason for hiding this comment

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

I still find these priorities a little hard to grok - you kind of need to jump around from class to class to figure out what the sorted order will be.
Not now, but please could we reconsider moving back to having this ordering defined in one place?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure! Since we remember last used strategy, I think we can remove them now. Will do as a follow up

Copy link
Member

Choose a reason for hiding this comment

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

Ooh, but can we actually do that? For example, I would like Docker-for-Mac/Win to always come before Docker Machine. That way users at least have a chance to control which gets used from the outside (they can quit DfM and docker-machine gets used as a fallback). Similarly for setting env vars to force use of a particular docker daemon.

This might need some care 🤔...

Copy link
Member Author

Choose a reason for hiding this comment

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

I actually had to remove ~/.testcontainers.properties a couple of times because it saved Docker Machine strategy when Docker for Mac was turned off :)

Maybe we can mark Docker Machine strategy as non-persistable, so that it will be used only as a fallback?
IMO it makes sense to do that in 2017 :D

@@ -61,7 +61,8 @@ public void testMetaInf() throws Exception {
);

assertThatFileList(root.resolve("META-INF").resolve("native")).containsOnly(
"liborg-testcontainers-shaded-netty-transport-native-epoll.so"
"liborg-testcontainers-shaded-netty-transport-native-epoll.so",
"liborg-testcontainers-shaded-netty-transport-native-kqueue.jnilib"
Copy link
Member Author

Choose a reason for hiding this comment

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

@rnorth seems like this test actually helps :D We almost shipped unshaded native dependency :)

@bsideup bsideup merged commit 3cce55a into master Jul 7, 2017
@bsideup bsideup deleted the update_docker_java branch July 7, 2017 09:17
@rnorth
Copy link
Member

rnorth commented Jul 7, 2017

Maybe we can mark Docker Machine strategy as non-persistable, so that it will be used only as a fallback?
Fantastic idea! 👍

rnorth added a commit that referenced this pull request Jul 11, 2017
rnorth added a commit that referenced this pull request Jul 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants