vmexport: Reject volume exports when no output file is specified

[alromeros]

What this PR does / why we need it:

When no output is specified, all vmexport downloads are dumped into stdout. This Pull Request improves the error handling to reject volume downloads when no output file is specified to avoid unexpected binary dumps.

Example before fix:

# virtctl vmexport download vm --pvc=test
�����������������������������������������������������������������������������������_/���y��<u�'��&
��
$q��D5�$����{�H�

...

After fix:

# virtctl vmexport download vm --pvc=test
Warning: Binary output can mess up your terminal. Use '--output -' to output into stdout anyway or consider '--output <FILE>' to save to a file.

Manifest downloads are still able to dump into stdout without specifying output.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes # https://issues.redhat.com/browse/CNV-36271

Special notes for your reviewer:

Release note:

Bugfix: Reject volume exports when no output is specified

[alicefr]

The option sound reasonable to me
/approve

[kubevirt-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alicefr

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 [alicefr]

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

[alromeros]

/retest-required

[awels]

/hold
So this is on purpose, this allows you to pipe the output into another command. This is actually a feature not a bug.

[ShellyKa13]

@awels @alromeros how about we make the output a must when doing download (as Alvaro did to return an error) but make stdout a valid output for piping purposes and so also change the error msg to "must set output value" and maybe give in the examples the stout option so people will know its possible.

[alicefr]

@awels @alromeros how about we make the output a must when doing download (as Alvaro did to return an error) but make stdout a valid output for piping purposes and so also change the error msg to "must set output value" and maybe give in the examples the stout option so people will know its possible.

I agree with Shelly. We should not mess the terminal if somebody is missing the flag by accident.

[alromeros]

Updated this to match curl's behavior:

# virtctl vmexport download vm --pvc=test
Warning: Binary output can mess up your terminal. Use '--output -' to output into stdout anyway or consider '--output <FILE>' to save to a file.

[alromeros]

/retest-required

[alromeros]

/retest-required

[awels]

/lgtm

[alromeros]

/unhold

[ShellyKa13]

you don't allow outputFile to be "" so you dont need to check it right?
what about someone explicitly doing --outputFile=stdout? we dont want that to go to stdout? is it not acceptable?

[alromeros]

Yeah we allow empty output files for manifest downloads. Also, I think it's good to be consistent and allow using whatever file name (even stdout).

[kubevirt-commenter-bot]

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

[kubevirt-bot]

@alromeros: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kubevirt-e2e-arm64	cc1db17	link	false	/test pull-kubevirt-e2e-arm64
pull-kubevirt-e2e-k8s-1.28-sig-compute	cc1db17	link	unknown	/test pull-kubevirt-e2e-k8s-1.28-sig-compute

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/test-infra repository. I understand the commands that are listed here.