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

podman inspect add State.Health field for docker compat #11654

Merged
merged 1 commit into from
Sep 23, 2021

Conversation

Luap99
Copy link
Member

@Luap99 Luap99 commented Sep 20, 2021

podman inspect shows the healthcheck status in .State.Healthcheck,
docker uses .State.Health. To make sure docker scripts work we
should add the Health key. Because we do not want to display both keys
by default we only use the new Health key. This is a breaking change
for podman users but matches what docker does. To provide some form of
compatibility users can still use --format {{.State.Healthcheck}}. It
is just not shown by default.

Fixes #11645

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 20, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Luap99

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 20, 2021
@Luap99 Luap99 force-pushed the health-docker branch 2 times, most recently from ca7b6fa to c5de0c8 Compare September 20, 2021 11:53
@rhatdan
Copy link
Member

rhatdan commented Sep 20, 2021

LGTM

@mheon
Copy link
Member

mheon commented Sep 20, 2021

I'm a little iffy about this. HCs are potentially a lot of output, printing them twice seems excessive.

@Luap99
Copy link
Member Author

Luap99 commented Sep 20, 2021

I'm a little iffy about this. HCs are potentially a lot of output, printing them twice seems excessive.

What do you suggest instead, renaming the field to match docker will break existing podman users.

@mheon
Copy link
Member

mheon commented Sep 20, 2021

APIv2 should already be working, because it directly used the Docker structs, so we don't need to worry about that side of things.

Just spitballing here, but we could detect if we have a terminal attached (and thus have a user looking at the output), and not add the Docker-compat field? Any use of podman inspect that requires exact Docker compat is likely either going through the API or being scripted.

Alternatively, we could make this a breaking change for 4.0.

@jwhonce
Copy link
Member

jwhonce commented Sep 20, 2021

@mheon the compatible handler does have code to resolve the naming and type differences. See

if l.HasHealthCheck() && state.Status != "created" {

@TomSweeneyRedHat
Copy link
Member

I'd vote breaking change for v4.0

@TomSweeneyRedHat
Copy link
Member

and a few red tests

@Luap99
Copy link
Member Author

Luap99 commented Sep 21, 2021

I went with a different approach, I really do not want to break podman users.
I added the Health key as method, this will make --format {{.State.Health}} work correctly otherwise this field will not be included.

@rhatdan
Copy link
Member

rhatdan commented Sep 21, 2021

LGTM

@rhatdan
Copy link
Member

rhatdan commented Sep 21, 2021

Since this is Podman 4.0, why not reverse this selection, so HealthCheck will still work with Format, but our output matches Docker.

@abitrolly
Copy link
Contributor

It would be better if the output matches Docker.

@umohnani8
Copy link
Member

LGTM
@mheon PTAL for final review

@mheon
Copy link
Member

mheon commented Sep 21, 2021

LGTM on my end

@Luap99
Copy link
Member Author

Luap99 commented Sep 22, 2021

I swapped the fields as @rhatdan suggested. We now match docker by default but users can still use --format {{.State.Healthcheck}}.
This is a breaking change, therefore this commit should not be backported.

@Luap99 Luap99 force-pushed the health-docker branch 2 times, most recently from 214592c to 0626e7a Compare September 22, 2021 20:53
podman inspect shows the healthcheck status in `.State.Healthcheck`,
docker uses `.State.Health`. To make sure docker scripts work we
should add the `Health` key. Because we do not want to display both keys
by default we only use the new `Health` key. This is a breaking change
for podman users but matches what docker does. To provide some form of
compatibility users can still use `--format {{.State.Healthcheck}}`. IT
is just not shown by default.

Fixes containers#11645

Signed-off-by: Paul Holzinger <[email protected]>
@jwhonce
Copy link
Member

jwhonce commented Sep 23, 2021

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 23, 2021
@openshift-merge-robot openshift-merge-robot merged commit e8fc990 into containers:main Sep 23, 2021
@Luap99 Luap99 added the 4.0 label Sep 24, 2021
@Luap99 Luap99 deleted the health-docker branch September 24, 2021 07:33
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 22, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

podman inspect returns State.Healthcheck when docker uses State.Health
8 participants