support for non-default docker host added#16477
Conversation
| self.docker_client = docker.DockerClient(base_url=base_url, version='auto') | ||
| break | ||
| except: | ||
| continue |
There was a problem hiding this comment.
If every try fails, we could have a pretty error for the user in that case
There was a problem hiding this comment.
In that case, we rise this exception:
raise ConanException("Docker Client failed to initialize."
"\n - Check if docker is installed and running"
"\n - Run 'pip install conan[runners]'")
memsharded
left a comment
There was a problem hiding this comment.
Looks good to me if it works, just some potential readability minor issues.
| docker_base_urls = [ | ||
| os.environ.get('CONAN_RUNNER_DOCKER_HOST'), | ||
| os.environ.get('DOCKER_HOST'), | ||
| 'unix:///var/run/docker.sock', | ||
| f'unix://{os.path.expanduser("~")}/.rd/docker.sock' | ||
| ] | ||
| for base_url in docker_base_urls: |
There was a problem hiding this comment.
Sounds a bit hacky, some suggestions:
- Add some comment explaining the why. Explain the different URLs a bit
- Is it possible to unify the above
docker.from_env()and this call in a single one? so trying to connect the docker client looks like less like a workaround hack?
| self.docker_api = self.docker_client.api | ||
| docker_base_urls = [ | ||
| None, # Default docker configuration, let the python library detect the socket | ||
| os.environ.get('CONAN_RUNNER_DOCKER_HOST'), # Connect to socket defined in CONAN_RUNNER_DOCKER_HOST |
There was a problem hiding this comment.
But why CONAN_RUNNER_DOCKER_HOST? This is not something defined by Rancher or anyone else, I suggest removing and leaving DOCKER_HOST if that is a "standard" docker env-var.
| for base_url in docker_base_urls: | ||
| try: | ||
| self.docker_client = docker.DockerClient(base_url=base_url, version='auto') |
There was a problem hiding this comment.
Some info message, maybe at least ConanOutput().verbose() of the things that are being tried would make sense.
|
Confirmed that I can now run ny own docker images from rancher without problems now :) |
Changelog: Fix: Add support for non default docker hosts in conan runners
Docs: Omit