-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
kube play: always pull when both imagePullPolicy and tag are missing #21493
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mporrato, vrothberg 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 |
test/e2e/play_kube_test.go
Outdated
inspect := podmanTest.Podman([]string{"inspect", BB}) | ||
inspect.WaitWithDefaultTimeout() | ||
oldBBinspect := inspect.InspectImageJSON() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is missing an exit code check, needs to have an Expect(inspect).Should(ExitCleanly())
test/e2e/play_kube_test.go
Outdated
|
||
kube := podmanTest.Podman([]string{"kube", "play", kubeYaml}) | ||
kube.WaitWithDefaultTimeout() | ||
// Cannot ExitCleanly() because pull output goes to stderr |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you should check the stderr output here as it should contain the pull output, i.e. Copying blob
Align the behaviour of `podman kube play file.yaml` to Kubernetes' by forcing an image pull when `imagePullPolicy` is omitted and the container image does not specify a tag. Signed-off-by: Maurizio Porrato <[email protected]>
Cockpit tests failed for commit cb81da9. @martinpitt, @jelly, @mvollmer please check. |
There is the podman build hang again on rawhide which I've seen several times in the last few weeks. The subsequent tests then also fail as there seems to be an unkillable I'll add this to the TODO list to see if I can reproduce it locally with running this in a loop, but I won't get to it much next week as I'll be on a sprint. In the meantime, you can either retry or ignore that failure (with my blessing 😁 ) |
I locally ran that test a few hundred times in two parallel VMs, with the same kernel version, and didn't manage to reproduce the lockup. That's either unfortunate timing or some quirk/bug on the EC2 (where Testing Farm instances run). Meh.. |
@martinpitt follow #21504 for the hang, so far we were not able to reproduce either. |
A friendly reminder that this PR had no activity for 30 days. |
@Luap99 anything needed on my side to move this forward? |
/lgtm |
a1a5bd3
into
containers:main
Align the behaviour of
podman kube play file.yaml
to Kubernetes' by forcing an image pull whenimagePullPolicy
is omitted and the container image does not specify a tag.Fixes #21211
Does this PR introduce a user-facing change?