-
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
e2e: fix one of the many log flakes #18959
e2e: fix one of the many log flakes #18959
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
@@ -468,11 +468,11 @@ var _ = Describe("Podman logs", func() { | |||
testPod.WaitWithDefaultTimeout() | |||
Expect(testPod).To(Exit(0)) | |||
|
|||
log1 := podmanTest.Podman([]string{"run", "--log-driver", log, "--name", containerName1, "-d", "--pod", podName, BB, "/bin/sh", "-c", "echo log1"}) |
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.
the test above has the same bug, and while you are at it could you please remove the unnecessary "/bin/sh", "-c"
and just exec "echo", "log1"
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.
Good catches, thank you.
A few tests were doing "podman run -d" + "podman logs". This is racy. Remove the unnecessary "-d". And, as long as we're mucking around in here: - remove the "-t" from the 800-lines test, so we get clean output without ^Ms - remove unnecessary "sh", "-c" from simple echo commands - add actual error-message checks to two places that were only checking exit status Resolves one (not all) of the flakes tracked in containers#18501 Signed-off-by: Ed Santiago <[email protected]>
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
/hold
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: edsantiago, Luap99, 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 |
/hold cancel |
/hold |
(CI still running) |
Artifacts job is dead. Crossing fingers that this can unstick things /hold cancel |
A few tests were doing "podman run -d" + "podman logs".
This is racy. Remove the unnecessary "-d".
And, as long as we're mucking around in here:
Resolves one (not all) of the flakes tracked in #18501
Signed-off-by: Ed Santiago [email protected]