You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Parse health status from Podman ps JSON Status field.
Report parsed status for running containers before falling back to podman inspect.
Preserve previous semantics for stopped containers by reporting healthStatus none.
Keep missing Status backward-compatible by falling back to inspect, while treating an explicit empty Status as no health check.
Add parser tests for real Status output, missing status fallback, empty status normalization, unknown status fallback, and case/whitespace normalization.
Why
Podman v6 source shows ListContainerBatch populates the list response Status from HealthCheckStatus. Using that value avoids extra per-container inspect calls when Podman includes Status, while preserving older or unexpected output through the existing inspect fallback path.
Review follow-up
The original PR parsed a nonexistent Health field. Review feedback correctly flagged that as inert. This update switches the implementation and tests to the actual Status field from Podman list output.
Validation
GOCACHE=/private/tmp/techulus-cloud-go-build GOMODCACHE=/private/tmp/techulus-cloud-go-mod go test ./...
I read both branches locally (origin/main vs origin/codex/podman6-health-status). The code itself is clean, defensive, and well-tested — go test ./internal/container/ and go vet both pass, and the fallback path guarantees no regression versus current behavior. But I think the central premise of the PR is unfounded, and as written the change is effectively a no-op that never delivers its stated benefit.
Blocking concern: podman ps --format json does not emit a Health field
The change hinges on podman ps -a --format json (in container.List) returning a top-level Health key that the new Health json.RawMessage \json:"Health"`` tag can bind to. It doesn't.
podman ps --format json serializes Podman's ListContainer struct. The current struct (pkg/domain/entities/types/container_ps.go on main) has no Health field — the health-related fields are only:
State string — e.g. "running"
Status string — documented in-source as "a human-readable approximation of a duration" (e.g. "Up 2 minutes"); it does not contain the health word for JSON output.
Health status (healthy/unhealthy/starting) lives under .State.Health.Status, which is only reachable via podman inspect — exactly what the existing GetHealthStatus already does. Podman only surfaces health in ps via the {{.Status}} table column and the --filter health= filter, neither of which is a JSON field.
Consequences:
pc.Health will always be empty → normalizeHealthStatus always returns "" → reporting.go always falls through to container.GetHealthStatus(c.ID). So the promised win ("avoids extra per-container inspect calls on newer Podman versions") is never realized; every running container still gets an inspect call, same as before.
The new runtime_test.go cases feed hand-crafted JSON that includes a "Health" key. That input shape is never produced by real podman ps, so the tests pass while validating a code path that won't fire in production — false confidence.
This matches the open question you flagged in the PR description ("verify the exact Health JSON shape on a live Podman 6 host"). Based on the Podman source, I don't believe that shape exists in the ps JSON on any version, including 6.
If the goal is genuinely to avoid the per-container inspect, the health signal would have to be parsed out of the Status string (which the health column derives from) rather than a nonexistent Health field — or the health of many containers batched via a single podman ps --format '{{.Names}} {{.Status}}' / podman healthcheck call. Worth confirming on a live Podman 6 host before merging, since if the field is absent this PR adds surface area without the optimization.
Minor
reporting.go: the trailing if healthStatus == "" { healthStatus = "none" } is dead in practice — GetHealthStatus never returns "" (it maps empty/<no value> to "none"), and the c.State == "running" branch already re-checks emptiness. Harmless, but it's belt-and-suspenders.
normalizeHealthString only whitelists healthy/unhealthy/starting/none and maps anything else to "" (→ inspect fallback), whereas the inspect path (GetHealthStatus) returns whatever podman prints verbatim. So the two paths normalize differently. Fine given the fallback, just noting the asymmetry.
Verdict
The code is safe to merge in the sense that it can't regress anything. But I'd hold it until the Health-in-ps-json assumption is confirmed on a real Podman 6 host — from the source it looks like the field never appears, which would make the whole change (plus its tests) inert.
Addressed the review comment: verified Podman v6 source has no Health field in ps JSON, then amended the PR to parse the real Status field populated from HealthCheckStatus. Also updated tests to match real Status output and kept missing Status as an inspect fallback.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed
Why
Podman v6 source shows ListContainerBatch populates the list response Status from HealthCheckStatus. Using that value avoids extra per-container inspect calls when Podman includes Status, while preserving older or unexpected output through the existing inspect fallback path.
Review follow-up
The original PR parsed a nonexistent Health field. Review feedback correctly flagged that as inert. This update switches the implementation and tests to the actual Status field from Podman list output.
Validation