Shell out to docker instead of using bollard#2629
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0e557b3582
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
This pull request replaces the bollard-based Docker integration in stellar container start|stop|logs with direct docker CLI invocations, delegating host/socket resolution (including ssh://) to Docker itself and removing the unused bollard dependency from the workspace.
Changes:
- Reimplemented container
start,stop, andlogsto shell out todocker, applying--docker-host/DOCKER_HOSTby settingDOCKER_HOSTon the spawned process. - Simplified container error handling by parsing
dockerstderr for “already in use” / “No such container” cases. - Removed the
bollarddependency from the workspace and affected crates.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| cmd/soroban-cli/src/commands/container/start.rs | Switches container startup/pull logic to docker pull + docker run, adds stderr-based error mapping. |
| cmd/soroban-cli/src/commands/container/stop.rs | Switches container stop to docker stop and maps “No such container” via stderr. |
| cmd/soroban-cli/src/commands/container/logs.rs | Switches log tailing to docker logs -f --tail all with inherited stdio. |
| cmd/soroban-cli/src/commands/container/shared.rs | Replaces bollard connection logic with a docker_command() builder that sets DOCKER_HOST when provided. |
| cmd/soroban-cli/Cargo.toml | Removes bollard dependency from the soroban-cli crate. |
| cmd/crates/stellar-ledger/Cargo.toml | Removes bollard dependency from stellar-ledger crate. |
| Cargo.toml | Removes bollard from workspace dependencies. |
| Cargo.lock | Removes bollard from the resolved dependency graph. |
What
Replaces the
bollardDocker API client with directdockerCLI invocations in thestellar container start|stop|logscommands. The--docker-hostflag (and theDOCKER_HOSTenv var) is now applied by settingDOCKER_HOSTon the spawneddockerprocess; host resolution is otherwise left to docker itself. The unusedbollarddependency instellar-ledgeris also removed.Why
The bollard connection code copied unreleased upstream source, pinned a hardcoded API version, and reimplemented host/socket resolution plus a Docker Desktop fallback that the
dockerCLI already handles natively. It also rejected any host scheme it didn't recognize — e.g.ssh://, which docker supports — withURI scheme is not supported. Shelling out todockerremoves that hand-rolled logic and lets docker resolve the connection, including remotessh://hosts.Known limitations
The
dockerbinary must now be installed and on the user'sPATH; if it isn't, commands fail with a "is docker installed and on your PATH?" hint.