-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Conversation
Removes custom Netty factory and tcp-to-unixsocket proxy.
@@ -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(); |
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.
Maybe save result of ||
in local variable unixLike
?
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.
isn't it self-descriptive already?
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.
Fine either way, I think it's okay for 3 arguments.
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.
actually, just IS_OS_UNIX
is enough. Changed. Also, there was a bug with ||
:D
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.
Great change 💃
# Conflicts: # CHANGELOG.md
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.
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> |
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.
At last this can die 😆
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.
It served a great job! 🥇 :)
@@ -16,6 +16,9 @@ | |||
*/ | |||
@Slf4j | |||
public class DockerMachineClientProviderStrategy extends DockerClientProviderStrategy { | |||
|
|||
public static final int PRIORITY = EnvironmentAndSystemPropertyClientProviderStrategy.PRIORITY - 10; |
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.
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?
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.
Sure! Since we remember last used strategy, I think we can remove them now. Will do as a follow up
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.
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 🤔...
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.
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" |
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.
@rnorth seems like this test actually helps :D We almost shipped unshaded native dependency :)
|
Removes custom Netty factory and tcp-to-unixsocket proxy.