Use default cert dir for oc cluster up engine API client if DOCKER_TLS_VERIFY is set #12035
Conversation
jimmidyson
force-pushed
the
docker-tls-default-dir
branch
from
be5b3a3 to
395ea26
Compare
|
Would love to get this merged if possible so I don't have to mess with environment variables or use a local build of openshift. |
|
@csrwng Any comments on this? |
|
@jimmidyson sorry for the delay. I didn't see this one. @jim-minter can you please review? |
| dockerTLSVerify := os.Getenv("DOCKER_TLS_VERIFY") | ||
| dockerCertPath := os.Getenv("DOCKER_CERT_PATH") | ||
| if len(dockerTLSVerify) > 0 && len(dockerCertPath) == 0 { | ||
| home := homedir.Get() |
There was a problem hiding this comment.
I think using
dockerCertPath = cliconfig.ConfigDir()
os.SetEnv("DOCKER_CERT_PATH", dockerCertPath)
would be preferable here as it should additionally handle the DOCKER_CONFIG env var (thanks Docker!) and keep us closer to the Docker client library for when we remove the fsouza client one day.
jimmidyson
force-pushed
the
docker-tls-default-dir
branch
from
395ea26 to
f510f9d
Compare
|
lgtm - @csrwng please merge |
|
[merge] |
|
Evaluated for origin merge up to f510f9d |
|
[Test]ing while waiting on the merge queue |
|
Evaluated for origin test up to f510f9d |
|
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/12023/) (Base Commit: b2def44) |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/12036/) (Base Commit: 8bc9053) (Image: devenv-rhel7_5491) |
If DOCKER_TLS_VERIFY env var is set and DOCKER_CERT_PATH is not set, this uses the default cert path for the engine API client of $HOME/.docker to be consistent with https://github.com/fsouza/go-dockerclient/blob/e374214216d867f2c16b84972ad3bb22b56b47c3/client.go#L992-L1019.
If we could upgrade fsouza/go-dockerclient then this could drop the engine API client completely to use one consistent client, however upgrading fsouza client would require upgrading docker dependency too which is probably undesirable atm (plus being additional work).