fix: remove stale instance entry on recreate and guard stop against missing containers#468
fix: remove stale instance entry on recreate and guard stop against missing containers#468Syedowais312 wants to merge 4 commits into
Conversation
…issing containers Signed-off-by: syedowais312 <syedowais312sf@gmail.com>
|
Matching by |
Signed-off-by: syedowais312 <syedowais312sf@gmail.com>
f3b0073 to
700e6db
Compare
i have added appropriate test cases in pkg/config/localconfig_test.go |
|
Thanks for adding tests after the earlier review feedback. One thing I want to call out: the PR title mentions “guard stop against missing containers”, but I don’t see an actual missing-container guard added in
|
a859375 to
700e6db
Compare
Signed-off-by: syedowais312 <syedowais312sf@gmail.com>
a5e72de to
cd4b467
Compare
sorry i missed it earlier. |
Signed-off-by: syedowais312 <syedowais312sf@gmail.com>
060992b to
6a740ad
Compare
Vaishnav88sk
left a comment
There was a problem hiding this comment.
Please resolve that. After that LGTM. 👍🏻
| exists, _ := containerClient.ContainerExists(instance.ContainerID) | ||
| errors.CheckError(err) |
There was a problem hiding this comment.
There is a bug here: ContainerExists(...) returns (bool, error), but the error is being discarded with _, and then errors.CheckError(err) is checking the previous err value instead.
Please capture the returned error here and validate that one.
There was a problem hiding this comment.
Also line ~70,
This should stay as RemoveInstance(instance.ContainerID), not RemoveInstance(instance.Name).
Switching it back to instance.Name reintroduces the original issue this PR is fixing, since RemoveInstance() was changed to match by ContainerID.
Problem
When a container is removed externally (e.g.
docker system prune) andmicrocks startrecreates it, the old instance entry was never removed fromconfig causing duplicate instance records to accumulate on every recreate.
Two root causes:
RemoveInstancematched byName(not unique) instead ofContainerID(unique)autoRemovepath instop.gowas passinginstance.NametoRemoveInstance,silently failing to remove the correct entry
start.gowas clearinginstance.ContainerID = ""instead of callingRemoveInstanceto actually remove the stale record from configChanges
cmd/start.go: calllocalConfig.RemoveInstance(instance.ContainerID)toremove the stale record before recreating, instead of just clearing the local variable
cmd/stop.go: passinstance.ContainerIDtoRemoveInstancein theautoRemovepathpkg/config/localconfig.go:RemoveInstancenow matches byContainerIDinstead ofNameTesting
Ran
docker rm <container>while config showsstatus: Running, thenmicrocks start --name <instance>config shows a single clean entry withthe new container ID, no duplicates.
Realted: #456
Fix: #483