chore(compose): comments state behavior, not history - #14130
Merged
Conversation
docker-agent
left a comment
docker-agent
left a comment
Contributor
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
All eleven comment rewrites accurately describe the behavior of the code they annotate. No logic changes, no misleading descriptions, and no omitted critical caveats were found across create.go, observed_state.go, and reconcile.go.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
glours
previously approved these changes
Aug 27, 2026
glours
left a comment
glours
left a comment
Contributor
There was a problem hiding this comment.
Nit: pkg/compose/reconcile_test.go:1245 still has // Exited containers are left as-is, matching convergence.go:199 behavior same stale-pointer pattern this PR fixes elsewhere, just left out of scope here (probably because it's a test file rather than "the reconciler" itself). Not blocking, just flagging for a follow-up pass.
Eleven comments in the reconciler and its neighbors justified decisions
by pointing at code that no longer exists ('matching the previous
ensureNetwork behavior', 'same way as convergence.go:138-160' — lines
that today hold unrelated code) instead of stating the behavior and its
reason. A reader following those pointers finds nothing, or worse,
something else.
Each is rewritten as a self-contained behavioral statement. Two carry
real weight:
- reconcileService's up-to-date branch now says explicitly that
starting containers is NOT the plan's job (a separate start phase in
start.go does it) — the previous wording invited 'fixing' the
StateCreated case;
- sortContainers now documents that its ordering IS the scale-down
policy (the tail it produces is what i >= expected trims), instead of
referencing stale line numbers.
Part of docker#14074 (A: the code misdescribes its own structure).
Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
ndeloof
force-pushed
the
a2-comments-state-behavior
branch
from
August 27, 2026 09:55
329cb6a to
080cc85
Compare
glours
enabled auto-merge (rebase)
August 27, 2026 09:58
glours
approved these changes
Aug 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Eleven comments in the reconciler justified decisions by referencing deleted code ("matching the previous ensureNetwork behavior", stale line numbers to code that moved). Each is rewritten as a self-contained behavioral statement — including that starting containers is not the plan's job, and that
sortContainers' ordering is the scale-down policy.Epic #14074, section A — split out of #14075 for focused review.
🤖 Generated with Claude Code