DOCKER_HOST handling fix #1960
DOCKER_HOST handling fix #1960
Conversation
AndreiOrmanji
changed the title
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR updates Docker connection handling across the codebase to improve compatibility with various Docker host configurations and modernizes the bollard dependency.
- Updates bollard dependency to v0.18.1 with
ssl_providerlessfeature inCargo.tomlfor better SSL handling - Replaces
connect_with_local_defaults()withconnect_with_defaults()inprovisioner_server.rsto properly handle tcp/http/https DOCKER_HOST configurations - Updates
connect_with_unixtoconnect_with_socketingateway/src/service.rsto align with bollard's renamed API
💡 (1/5) You can manually trigger the bot by mentioning @greptileai in a comment!
4 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings | Greptile
gateway/src/service.rs
Outdated
| @@ -325,7 +325,7 @@ impl GatewayService { | |||
| )); | |||
| } | |||
|
|
|||
| let docker = Docker::connect_with_unix(&args.docker_host, 60, API_DEFAULT_VERSION).unwrap(); | |||
| let docker = Docker::connect_with_socket(&args.docker_host, 60, API_DEFAULT_VERSION).unwrap(); | |||
There was a problem hiding this comment.
style: Consider adding error handling here instead of unwrap() since Docker connection is critical for service operation
AndreiOrmanji
force-pushed
the
cargo-shuttle-docker-host-fix
branch
from
fb8522a to
2f7a0e1
Compare
There was a problem hiding this comment.
PR Summary
Based on the latest changes, here's my concise expert review focusing only on new/different aspects not covered in the previous review:
This PR completes the Docker host configuration improvements by updating the remaining connection handling code in the gateway service.
- Adds Docker connection timeout configuration in
gateway/src/service.rswith a 60-second default - Implements consistent Docker host handling across test utilities in
gateway/src/lib.rs - Ensures proper error handling for Docker connection failures in production/staging environments
The changes look well-considered and maintain consistency across the codebase while improving Docker connectivity reliability.
4 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile
| // This only constructs the client and does not try to connect. | ||
| // If the socket is not found, a "no such file" error will happen on the first request to Docker. | ||
| Ok(Self { | ||
| docker: Docker::connect_with_local_defaults()?, | ||
| docker: Docker::connect_with_defaults()?, |
There was a problem hiding this comment.
@fussybeaver Is the comment above this line still true for the new version? CI is failing for tests that shouldn't try to connect to Docker.
There was a problem hiding this comment.
I believe connect_with_defaults is a new method that tries to infer from environment variables whether you are connecting to a socket or https or a named pipe - IIRC it was added for better compatibility with testcontainers and co. I would stick to the old method if CI is failing, because we may not be setting those env vars appropriately.
Description of change
fixes #1959
Cargo.tomlconnect_with_unixtoconnect_with_socket, because it was renamed in new versionsreplace calls of
connect_with_local_defaultstoconnect_with_defaultsto fix cases when DOCKER_HOST is set up as tcp/http/httpsHow has this been tested? (if applicable)
By running tests locally