-
Notifications
You must be signed in to change notification settings - Fork 2.9k
remote: don't print bogus error when starting container attached #25966
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
Conversation
This looks like debug leftover, in any case this is not an error so simply remove the line. Fixes containers#25965 Signed-off-by: Paul Holzinger <[email protected]>
This comment was marked as off-topic.
This comment was marked as off-topic.
|
/hold Please don't add "/lgtm" right away, we generally require two reviews from maintainers first. |
This comment was marked as outdated.
This comment was marked as outdated.
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, Thank you. I think --rm doesn't work for podman create, but that's not the subject of this PR.
/approve
|
can it be cherry-picked to 5.5 branch once approved/merged ? thanks |
|
@benoitf Please do not rerun blindly tests, the log clearly shows my new tests failing which means (looking at the high number of failures) it is flaky and thus this cannot be merged like this. |
Like podman run --rm, start --attach must also ensure the contianer is removed before it exist. Otherwise there is a race where the container still exist after the command exits, because removal would only happen by the cleanup process in the background. Signed-off-by: Paul Holzinger <[email protected]>
Honny1
left a comment
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.
Nice fix of tests. I think test: Podman logs [It] since time 2017-08-07: journald is flaky.
Yes, every podman logs test is flaky in some way. journald is just super unreliable, #24220 |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Honny1, 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 |
|
/cherry-pick v5.5 |
|
@baude: once the present PR merges, I will cherry-pick it on top of In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@baude: new pull request created: #25976 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
This looks like debug leftover, in any case this is not an error so simply remove the line.
Fixes #25965
Does this PR introduce a user-facing change?