Edited compat handling code for containers/json status and added python tests #10610

cdoern · 2021-06-09T13:24:58Z

Health Check is not handled in the compat LibpodToContainerJSON function within containers.go. Added parsing and handling for this status check and also edited some of the list container filter code.

fixes #10457

pkg/api/handlers/compat/containers.go

test/apiv2/python/rest_api/test_v2_0_0_container.py

mheon · 2021-06-10T02:12:33Z

Does this actually fix #10457 - I don't see any HC-related changes, just filter ones?

cdoern · 2021-06-10T02:51:04Z

@mheon I am pretty sure the issue is originating with the lack of handling for both status and health filters in the compat code upon inspect. I am planning on taking another look in the morning specifically at the Health filter to see if that is also just not handled.

Luap99 · 2021-06-10T08:39:59Z

@cdoern I just noticed, could please include the PR description in the commit message. Also the fixes #123 should be in the commit message. This makes it easier to track the commits in the local git history and the sign-off-by line should include your real name not the nick.

Luap99

You say this fixes #10457 but I don't see why this would fix it. There is no test for #10457

pkg/api/handlers/compat/containers.go

cdoern · 2021-06-10T12:10:56Z

@cdoern I just noticed, could please include the PR description in the commit message. Also the fixes #123 should be in the commit message. This makes it easier to track the commits in the local git history and the sign-off-by line should include your real name not the nick.

will do, sorry for the formatting issues

cdoern · 2021-06-10T12:14:09Z

You say this fixes #10457 but I don't see why this would fix it. There is no test for #10457

Added further fixing for health check specifically in most recent commit.

pkg/api/handlers/compat/containers.go

TomSweeneyRedHat · 2021-06-12T17:16:16Z

@edsantiago can you take a peak at the failing test here? It's failing on a bad release version and I don't see any changes here that should have triggered that.

test/apiv2/python/rest_api/test_v2_0_0_container.py

edsantiago · 2021-06-12T17:24:31Z

This test should never run. I have to assume that someone manually pressed the 'Trigger' button. Whoever did that, live and learn, don't do it again.

You can ignore the test result. It does not gate merging.

cdoern · 2021-06-14T12:35:41Z

This test should never run. I have to assume that someone manually pressed the 'Trigger' button. Whoever did that, live and learn, don't do it again.

You can ignore the test result. It does not gate merging.

that was me sorry, one test failed due to a flake and cancelled all of the others so I must've accidentally restarted that one.

edsantiago · 2021-06-14T12:38:03Z

No worries! To restart a failed test, just press the 'Re-run' link in the left-hand panel. You do not need to restart any others: once the flaked test succeeds, the others will eventually run even though the @#$% UI says "Canceled". It's completely anti-intuitive, just one of those things we have to get used to.

TomSweeneyRedHat · 2021-06-23T00:16:25Z

LGTM

rhatdan · 2021-06-23T14:17:56Z

@mheon @Luap99 PTAL

mheon · 2021-06-23T15:12:52Z

pkg/api/handlers/compat/containers.go

+	state.Health.Status = inspect.State.Healthcheck.Status
+	state.Health.FailingStreak = inspect.State.Healthcheck.FailingStreak
+
+	//item := define.HealthCheckLog{}


Can you trim this bit?

mheon · 2021-06-23T15:15:17Z

pkg/api/handlers/compat/containers.go

+
+	//item := define.HealthCheckLog{}
+
+	var res *types.HealthcheckResult = &types.HealthcheckResult{}


This looks a little dubious - why is it being created only once? I think every time the for loop below runs, you're overwriting the contents, and appending the same entry to the array again?

I would move this inside the loop for safety

rhatdan · 2021-06-23T17:10:13Z

pkg/api/handlers/compat/containers.go

+	log := inspect.State.Healthcheck.Log
+
+	for _, item := range log {
+		var res *types.HealthcheckResult = &types.HealthcheckResult{}


Can this be stated as

res:=&types.HealthcheckResult{}

yep, pushing that change now.

Added parsing and handling for the healthCheck status within containers.go. Also modified tests fixes containers#10457 Signed-off-by: cdoern <[email protected]>

rhatdan · 2021-06-23T18:44:14Z

/approve
/lgtm

openshift-ci · 2021-06-23T18:44:21Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cdoern, rhatdan

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:

~~OWNERS~~ [rhatdan]

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

cdoern force-pushed the healthCheck branch 3 times, most recently from a21fcc3 to 2e61416 Compare June 9, 2021 14:36

Luap99 reviewed Jun 9, 2021

View reviewed changes

pkg/api/handlers/compat/containers.go Outdated Show resolved Hide resolved

mheon reviewed Jun 9, 2021

View reviewed changes

test/apiv2/python/rest_api/test_v2_0_0_container.py Outdated Show resolved Hide resolved

cdoern force-pushed the healthCheck branch from 2e61416 to f2a8875 Compare June 9, 2021 16:22

cdoern mentioned this pull request Jun 9, 2021

podman docker-compose healthchecks issue #10617

Closed

cdoern requested a review from Luap99 June 9, 2021 18:32

cdoern changed the title ~~Added Filter query param for containers/json as well as python tests~~ Edited compat handling code for containers/json status and added python tests Jun 9, 2021

Luap99 reviewed Jun 10, 2021

View reviewed changes

pkg/api/handlers/compat/containers.go Outdated Show resolved Hide resolved

cdoern force-pushed the healthCheck branch from f2a8875 to cf5db0f Compare June 10, 2021 12:09

cdoern force-pushed the healthCheck branch from cf5db0f to 1a6a01a Compare June 10, 2021 12:13

cdoern force-pushed the healthCheck branch 2 times, most recently from 6150de2 to 5057b12 Compare June 10, 2021 12:42

mheon reviewed Jun 10, 2021

View reviewed changes

pkg/api/handlers/compat/containers.go Outdated Show resolved Hide resolved

mheon reviewed Jun 10, 2021

View reviewed changes

pkg/api/handlers/compat/containers.go Outdated Show resolved Hide resolved

cdoern force-pushed the healthCheck branch 6 times, most recently from 9f8d37c to b54f151 Compare June 10, 2021 16:40

cdoern requested a review from mheon June 11, 2021 12:38

TomSweeneyRedHat reviewed Jun 12, 2021

View reviewed changes

test/apiv2/python/rest_api/test_v2_0_0_container.py Show resolved Hide resolved

cdoern requested a review from TomSweeneyRedHat June 18, 2021 13:39

mheon reviewed Jun 23, 2021

View reviewed changes

cdoern force-pushed the healthCheck branch from b54f151 to fdb6eaa Compare June 23, 2021 15:23

rhatdan reviewed Jun 23, 2021

View reviewed changes

Health Check is not handled in the compat LibpodToContainerJSON

6ecdf4c

Added parsing and handling for the healthCheck status within containers.go. Also modified tests fixes containers#10457 Signed-off-by: cdoern <[email protected]>

cdoern force-pushed the healthCheck branch from fdb6eaa to 6ecdf4c Compare June 23, 2021 17:16

openshift-ci bot assigned rhatdan Jun 23, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 23, 2021

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 23, 2021

openshift-merge-robot merged commit b0a3ac3 into containers:master Jun 23, 2021

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 23, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Edited compat handling code for containers/json status and added python tests #10610

Edited compat handling code for containers/json status and added python tests #10610

cdoern commented Jun 9, 2021 •

edited

Loading

mheon commented Jun 10, 2021

cdoern commented Jun 10, 2021

Luap99 commented Jun 10, 2021

Luap99 left a comment

cdoern commented Jun 10, 2021

cdoern commented Jun 10, 2021

TomSweeneyRedHat commented Jun 12, 2021

edsantiago commented Jun 12, 2021

cdoern commented Jun 14, 2021

edsantiago commented Jun 14, 2021

TomSweeneyRedHat commented Jun 23, 2021

rhatdan commented Jun 23, 2021

mheon Jun 23, 2021

mheon Jun 23, 2021

rhatdan Jun 23, 2021

cdoern Jun 23, 2021

rhatdan commented Jun 23, 2021

openshift-ci bot commented Jun 23, 2021


		//item := define.HealthCheckLog{}

		var res *types.HealthcheckResult = &types.HealthcheckResult{}

Edited compat handling code for containers/json status and added python tests #10610

Edited compat handling code for containers/json status and added python tests #10610

Conversation

cdoern commented Jun 9, 2021 • edited Loading

mheon commented Jun 10, 2021

cdoern commented Jun 10, 2021

Luap99 commented Jun 10, 2021

Luap99 left a comment

Choose a reason for hiding this comment

cdoern commented Jun 10, 2021

cdoern commented Jun 10, 2021

TomSweeneyRedHat commented Jun 12, 2021

edsantiago commented Jun 12, 2021

cdoern commented Jun 14, 2021

edsantiago commented Jun 14, 2021

TomSweeneyRedHat commented Jun 23, 2021

rhatdan commented Jun 23, 2021

mheon Jun 23, 2021

Choose a reason for hiding this comment

mheon Jun 23, 2021

Choose a reason for hiding this comment

rhatdan Jun 23, 2021

Choose a reason for hiding this comment

cdoern Jun 23, 2021

Choose a reason for hiding this comment

rhatdan commented Jun 23, 2021

openshift-ci bot commented Jun 23, 2021

cdoern commented Jun 9, 2021 •

edited

Loading