Conversation
Removes custom Netty factory and tcp-to-unixsocket proxy.
| @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.
Maybe save result of || in local variable unixLike?
There was a problem hiding this comment.
isn't it self-descriptive already?
There was a problem hiding this comment.
Fine either way, I think it's okay for 3 arguments.
There was a problem hiding this comment.
actually, just IS_OS_UNIX is enough. Changed. Also, there was a bug with || :D
# Conflicts: # CHANGELOG.md
rnorth
left a comment
rnorth
left a comment
There was a problem hiding this comment.
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!
| <dependency> | ||
| <groupId>org.rnorth</groupId> | ||
| <artifactId>tcp-unix-socket-proxy</artifactId> | ||
| <version>1.0.1</version> |
There was a problem hiding this comment.
It served a great job! 🥇 :)
| @Slf4j | ||
| public class DockerMachineClientProviderStrategy extends DockerClientProviderStrategy { | ||
|
|
||
| public static final int PRIORITY = EnvironmentAndSystemPropertyClientProviderStrategy.PRIORITY - 10; |
There was a problem hiding this comment.
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.
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.
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.
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
| 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.
@rnorth seems like this test actually helps :D We almost shipped unshaded native dependency :)
|
3 participants
Removes custom Netty factory and tcp-to-unixsocket proxy.