Skip to content

Commit 136845b

Browse files
ndeloofclaude
andcommitted
Fix code review issues: replace label, observed names for disconnect/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>
1 parent f9e0f6a commit 136845b

3 files changed

Lines changed: 31 additions & 9 deletions

File tree

pkg/compose/executor.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -207,7 +207,7 @@ func (exec *planExecutor) execCreateContainer(ctx context.Context, node *PlanNod
207207
}
208208

209209
labels := mergeLabels(service.Labels, service.CustomLabels)
210-
if op.Container != nil {
210+
if op.Inherited != nil {
211211
// This is a recreate: add the replace label
212212
replacedName := op.Service.ContainerName
213213
if replacedName == "" {

pkg/compose/reconcile.go

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -148,6 +148,7 @@ func (r *reconciler) planCreateNetwork(key string, nw *types.NetworkConfig) *Pla
148148
// planRecreateNetwork adds the full sequence for a diverged network:
149149
// stop affected containers → disconnect → remove network → create network.
150150
func (r *reconciler) planRecreateNetwork(key string, nw *types.NetworkConfig) error {
151+
observed := r.observed.Networks[key]
151152
affectedServices := r.servicesUsingNetwork(key)
152153
affectedContainers := r.containersForServices(affectedServices)
153154

@@ -164,26 +165,25 @@ func (r *reconciler) planRecreateNetwork(key string, nw *types.NetworkConfig) er
164165
stopNodes = append(stopNodes, node)
165166
}
166167

167-
// Disconnect all affected containers (each depends on its own stop)
168+
// Disconnect all affected containers from the *observed* network (each depends on its own stop)
168169
var disconnectNodes []*PlanNode
169170
for i, oc := range affectedContainers {
170171
node := r.plan.addNode(Operation{
171172
Type: OpDisconnectNetwork,
172173
ResourceID: fmt.Sprintf("service:%s:%d", oc.Summary.Labels[serviceLabel], oc.Number),
173174
Cause: fmt.Sprintf("network %s recreate", key),
174175
Container: &affectedContainers[i].Summary,
175-
Name: nw.Name,
176+
Name: observed.Name,
176177
}, "", stopNodes[i])
177178
disconnectNodes = append(disconnectNodes, node)
178179
}
179180

180-
// Remove network (depends on all disconnects)
181+
// Remove the *observed* network (depends on all disconnects)
181182
removeNode := r.plan.addNode(Operation{
182183
Type: OpRemoveNetwork,
183184
ResourceID: fmt.Sprintf("network:%s", key),
184185
Cause: "config hash diverged",
185-
Name: nw.Name,
186-
Network: nw,
186+
Name: observed.Name,
187187
}, "", disconnectNodes...)
188188

189189
// Create network (depends on remove)
@@ -251,6 +251,7 @@ func (r *reconciler) planCreateVolume(key string, vol *types.VolumeConfig) *Plan
251251
// Containers must be removed (not just stopped) because Docker does not allow
252252
// removing a volume that is referenced by any container, even a stopped one.
253253
func (r *reconciler) planRecreateVolume(key string, vol *types.VolumeConfig) {
254+
observed := r.observed.Volumes[key]
254255
affectedServices := r.servicesUsingVolume(key)
255256
affectedContainers := r.containersForServices(affectedServices)
256257

@@ -279,13 +280,12 @@ func (r *reconciler) planRecreateVolume(key string, vol *types.VolumeConfig) {
279280
removeNodes = append(removeNodes, node)
280281
}
281282

282-
// Remove volume (depends on all container removals)
283+
// Remove the *observed* volume (depends on all container removals)
283284
removeVolNode := r.plan.addNode(Operation{
284285
Type: OpRemoveVolume,
285286
ResourceID: fmt.Sprintf("volume:%s", key),
286287
Cause: "config hash diverged",
287-
Name: vol.Name,
288-
Volume: vol,
288+
Name: observed.Name,
289289
}, "", removeNodes...)
290290

291291
// Create volume (depends on remove)

reconciliation.md

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -235,6 +235,28 @@ dans les tests, il faut trier les cles de maps partout : `reconcileNetworks`,
235235
`sortedKeys[V any](map[string]V)` centralise ce pattern. Le `String()` du plan trie
236236
aussi les IDs de dependances.
237237

238+
### 7.8 Disconnect/Remove utilisent les noms observes, pas desires
239+
240+
Quand un reseau ou volume a diverge, les operations de disconnect et remove doivent
241+
cibler la ressource **observee** (l'ancienne), pas la ressource **desiree** (la nouvelle
242+
config). Si le nom a change entre les deux, utiliser le nom desire pour le remove
243+
echouerait ou supprimerait la mauvaise ressource. Seul le `CreateNetwork`/`CreateVolume`
244+
utilise le nom desire.
245+
246+
### 7.9 Le label ContainerReplaceLabel necessite un signal fiable pour le recreate
247+
248+
Dans `execCreateContainer`, le label `api.ContainerReplaceLabel` doit etre ajoute pour
249+
les conteneurs crees en remplacement d'un autre (recreate). Le signal fiable est
250+
`op.Inherited != nil` (conteneur dont on herite les volumes anonymes), pas
251+
`op.Container != nil` (qui n'est pas set sur le noeud CreateContainer du recreate).
252+
253+
### 7.10 La boucle `for range` en Go 1.22+
254+
255+
Depuis Go 1.22, les variables de boucle `for _, x := range` sont copiees a chaque
256+
iteration. Le pattern `x := x` dans les goroutines n'est plus necessaire. Docker Compose
257+
utilise Go 1.25+. Si le projet devait un jour revenir a une version anterieure a 1.22,
258+
les closures dans `executePlan` devraient capturer explicitement la variable de boucle.
259+
238260
---
239261

240262
## 8. Strategie d'implementation recommandee

0 commit comments

Comments
 (0)