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
pass All option to backend api.Service when length statuses is not equal to zero #11475
Conversation
I am not sure if the issue is the code or the documentation. The PR addresses the issue in the code but the issue could also be the documentation. :) |
@ndeloof would appreciate it if you could take a look here |
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.
LGMT, could be simplified by just forcing All
when a status filter is selected
cmd/compose/ps.go
Outdated
@@ -113,7 +113,7 @@ func runPs(ctx context.Context, dockerCli command.Cli, backend api.Service, serv | |||
|
|||
containers, err := backend.Ps(ctx, name, api.PsOptions{ | |||
Project: project, | |||
All: opts.All, | |||
All: opts.All || containsStatus(opts.Status, "exited"), |
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.
AFAICT as long as we filter by status we can force All
, as irrelevant containers will anyway be filtered
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.
You are right but in cases where users explicitly set it to false we will not be able to see the exited containers.
IMO either we should throw an error in these cases or have the default behaviour as in the change.
Open to your opinion on this.
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.
opts.All || opts.Status != ""
sounds enough to me. We could check --all
flag was not intentionally set to false
in command PreRun
to prevent misuse, but as a corner case with no benefit I wouldn't worry about it
…ual to zero Signed-off-by: 1arp <dekhijaegi@gmail.com>
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #11475 +/- ##
==========================================
+ Coverage 58.09% 58.12% +0.03%
==========================================
Files 136 136
Lines 11543 11581 +38
==========================================
+ Hits 6706 6732 +26
- Misses 4177 4185 +8
- Partials 660 664 +4 ☔ View full report in Codecov by Sentry. |
@ndeloof Can you please rerun the tests, seems like there was a timeout while getting the docker image. |
What I did
For ps command: Aligned --status=exited behaviour with the documentation.
https://docs.docker.com/engine/reference/commandline/compose_ps/#status
The compose service internally calls docker cli client which lists only running containers by default unless the
all
flag is passed.With the change the all flag is passed when the statuses includes
exited
.Related issue
Fixes #11464
(not mandatory) A picture of a cute animal, if possible in relation to what you did