-
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
podman wait accept args > 1 #7630
Conversation
LGTM |
9c704a5
to
3ff60b2
Compare
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.
Before we go through with this: do we really, really want this? I see this as a huge can of worms: how exactly is multi-wait defined? Is it AND or OR? Right now it looks like it's sequential AND:
$ ./bin/podman run -d --name foo1 ... sleep 90
$ !!:gs/90/5/
$ ./bin/podman wait foo1 foo2 (hangs for ~90 seconds with no output)
0
0
If this is truly the desired behavior -- and it might not be, because I for one was expecting "wait for any one of these" -- I believe it should be clearly documented in the man page.
I would encourage is all to step back and maybe consider saying "podman wait takes exactly one argument".
@edsantiago According to the man page this is how it should work. https://github.com/containers/podman/blob/master/docs/source/markdown/podman-wait.1.md#description |
@Luap99 thanks but that's still an unhelpful description: it does not specify how the exit codes are emitted (by command-line order? by order of which exited first?) Whether or not this approach is desired, that whole description needs a lot of work. |
@edsantiago A quick test shows it will print in the same order as the given containers and will only print the exit codes after all containers exited.
|
Understood; that was my experience as well. My point is, this should be clearly and unambiguously documented in the man page. |
@edsantiago @rhatdan PTAL |
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.
Orthographic nit. Aside from that, if this truly is the desired behavior for wait
, it lgtm.
Signed-off-by: Paul Holzinger <[email protected]>
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: edsantiago, 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 |
/lgtm |
PR containers#7633 disallowed "-l" (--latest) with container args. PR containers#7630 made changes to the "podman wait" command. The error message it issues is inconsistent (and incompatible) with the one required by the new BATS --help test. Fix that. This is entirely my fault. I was aware of containers#7630, and I was careful to check the output message format, but I was not careful enough (I trusted my eyes, not my code). Signed-off-by: Ed Santiago <[email protected]>
Fixes #7627