Skip to content
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

Improve completion of containers for docker rm #5527

Merged
merged 1 commit into from
Oct 11, 2024

Conversation

albers
Copy link
Collaborator

@albers albers commented Oct 10, 2024

- What I did
The cobra completion for docker rm completes all containers. This does not make much sense as not all containers are removable unless --force is given.
This PR aligns the completion to the legacy completion, where only removable containers are completed unless --force is given, see here.

- How I did it
Added a filter to the call of completion.ContainerNames that filters on container status and the value of --force.

- How to verify it

- Description for the changelog

Improve completion of containers for `docker rm`

@thaJeztah PTAL

@codecov-commenter
Copy link

codecov-commenter commented Oct 10, 2024

Codecov Report

Attention: Patch coverage is 0% with 3 lines in your changes missing coverage. Please review.

Project coverage is 60.67%. Comparing base (88f1e99) to head (147630a).
Report is 30 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5527      +/-   ##
==========================================
- Coverage   60.67%   60.67%   -0.01%     
==========================================
  Files         345      345              
  Lines       23488    23491       +3     
==========================================
  Hits        14252    14252              
- Misses       8263     8266       +3     
  Partials      973      973              

@albers

This comment was marked as resolved.

@@ -38,7 +38,9 @@ func NewRmCommand(dockerCli command.Cli) *cobra.Command {
Annotations: map[string]string{
"aliases": "docker container rm, docker container remove, docker rm",
},
ValidArgsFunction: completion.ContainerNames(dockerCli, true),
ValidArgsFunction: completion.ContainerNames(dockerCli, true, func(ctr container.Summary) bool {
return opts.force || ctr.State == "exited" || ctr.State == "created"
Copy link
Member

Choose a reason for hiding this comment

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

This is totally fine for now, but I wish we had an enum (or whatever passes for one in Go) for the State.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, yes! I think I may have started working on a branch at some point, but need to dig it up. It's all a bit awkward as these parts are rather quirky, and these values are currently set in the container package, not in anything inside the api/xxx or client/xxx; https://github.com/moby/moby/blob/61030f0e87c8d893082599815897f10d440f9bda/container/state.go#L119-L158

// StateString returns a single string to describe state
func (s *State) StateString() string {
	if s.Running {
		if s.Paused {
			return "paused"
		}
		if s.Restarting {
			return "restarting"
		}
		return "running"
	}

	if s.RemovalInProgress {
		return "removing"
	}

	if s.Dead {
		return "dead"
	}

	if s.StartedAt.IsZero() {
		return "created"
	}

	return "exited"
}

// IsValidStateString checks if the provided string is a valid container state or not.
func IsValidStateString(s string) bool {
	if s != "paused" &&
		s != "restarting" &&
		s != "removing" &&
		s != "running" &&
		s != "dead" &&
		s != "created" &&
		s != "exited" {
		return false
	}
	return true
}

Copy link
Member

Choose a reason for hiding this comment

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

I recall now that I also wanted to look at these completion helpers to allow passing filters so that the daemon would only return containers in specific state(s) to get the equivalent of;

docker ps --format='{{.Names}} {{.Status}}' --filter status=exited --filter status=created
container-1 created
container-2 exited
container-3 exited

(and perhaps add a != option for status to allow --filter status!=running)

Copy link
Member

@laurazard laurazard left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM, nice!!

@thaJeztah thaJeztah merged commit 21eea1e into docker:master Oct 11, 2024
95 checks passed
@thaJeztah thaJeztah added this to the 28.0.0 milestone Oct 11, 2024
@albers albers deleted the completion-container-rm branch October 13, 2024 11:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants