Feature/#21256 extend cidfile support #25512
Conversation
Signed-off-by: Martin Glatzle <[email protected]>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: findesgh The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
findesgh
force-pushed
the
feature/#21256-extend-cidfile-support
branch
4 times, most recently
from
7fc85ff to
31cd56e
Compare
cmd/podman/containers/exec.go
Outdated
| @@ -24,7 +24,7 @@ var ( | |||
| execDescription = `Execute the specified command inside a running container. | |||
| ` | |||
| execCommand = &cobra.Command{ | |||
| Use: "exec [options] CONTAINER COMMAND [ARG...]", | |||
| Use: "exec [options] [CONTAINER] COMMAND [ARG...]", | |||
There was a problem hiding this comment.
This breaks the cli help test, I suggest you do not change it.
Neither is right since it is not really optional on its own, only when --latest or --cidfile is given then no container name is set. Since we have no way to show that here easily I suggest you just leave it as.
There was a problem hiding this comment.
Yeah, you're right. Leaving it as is is also in line with other commands that accept --cidfile. Will change this back also in the docs.
| @@ -97,6 +102,7 @@ func execFlags(cmd *cobra.Command) { | |||
|
|
|||
| if registry.IsRemote() { | |||
| _ = flags.MarkHidden("preserve-fds") | |||
| _ = flags.MarkHidden("cidfile") | |||
There was a problem hiding this comment.
That is not true, none of the other commands seem to have this limit, the file is read on the client so this works fine.
There was a problem hiding this comment.
I have to admit that I don't understand why, but it seems to me that pause, restart, unpause and stop do this as well. rm and kill, on the other hand, don't.
I would tend to agree with you that there is no reason to hide the cidfile flag, but I wanted to err on the side of caution.
There was a problem hiding this comment.
The behaviour should probably be consistent across all commands that accept the cidfile flag?
cmd/podman/containers/exec.go
Outdated
| var command []string | ||
|
|
||
| if len(args) == 0 && !latestSpecified && !execCidFileProvided { | ||
| return "", []string{}, errors.New("exec requires the name or ID of a container or the --latest or --cidfile flag") |
There was a problem hiding this comment.
you can just return nil instead if []string{} which needs an extra allocation for no reason.
same in the error branches below.
Thanks for the feedback, will squash. |
findesgh
force-pushed
the
feature/#21256-extend-cidfile-support
branch
from
31cd56e to
b6ca843
Compare
|
One more thing: |
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
Fixes: containers#21256 Signed-off-by: Martin Glatzle <[email protected]>
findesgh
force-pushed
the
feature/#21256-extend-cidfile-support
branch
from
b6ca843 to
027fecb
Compare
| command = args[1:] | ||
| nameOrID = strings.TrimPrefix(args[0], "/") | ||
| } else { | ||
| content, err := os.ReadFile(execCidFile) |
There was a problem hiding this comment.
Did I miss checks to make sure the file provided is secure? I would expect an absPath kind of check at least.
This PR adds support for the
--cidfile-flag topodman exec. See also #21256.Does this PR introduce a user-facing change?
Yes: