Added the "--out" parameter and fixed an issue with "--noout" which prevented stdout from being written to. #18657

arizvisa · 2023-05-22T20:15:27Z

This adds an additional parameter, "--out", which can be used to capture the output of any part of podman. This is similar to the original "--noout", but instead of redirecting stdout to "/dev/null" it allows the user to choose the file that is being written to. Thus using the parameter as in podman --out /dev/null ... corresponds to the same logic. The original "--noout" parameter was left alone as I expect someone is already depending on this functionality. The original "--noout" parameter had a bug in that "/dev/null" was not being opened as writable. This results in an EBADF error being returned everytime the fd being written to gets used.

The PR adds the additional parameter using the same logic as "--noout", fixes the prior-mentioned issue with "--noout", and adds testcases for each of them. To perform the test for the "--noout" error, the only symptom I could identify (without running strace) was that golang would complain to stderr if it couldn't write to stdout when running podman without any parameters. Hence the test is written against this symptom.

This fixes issue #18120.

Does this PR introduce a user-facing change?

All Podman commands now support a new option, `--out`, that allows redirection or suppression of STDOUT.

… by various commands Commands like podman-create(1), podman-run(1), podman-inspect(1), podman-ps(1) will emit formatted output upon success. This allows the output from commands to be emitted directly to a file and can supersede the --noout parameter by using /dev/null. An issue with --noout was also remedied. This closes issue containers#18120. Signed-off-by: Ali Rizvi-Santiago <[email protected]>

edsantiago

Thank you for your PR! A few quick suggestions and requests from a very quick pass. Also, please collapse your commits.

test/system/200-pod.bats

Luap99

I am not a huge fan of these kind of options but I won't block over it. Is there a reason to do stdout only but not for stderr, seems inconsistent?

Please squash the commits into one and where say add unit test it should really say system test as these are not unit tests.

cmd/podman/root.go

test/system/500-networking.bats

edsantiago

Thanks again. And please remember to squash your commits.

test/system/200-pod.bats

docs/source/markdown/podman.1.md

arizvisa · 2023-05-23T21:12:12Z

Awesome. All the tests passed, re-requesting review. (Do at your leisure).

edsantiago · 2023-05-24T15:00:53Z

@arizvisa could you please squash your commits? And please remove unnecessary tests. There is no need for so many tests. (I can't believe I'm saying that!)

… requires rootless. Signed-off-by: Ali Rizvi-Santiago <[email protected]>

arizvisa · 2023-05-31T02:00:01Z

squashed and included as many suggestions as you gave input on.

Luap99

I wonder if we should just hide --noout now that you can use --out=/dev/null and just keep it for backwards compat.
i.e. add lFlags.MatkHidden("noout") and remove the docs for this option.

…Luap99

…Luap99 in containers#18657. Signed-off-by: Ali Rizvi-Santiago <[email protected]>

arizvisa · 2023-05-31T15:12:00Z

I wonder if we should just hide --noout now that you can use --out=/dev/null and just keep it for backwards compat. i.e. add lFlags.MatkHidden("noout") and remove the docs for this option.

I think it makes sense. Added your suggestions until someone suggests otherwise.

Luap99

Thanks, LGTM.

openshift-ci · 2023-06-01T11:47:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: arizvisa, 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:

~~OWNERS~~ [Luap99]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

rhatdan · 2023-06-05T18:31:47Z

/lgtm
Thanks @arizvisa

vans163 · 2023-07-24T18:05:58Z

I am not a huge fan of these kind of options but I won't block over it. Is there a reason to do stdout only but not for stderr, seems inconsistent?

Please squash the commits into one and where say add unit test it should really say system test as these are not unit tests.

Where does STDERR go? Anyway to redirect it as well?

arizvisa · 2023-07-26T00:12:36Z

Where does STDERR go? Anyway to redirect it as well?

@vans163, Which podman command are you trying to capture STDERR from specifically? Just all of podman's STDERR in general or container output?

openshift-ci bot added the release-note label May 22, 2023

arizvisa mentioned this pull request May 22, 2023

