filters: accept Docker-compat status values "dead" and "restarting"#28926
filters: accept Docker-compat status values "dead" and "restarting"#28926crawfordxx wants to merge 4 commits into
Conversation
Honny1
left a comment
There was a problem hiding this comment.
I have some comments about testing. Also, I miss integration tests for podman ps --filter status=dead, etc...
|
|
||
| #libpod api list containers sanity checks | ||
| t GET libpod/containers/json?filters='{"status":["removing"]}' 200 length=0 | ||
| # Docker-compat: "dead" and "restarting" have no Podman equivalent; accept them |
There was a problem hiding this comment.
The tests only verify the libpod path (libpod/containers/json). Since the issue reporter and the Swagger docs reference both the compat and libpod endpoints. It would be more thorough to also add a compat-path.
Add tests that were missing per reviewer feedback: 1. Compat API path (containers/json): verify that status=dead and status=restarting are accepted without error and return an empty list, matching the existing libpod-path tests. 2. podman ps --filter status=dead/restarting: verify the same behaviour through the CLI, since the issue was originally reported via podman ps. Closes the review comments on podman-container-tools#28926.
|
Thanks for the review! I've pushed a follow-up commit that addresses both points:
|
| // container states that have no direct equivalent in Podman. | ||
| // Accept them without error; they will not match any container. | ||
| if filterValue == "dead" || filterValue == "restarting" { | ||
| continue |
There was a problem hiding this comment.
Keeping aside docker-compat; does this not end up being a misleading signal? how is the consumer to distinguish between "filter honored and no containers matched" vs "filter not-honored"?
|
I think I'd rather only accept these values in our docker compat api, since these are docker values that don't exist in podman land. |
| case "status": | ||
| for _, filterValue := range filterValues { | ||
| // Docker compat: "dead" and "restarting" are valid Docker | ||
| // container states that have no direct equivalent in Podman. |
There was a problem hiding this comment.
Actually "dead" has a podman equivalent:
> podman inspect unruffled_keller | grep -i dead
"Dead": false,podman/libpod/container_inspect.go
Line 132 in d84fb8b
The /libpod/containers/json API documents "dead" and "restarting" as valid status filter values (matching Docker's API), but passing either to StringToContainerStatus causes an "unknown container state" error because Podman has no such internal states. Accept "dead" and "restarting" as no-op filters: skip validation and return no containers for those values, preserving Docker compatibility without breaking the documented API contract. Fixes podman-container-tools#28904 Signed-off-by: crawfordxx <crawfordxx@users.noreply.github.com>
These two Docker status values have no Podman equivalent and should be accepted by the libpod /containers/json status filter without error, returning an empty list of containers. Relates-To: podman-container-tools#28904 Signed-off-by: crawfordxx <crawfordxx@users.noreply.github.com>
Add tests that were missing per reviewer feedback: 1. Compat API path (containers/json): verify that status=dead and status=restarting are accepted without error and return an empty list, matching the existing libpod-path tests. 2. podman ps --filter status=dead/restarting: verify the same behaviour through the CLI, since the issue was originally reported via podman ps. Closes the review comments on podman-container-tools#28926. Signed-off-by: crawfordxx <crawfordxx@users.noreply.github.com>
Per review feedback, limit acceptance of Docker status values 'dead' and 'restarting' to the compat (Docker-compatible) API path only. In the compat containers list handler, strip these values from the status filter before passing to the domain layer. If the filter contained only these Docker-specific values, return an empty list immediately (no Podman container can be in those states). The libpod API continues to reject unknown status values as before. Remove the libpod tests that were checking this behaviour. Signed-off-by: crawfordxx <crawfordxx@users.noreply.github.com>
7a1050b to
8599c21
Compare
|
Thanks for all the feedback! I've pushed an update that addresses the review concerns:
The libpod tests for 'dead'/'restarting' have been removed; only the compat-path test remains. |
What did you do?
The
GET /libpod/containers/jsonAPI documentsdeadandrestartingas validstatusfilter values (matching Docker's API contract), but passing either causes a 500 error:What did you expect to happen?
status=deadandstatus=restartingshould be accepted without error (returning an empty list, since Podman has no such internal states).What actually happened?
StringToContainerStatusrejects them as unknown, bubbling up a 500.How to reproduce it (as minimally and precisely as possible)?
Fixes
Fixes #28904
Description of fix
Skip
StringToContainerStatusvalidation for"dead"and"restarting"in thestatusfilter; they are silently accepted and return no containers, which matches Docker's behavior for states that don't apply to the engine.