Skip to content
This repository has been archived by the owner on Nov 27, 2023. It is now read-only.

introduce compose rm command #1299

Merged
merged 2 commits into from
Feb 15, 2021
Merged

introduce compose rm command #1299

merged 2 commits into from
Feb 15, 2021

Conversation

ndeloof
Copy link
Collaborator

@ndeloof ndeloof commented Feb 15, 2021

What I did
Reproduced docker-compose rm command and flags

/test-windows

(not mandatory) A picture of a cute animal, if possible in relation with what you did
image

@ndeloof ndeloof requested a review from aiordache February 15, 2021 08:18
@github-actions github-actions bot added aci api api cli cli ecs kube Kubernetes backend local Local context (moby) labels Feb 15, 2021
Signed-off-by: Nicolas De Loof <[email protected]>
projectOptions: p,
}
cmd := &cobra.Command{
Use: "rm [SERVICE...]",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we add remove as alias?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there's no such thing as a remove alias on docker rm nor docker-compose rm, so to prevent heterogeneous UX I'd prefer we keep such "UX improvement" a separate debate

}
msg := fmt.Sprintf("Going to remove %s", strings.Join(names, ", "))
if options.Force {
fmt.Println(msg)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about it but this may be the cause for the inconsistent output with the progress writer:

$ ./bin/docker compose rm --stop --force
The new 'docker compose' command is currently experimental. To provide feedback or request new features please open issues at https://github.com/docker/compose-cli
[+] Running 0/1
[+] Running 1/1mpose-cli_web_1  Stopping                                                         0.3s
 ⠿ Container compose-cli_web_1  Removed                                                          0.4s

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh yes indeed, need to get user interaction to take place before the progresswriter is started. But can't do this without breaking the backend API abstraction. Damned :'(

w := progress.ContextWriter(ctx)
eg, ctx := errgroup.WithContext(ctx)
for _, c := range stoppedContainers {
c := c
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

required. go iteration re-assigning c variable. such a stupid idiom (ㆆ _ ㆆ)

@ndeloof ndeloof merged commit 9063c13 into main Feb 15, 2021
@ndeloof ndeloof deleted the remove branch February 15, 2021 14:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
aci api api cli cli ecs kube Kubernetes backend local Local context (moby)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants