Fix Missing HostConfig field from API response#28714
Conversation
github-actions
Bot
added
the
kind/api-change
Honny1
left a comment
Honny1
left a comment
There was a problem hiding this comment.
Thanks for the PR. Can you please add APIv2 tests?
| }{ | ||
| NetworkMode: "host", | ||
| // TODO: add annotations here for >= v1.46 | ||
| NetworkMode: inspect.HostConfig.NetworkMode, |
|
@Honny1 I have added the nil check and the test under the already existing compat containers/json test invocation. I had to run the patches against 5.8.2 for tests to pass, main is currently in a broken state. |
|
It was written in the CI logs:
Just pushed the fixed version |
Honny1
left a comment
Honny1
left a comment
There was a problem hiding this comment.
LGTM, can you please stash your commits?
- Populate the Docker-compatible container summary HostConfig from the container inspect data - Remove the unused ContainerCreateConfig wrapper from the compat handler - Add APIv2 test Signed-off-by: Christopher Bii <christopherbii@hyub.org>
- Top process killed with sigkill would not exit with the expected 0 code Signed-off-by: Christopher Bii <christopherbii@hyub.org>
|
I am not sure if or how this test has been passing, but the container runs a top process and simply waits for timeout before sending a SIGABRT. This should never exit cleanly. |
| c1 := "ctr1" | ||
| c1s := podmanTest.Podman([]string{"run", "-d", "--expose", "22/tcp", "-p", "8080:80/tcp", "--name", c1, ALPINE, "top"}) | ||
| c1s.WaitWithDefaultTimeout() | ||
| Expect(c1s).Should(ExitCleanly()) |
There was a problem hiding this comment.
Why was this changed? This is not right, the exit status we're checking here is podman run ...'s, not top's inside the container.
3 participants
No description provided.