Open
Conversation
Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
String() is designed to make it easy to compare coomputed plan vs expectations Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
ndeloof
force-pushed
the
reconciliation_plan
branch
4 times, most recently
from
e548eb4 to
48ff1db
Compare
Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
ndeloof
force-pushed
the
reconciliation_plan
branch
from
48ff1db to
a062d54
Compare
- Remove the `convergence` struct and `newConvergence` constructor - Extract `resolveServiceReferences` as a standalone function taking `map[string]Containers` instead of a method on convergence - Add `getContainersByService` helper on composeService - Update run.go and executor.go to use the new standalone function - Remove dead code: `getObservedState`, `setObservedState` Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
ndeloof
force-pushed
the
reconciliation_plan
branch
from
a057fa0 to
46e274e
Compare
Contributor
Author
|
/review |
Rewrite the design document based on what was actually implemented, lessons learned during CI validation, and guidance for future work. Key additions: section 7 (lessons learned) with the 7 pitfalls encountered, updated architecture reflecting that ensureNetworks/ ensureVolumes remain unchanged, and streamlined implementation guide. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Introduces a new “reconciliation plan” pipeline for docker compose create by replacing the legacy convergence flow with: observed state collection → plan generation (DAG) → parallel plan execution.
Changes:
- Added new reconciliation components:
ObservedState,Plan,reconcilelogic, and a parallelexecutePlanexecutor with grouped progress events. - Refactored
create()to use the new pipeline (including “Running” events for unchanged containers) and removed most of the oldconvergenceimplementation. - Updated
runto resolveservice:references withoutconvergence, and added/updated unit tests for the new modules.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| reconciliation.md | Architecture/design doc for the new reconciliation engine and pipeline. |
| pkg/compose/run.go | Replaces convergence-based service-reference resolution for run with a new helper. |
| pkg/compose/reconcile_test.go | Adds unit tests validating plan output for networks/volumes/containers/orphans. |
| pkg/compose/reconcile.go | Implements reconciler producing a deterministic DAG plan for infra + containers. |
| pkg/compose/progress.go | Removes runningEvent helper (replaced by direct newEvent usage). |
| pkg/compose/plan_test.go | Adds unit tests for Plan and OperationType stringification. |
| pkg/compose/plan.go | Introduces Plan, PlanNode, Operation, and OperationType abstractions. |
| pkg/compose/observed_state_test.go | Adds tests for observed-state snapshot construction and classification. |
| pkg/compose/observed_state.go | Adds snapshot types + collection logic; adds “Running” events emission helper. |
| pkg/compose/executor_test.go | Adds executor tests using mocked API client calls. |
| pkg/compose/executor.go | Implements parallel plan execution, operation handlers, and grouped progress events. |
| pkg/compose/create.go | Refactors create() to build observed state → reconcile → emit running events → execute plan. |
| pkg/compose/convergence.go | Removes convergence struct/logic; keeps/exports service-reference resolution helpers. |
| pkg/compose/containers.go | Adds getContainersByService() and removes Containers.names(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+8
to
+10
| ensureImagesExists -> ensureNetworks/Volumes -> collectObservedState -> reconcile -> executePlan | ||
| (inchange) (inchange) (1) (2) (3) | ||
| ``` |
There was a problem hiding this comment.
Spelling: "inchange" should be "inchangé".
…remove - Fix ContainerReplaceLabel detection: use op.Inherited != nil (not op.Container) as signal for recreate in execCreateContainer - Use observed network name (not desired) for DisconnectNetwork and RemoveNetwork operations, in case the name changed - Use observed volume name (not desired) for RemoveVolume operations - Update reconciliation.md with 3 new lessons learned (7.8, 7.9, 7.10) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
2 tasks
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
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.
this is an alternative for #13641
based on previous work, I first generated an analysis an plan document, so AI work on this significant refactoring is better guided and impact is well understood