docs(readme): document Docker as the only backend - #122
Conversation
Removes the applecontainer and podman backend documentation from the README: the backend bullets and the Apple-container gotchas section, the applecontainer note on the compose spec-compliance row, the non-Docker entries under Install → Requires, the quick-start "swap the backend import" snippet, the runtime/applecontainer sub-package bullet, and the apple integration-test paragraph. Docker is now the only documented backend; the pluggability of runtime.Runtime is still stated, since the engine, feature pipeline, lifecycle and compose paths are written against the interface. Design records under design/ are untouched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
Included review availability: 0 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 2 reviews per hour. 📝 WalkthroughWalkthroughREADME.md now documents Docker as the supported container backend. It removes Apple-container and Podman references from backend, compose, requirements, quick-start, API, and integration-test sections. ChangesDocker-only README documentation
Estimated code review effort: 1 (Trivial) | ~3 minutes Merge Risk: ⚪ Minimal · up to This is a localized documentation-only change and no actionable merge-blocking risk remains after normal checks and review. Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0 files. (1 skipped: 1 unsupported.) ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Code Review — head 547c072
Reviewed the checked-out PR head 547c07277022e056ad6b5d537d3d0a5e2185b259 against base 71e2e099f5597c09cf3508d78726ff6b60e17471; the range is one commit changing one file (README.md, 15 additions/97 deletions). Read the base-ref engineering directives and CONTRIBUTING.md, the complete changed README, the Makefile and CI/test sources that consume its integration-test instructions, and the checkpoint/backend sources affected by the removed backend documentation. Findings were recorded for the inaccurate multi-backend integration-test instructions and the loss of checkpoint API/backend requirements. No code or tests were executed. Coverage gap: the directives reference PRD.md, but that file does not exist at the governing base commit, so no PRD claims could be checked. No design file was changed; relevant backend/checkpoint design records were inspected for contract context. Replies typed into inline comment threads will not reach this review; responses should be made in the pull request conversation.
Medium / Low
[MEDIUM] [D12] The integration-test instructions now misidentify a multi-backend target as Docker-only
-
Anchor:
README.md:210-219 -
Witness:
Makefile:13-15implementsmake test-integrationasgo test -tags=integration ... ./test/integration/..., and the real source includes Linux Podman tests gated byPODMAN_SOCKET(test/integration/podman_checkpoint_restore_test.go:1-18) plus darwin/arm64 Apple-container tests (test/integration/applecontainer_image_source_test.go:1-31). WithPODMAN_SOCKETset and no Docker daemon, or on macOS with the Apple daemon running, this target still runs real non-Docker tests; nevertheless the README says the target is “integration tests against real Docker” and that the suite is skipped when a Docker daemon is unavailable (README.md:210-219). -
Consumer: A contributor following
README.md:210-219and invokingmake test-integrationreceives the target behavior fromMakefile:13-15; the command can exercise Podman or Apple-container and can fail or consume resources even when Docker is unavailable, contrary to the stated prerequisite and skip behavior. -
Fix: Make the README describe the platform/backend gates (Docker tests, Linux Podman tests when
PODMAN_SOCKETis set, and darwin/arm64 Apple-container tests when their daemon is available), or change the Make target to explicitly select only the documented Docker suite and retain separate commands for the other backends.
Commented inline
- [MEDIUM] [D11] Removing the backend section makes the public checkpoint APIs undiscoverable and directs users to an incapable backend —
README.md:40
Verdict
ADVISORY — findings worth reading, none of them blocking.
| The container backend is pluggable: anything implementing | ||
| `runtime.Runtime` can be wired in at engine construction time. The | ||
| documented backend is: | ||
|
|
||
| - **`runtime/docker`** — Docker Engine over `moby/moby/client`. | ||
| Requires a reachable Docker daemon socket. | ||
|
|
||
| The engine, feature pipeline, lifecycle, and compose paths are written | ||
| against the `runtime.Runtime` interface rather than against Docker | ||
| directly. |
There was a problem hiding this comment.
[MEDIUM] [D11] Removing the backend section makes the public checkpoint APIs undiscoverable and directs users to an incapable backend
- Witness: The new README contains no
checkpoint,restore, orpodmanreference (the repository grep overREADME.mdreturns no matches), while it presentsruntime/dockeras the documented backend atREADME.md:31-36. The public source exposesEngine.CheckpointandEngine.Restoreatcheckpoint.go:55-66andcheckpoint.go:93-109, but those methods gate onruntime.CheckpointRuntime; the runtime contract explicitly says onlyruntime/podmanimplements it and Docker does not (runtime/runtime.go:88-99), and Docker'sCapabilitiesleavesCheckpointfalse (runtime/docker/compose_primitives.go:196-204). - Consumer: A user following the README's only backend setup and then looking for the public checkpoint feature is sent to Docker, where
Engine.Checkpoint/RestorereturnErrCheckpointUnsupported; the README no longer provides even the capability requirement or a path to the implementing backend, despite these APIs being part of the shipped root package. - Fix: Keep the Docker-only backend overview if required, but add a backend-neutral note in the API section that
Engine.Checkpoint,Restore,CheckpointProject, andRestoreProjectrequire a runtime implementingruntime.CheckpointRuntime(and link the capability/backend documentation), or retain the minimal Podman prerequisite needed to make those public APIs usable.
Follow-up to #121 (which removed the Apple Containers CI jobs). Strips the applecontainer and podman backend documentation from the README, leaving Docker as the only documented backend.
What changed
runtime/applecontainerandruntime/podmanbullets and the "All three backends" line. Kept the pluggability statement, since the engine, feature pipeline, lifecycle and compose paths are written againstruntime.Runtimerather than against Docker directly.runtime/dockerandruntime/applecontainer".runtime/applecontainerbullet.Docs only, no code changes.
grep -i "apple\|podman\|criu\|checkpoint\|swift\|darwin\|macos" README.mdis now empty. 318 → 236 lines.Deliberately out of scope
design/is untouched —runtime-applecontainer.md,podman-backend.mdandcheckpoint-restore.mdare design records of decisions that were actually made, anddesign/README.mdstill indexes them. No code was removed:runtime/applecontainer/,applecontainer-bridge/andruntime/podman/all still ship.Two things reviewers should know
Podman is not in the same state as applecontainer. Its CI job is still alive —
test-integration-podmanat.github/workflows/ci.yml:103runs real Podman + CRIU checkpoint/restore in a privilegedquay.io/podman/stablecontainer, and it was untouched by #121. So after this PR the README documents less than CI actually covers. That is intentional per the request, not an oversight.Checkpoint/restore lost its only README mention.
Engine.Checkpoint,Engine.Restore,Engine.CheckpointProjectandEngine.RestoreProject(checkpoint.go:66,109,checkpoint_project.go:110,193) are public root-package API, andruntime/podmanis the only backend implementingruntime.CheckpointRuntime. The podman bullet was the README's only explanation of what those four methods require; that is now documented only underdesign/checkpoint-restore.md. A backend-neutral line under API surface ("checkpoint/restore requires a backend implementingruntime.CheckpointRuntime") would close that gap without naming Podman — happy to add it if wanted.🤖 Generated with Claude Code
Summary by CodeRabbit