-
Notifications
You must be signed in to change notification settings - Fork 717
Enhancements for compose up command. (--remove-orphans)
#685
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enhancements for compose up command. (--remove-orphans)
#685
Conversation
pkg/composer/up.go
Outdated
| return err | ||
| } | ||
|
|
||
| orphans, err := c.getOrphanContainers(ctx, parsedServices) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This operation might be slow and should be called only when RemoveOrphans is true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @AkihiroSuda, I appreciate this operation could introduce additional delays into the compose up workflow, but without performing it we cannot warn users who have not specified --remove-orphans that orphaned containers exist:
if numOrphans > 0 {
if uo.RemoveOrphans {
if err := c.downContainers(ctx, orphans, true); err != nil {
return fmt.Errorf("error removing orphaned containers: %s", err)
}
} else {
logrus.Warnf("%d orphaned containers exist. Run with --remove-orphans to remove them.", numOrphans)
}
}This would be a deviation from the behaviour of docker-compose, which prints a similar warning.
We can probably reduce the time taken without losing this error message by comparing the total number of containers in the parsedServices slice with the total number of running containers in the compose project; this would remove the requirement to call the .Labels() method for each container:
for _, container := range containers {
containerLabels, err := container.Labels(ctx)
if err != nil {
return false, err
}
if name == containerLabels[labels.Name] {
// container exists
return true, nil
}
}Please let me know if you're happy with that approach, and I'll make the changes accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be a deviation from the behaviour of docker-compose, which prints a similar warning.
I think this difference is acceptable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ping @fhke
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @AkihiroSuda, apologies for the delay. I've updated this PR with the requested changes.
This PR contains the following updates to the `compose up` command: * Idempotency: rather than failing when a container already exists and has been created by compose, recreate the container. This allows users to iterate rapidly with `compose up`, rather than deleting & recreating their compose stack to effect changes. * Add support for `--remove-orphans` flag: remove containers for services not defined in the Compose file. This fixes #342 Signed-off-by: fergal kearns <fhke@protonmail.com>
AkihiroSuda
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
compose up command.compose up command. (--remove-orphans)
This PR contains the following updates to the
compose upcommand:compose up, rather than deleting & recreating their compose stack to effect changes.--remove-orphansflag: remove containers for services not defined in the Compose file. This fixes nerdctl compose up support for remove-orphans #342Signed-off-by: fergal kearns fhke@protonmail.com