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.

Sign up for GitHub

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

Jump to bottom

Fixed Healthcheck formatting, string to []string #11048

Merged
merged 2 commits into from
Jul 29, 2021
Merged

Fixed Healthcheck formatting, string to []string #11048

merged 2 commits into from
Jul 29, 2021

Conversation

cdoern
Copy link
Contributor

@cdoern cdoern commented Jul 26, 2021 •
edited
Loading

Compat healthcheck tests are of the format []string but podman's were of
the format string. Converted podman's to []string at the specgen level since it has the same effect
and removed the incorrect parsing of compat healthchecks.

fixes #10617

Signed-off-by: cdoern [email protected]

@cdoern cdoern force-pushed the heatlhCheckCompat branch from e656a2e to df0ae5a Compare July 26, 2021 13:49
@rhatdan
Copy link
Member

rhatdan commented Jul 26, 2021

LGTM
But tests are failing.

@cdoern cdoern force-pushed the heatlhCheckCompat branch 4 times, most recently from 6fe7630 to 2295e62 Compare July 26, 2021 17:59
jwhonce
jwhonce reviewed Jul 26, 2021
View reviewed changes
test/e2e/run_test.go Outdated Show resolved Hide resolved
cmd/podman/common/specgen.go Outdated Show resolved Hide resolved
@cdoern
Copy link
Contributor Author

cdoern commented Jul 26, 2021 •
edited
Loading

@jwhonce @rhatdan I am realizing now that this would be much simpler and compat if we required users (like docker) to specify CMD-SHELL (one command to read in as a whole) vs CMD (arg1, arg2 etc). Would this be something you would like me to add? I have a few fixes ready to push for this right now but they are contingent on that.

EDIT: I was able to basically recreate the logic of cmd vs cmd-shell vs none in our specgen code so there is no need for differentiation in the CLI

@cdoern cdoern force-pushed the heatlhCheckCompat branch 2 times, most recently from 0351ad6 to 4bbf5c6 Compare July 26, 2021 19:33
@cdoern
Fixed Healthcheck formatting, string to []string
fd1f57b 
Compat healthcheck tests are of the format []string but podman's were of
the format string. Converted podman's to []string at the specgen level since it has the same effect
and removed the incorrect parsing of compat healthchecks.

fixes containers#10617

Signed-off-by: cdoern <[email protected]>
@cdoern cdoern force-pushed the heatlhCheckCompat branch from 4bbf5c6 to fd1f57b Compare July 26, 2021 20:59
@TomSweeneyRedHat
Copy link
Member

LGTM
and happy green test buttons

vrothberg
vrothberg reviewed Jul 27, 2021
View reviewed changes
Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

/lgtm
/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 27, 2021
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 27, 2021
@Luap99
Copy link
Member

Luap99 commented Jul 27, 2021

Please do not merge until #11048 (comment) is resolved.

@Luap99 Luap99 removed the lgtm Indicates that a PR is ready to be merged. label Jul 27, 2021
@cdoern cdoern force-pushed the heatlhCheckCompat branch from 612a15c to 9ac9fed Compare July 27, 2021 14:22
Luap99
Luap99 reviewed Jul 27, 2021
View reviewed changes
cmd/podman/common/specgen.go Outdated Show resolved Hide resolved
cmd/podman/common/specgen.go Outdated Show resolved Hide resolved
@cdoern cdoern force-pushed the heatlhCheckCompat branch 3 times, most recently from 1756c17 to 17dc466 Compare July 27, 2021 21:11
mheon
mheon reviewed Jul 27, 2021
View reviewed changes
cmd/podman/common/specgen.go Outdated Show resolved Hide resolved
mheon
mheon reviewed Jul 27, 2021
View reviewed changes
cmd/podman/common/specgen.go Outdated Show resolved Hide resolved
mheon
mheon reviewed Jul 27, 2021
View reviewed changes
cmd/podman/common/specgen.go Outdated Show resolved Hide resolved
@cdoern cdoern force-pushed the heatlhCheckCompat branch from 17dc466 to fb89f7d Compare July 28, 2021 00:45
mheon
mheon reviewed Jul 28, 2021
View reviewed changes
cmd/podman/common/specgen.go Outdated Show resolved Hide resolved
mheon
mheon reviewed Jul 28, 2021
View reviewed changes
cmd/podman/common/specgen.go Outdated Show resolved Hide resolved
@cdoern
Fixed Healthcheck formatting, string to []string
a9f6592 
Compat healthcheck tests are of the format []string but podman's were of
the format string. Converted podman's to []string at the specgen level since it has the same effect
and removed the incorrect parsing of compat healthchecks.

fixes containers#10617

Signed-off-by: cdoern <[email protected]>
@cdoern cdoern force-pushed the heatlhCheckCompat branch from fb89f7d to a9f6592 Compare July 28, 2021 13:53
@TomSweeneyRedHat
Copy link
Member

LGTM
and happy green test buttons
@Luap99 can you do the merge honors if your concerns have been addressed?

@Luap99
Copy link
Member

Luap99 commented Jul 29, 2021

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 29, 2021
@Luap99
Copy link
Member

Luap99 commented Jul 29, 2021

/approve
/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 29, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 29, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cdoern, 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 Jul 29, 2021
@openshift-merge-robot openshift-merge-robot merged commit 1ec1c85 into containers:main Jul 29, 2021
@edsantiago edsantiago mentioned this pull request Jul 29, 2021
Merged
@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.
Reviewers

@Luap99 Luap99 Luap99 left review comments

@vrothberg vrothberg vrothberg left review comments

@mheon mheon mheon left review comments

@jwhonce jwhonce jwhonce left review comments

Assignees

@vrothberg vrothberg

@Luap99 Luap99

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

podman docker-compose healthchecks issue
8 participants
@cdoern @rhatdan @TomSweeneyRedHat @Luap99 @jwhonce @mheon @vrothberg @openshift-merge-robot