Skip to content

Shell out to docker instead of using bollard#2629

Open
fnando wants to merge 2 commits into
mainfrom
drop-bollard
Open

Shell out to docker instead of using bollard#2629
fnando wants to merge 2 commits into
mainfrom
drop-bollard

Conversation

@fnando

@fnando fnando commented Jun 26, 2026

Copy link
Copy Markdown
Member

What

Replaces the bollard Docker API client with direct docker CLI invocations in the stellar container start|stop|logs commands. The --docker-host flag (and the DOCKER_HOST env var) is now applied by setting DOCKER_HOST on the spawned docker process; host resolution is otherwise left to docker itself. The unused bollard dependency in stellar-ledger is 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 docker CLI already handles natively. It also rejected any host scheme it didn't recognize — e.g. ssh://, which docker supports — with URI scheme is not supported. Shelling out to docker removes that hand-rolled logic and lets docker resolve the connection, including remote ssh:// hosts.

$ stellar container start --docker-host ssh://fnando@fnando-server.local
ℹ️ Starting local network
ℹ️ testing: Pulling from stellar/quickstart
ℹ️ Digest: sha256:3b98cc624a235215bbb2dedd18bd3ec3a498053b6b6533533b6a0d94354a7885
ℹ️ Status: Image is up to date for stellar/quickstart:testing
✅ Started container
🔎 Watch logs with `stellar container logs local --docker-host ssh://fnando@fnando-server.local`
ℹ️ Stop the container with `stellar container stop local --docker-host ssh://fnando@fnando-server.local`

$ stellar container start --docker-host ssh://fnando@fnando-server.local
ℹ️ Starting local network
ℹ️ testing: Pulling from stellar/quickstart
ℹ️ Digest: sha256:3b98cc624a235215bbb2dedd18bd3ec3a498053b6b6533533b6a0d94354a7885
ℹ️ Status: Image is up to date for stellar/quickstart:testing
❌ error: a container named "stellar-local" already running

$ stellar container logs --docker-host ssh://fnando@fnando-server.local
Starting Stellar Quickstart
versions:
  quickstart: 16a57418b555ab859e133550c449ff15c2f5c06e
  xdr:
    stellar-xdr 27.0.0 (5262803470be965e42f80023d12fba12808c774a)
    xdr: 68fa1ac55692f68ad2a2ca549d0a283273554439
    features: <none>
  ...

$ stellar container stop --docker-host ssh://fnando@fnando-server.local
ℹ️ Stopping local container
✅ Container stopped

$ stellar container stop --docker-host ssh://fnando@fnando-server.local
ℹ️ Stopping local container
❌ error: container local not found

Known limitations

The docker binary must now be installed and on the user's PATH; if it isn't, commands fail with a "is docker installed and on your PATH?" hint.

Copilot AI review requested due to automatic review settings June 26, 2026 19:44
@github-project-automation github-project-automation Bot moved this to Backlog (Not Ready) in DevX Jun 26, 2026
@fnando fnando moved this from Backlog (Not Ready) to Needs Review in DevX Jun 26, 2026
@fnando fnando self-assigned this Jun 26, 2026

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread cmd/soroban-cli/src/commands/container/shared.rs Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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, and logs to shell out to docker, applying --docker-host/DOCKER_HOST by setting DOCKER_HOST on the spawned process.
  • Simplified container error handling by parsing docker stderr for “already in use” / “No such container” cases.
  • Removed the bollard dependency 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.

Comment thread cmd/soroban-cli/src/commands/container/start.rs
Comment thread cmd/soroban-cli/src/commands/container/start.rs
@fnando fnando requested a review from mootz12 June 26, 2026 20:27
@fnando fnando enabled auto-merge (squash) June 26, 2026 21:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Needs Review

Development

Successfully merging this pull request may close these issues.

2 participants