Allowing an option to redirect stdout to a file instead of explicitly suppressing it with /dev/null #18120

Closed

edsantiago suggested changes May 22, 2023

View reviewed changes

test/system/200-pod.bats Outdated Show resolved Hide resolved

test/system/200-pod.bats Outdated Show resolved Hide resolved

arizvisa force-pushed the GH-18120 branch 5 times, most recently from 80768b6 to c036081 Compare May 23, 2023 01:41

Luap99 reviewed May 23, 2023

View reviewed changes

cmd/podman/root.go Outdated Show resolved Hide resolved

cmd/podman/root.go Outdated Show resolved Hide resolved

test/system/500-networking.bats Outdated Show resolved Hide resolved

edsantiago suggested changes May 23, 2023

View reviewed changes

test/system/200-pod.bats Outdated Show resolved Hide resolved

docs/source/markdown/podman.1.md Outdated Show resolved Hide resolved

arizvisa force-pushed the GH-18120 branch from bca0a70 to a554e33 Compare May 23, 2023 19:01

arizvisa requested a review from edsantiago May 23, 2023 21:12

arizvisa force-pushed the GH-18120 branch 3 times, most recently from 65ce994 to 55a7d3f Compare May 30, 2023 23:00

arizvisa added a commit to arizvisa/containers-podman that referenced this pull request May 30, 2023

cirrus.6155650586116096: @Luap99's test suggestion for containers#18657…

a976f91

… requires rootless. Signed-off-by: Ali Rizvi-Santiago <[email protected]>

arizvisa requested a review from Luap99 May 31, 2023 02:02

Luap99 reviewed May 31, 2023

View reviewed changes

arizvisa added a commit to arizvisa/containers-podman that referenced this pull request May 31, 2023

pullrequestreview-1452571948: Hide the --noout option as suggested by @…

5d0f9d5

…Luap99 in containers#18657. Signed-off-by: Ali Rizvi-Santiago <[email protected]>

arizvisa force-pushed the GH-18120 branch 3 times, most recently from 7587ec9 to bb2deef Compare May 31, 2023 14:07

Luap99 approved these changes Jun 1, 2023

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 1, 2023

openshift-ci bot assigned rhatdan Jun 5, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 5, 2023

openshift-merge-robot merged commit eec15a1 into containers:main Jun 5, 2023

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Oct 25, 2023

github-actions bot locked as resolved and limited conversation to collaborators Oct 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added the "--out" parameter and fixed an issue with "--noout" which prevented stdout from being written to. #18657

Added the "--out" parameter and fixed an issue with "--noout" which prevented stdout from being written to. #18657

arizvisa commented May 22, 2023 •

edited

Loading

edsantiago left a comment

Luap99 left a comment

edsantiago left a comment

arizvisa commented May 23, 2023 •

edited

Loading

edsantiago commented May 24, 2023

arizvisa commented May 31, 2023

Luap99 left a comment

arizvisa commented May 31, 2023

Luap99 left a comment

openshift-ci bot commented Jun 1, 2023

rhatdan commented Jun 5, 2023

vans163 commented Jul 24, 2023

arizvisa commented Jul 26, 2023

Added the "--out" parameter and fixed an issue with "--noout" which prevented stdout from being written to. #18657

Added the "--out" parameter and fixed an issue with "--noout" which prevented stdout from being written to. #18657

Conversation

arizvisa commented May 22, 2023 • edited Loading

Does this PR introduce a user-facing change?

edsantiago left a comment

Choose a reason for hiding this comment

Luap99 left a comment

Choose a reason for hiding this comment

edsantiago left a comment

Choose a reason for hiding this comment

arizvisa commented May 23, 2023 • edited Loading

edsantiago commented May 24, 2023

arizvisa commented May 31, 2023

Luap99 left a comment

Choose a reason for hiding this comment

arizvisa commented May 31, 2023

Luap99 left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Jun 1, 2023

rhatdan commented Jun 5, 2023

vans163 commented Jul 24, 2023

arizvisa commented Jul 26, 2023

arizvisa commented May 22, 2023 •

edited

Loading

arizvisa commented May 23, 2023 •

edited

Loading