Change docker default port to support windows and boot2docker #225
Conversation
Uses the environment definition for docker by default. Docker will default to the unix/tcp socket if the environment is not set.
| // | ||
| // Also note that we need to turn on DevMode in the test configs. | ||
| if d.config.DevMode { | ||
| return docker.NewClientFromEnv() |
There was a problem hiding this comment.
I feel like if this returns an error we should try and use the default Docker host.
There was a problem hiding this comment.
I don't think it makes sense to do this. If you are on e.g. windows and have misconfigured your DOCKER_HOST setting then this should reasonably be expected to fail. I want to know if my config is wrong, rather than having it be magically replaced by something else.
We will use the defaults if you have not specified anything.
There was a problem hiding this comment.
Ah didn't see the part were it falls back. That is really what I wanted. That happens in NewClientFromEnv?
There was a problem hiding this comment.
Yeah you have to dig into the calls but it eventually calls getDefaultDockerHost() in go-dockerclient which does basically the same thing you see in getDefaultDockerHost in docker.go. It's private so I had to copy-pasta. I should probably change it to public and upstream that one.
There was a problem hiding this comment.
It's private so I had to copy-pasta. I should probably change it to public and upstream that one.
That PR is here: fsouza/go-dockerclient#394
…in docs for docker.cleanup
| @@ -33,15 +35,54 @@ type dockerHandle struct { | |||
| doneCh chan struct{} | |||
| } | |||
|
|
|||
| // getDefaultDockerHost is copied from fsouza. If it were exported we woudn't | |||
| // need this here. | |||
| func getDefaultDockerHost() (string, error) { | |||
There was a problem hiding this comment.
It looks like your PR has been merged so we should remove this
There was a problem hiding this comment.
Yep! Testing that change locally.
|
Tests are passing locally (OS X and linux): |
|
@cbednarski I think @dadgar was going to take a look at #236 to see what those three tests are failing on Travis |
|
LGTM |
Change docker default port to support windows and boot2docker
Clean up temporary snapshots when there are errors
|
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
DOCKER_HOST,DOCKER_TLS_VERIFY, andDOCKER_CERT_PATHto facilitate development and testing on Windows and OSX using boot2docker or a VMdocker.endpointconfig option, and falls back on a platform-specific default.If merged, this also closes #141