When removing objects specifying --force,podman should exit with 0 #14959

rhatdan · 2022-07-18T14:35:38Z

This Patch will cause podman COMMAND rm --force bogus exit with 0.

This is how Docker works, so Podman should follow this to allow existing
scripts to convert from Docker to Podman.

Signed-off-by: wufan [email protected]
Signed-off-by: Daniel J Walsh [email protected]

Does this PR introduce a user-facing change?

podman COMMAND rm -f BOGUS: Command will now exit with 0 if the only error is OBJECT does not exist.

openshift-ci · 2022-07-18T14:35:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhatdan

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~~ [rhatdan]

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

rhatdan · 2022-07-18T14:36:15Z

Replaces: #14782

Luap99

We should never print an error when exiting with 0.
At least for the commands that have --ignore, --force should just imply ignore to fix this.

Luap99 · 2022-07-18T14:41:38Z

cmd/podman/pods/rm.go

@@ -104,13 +109,17 @@ func removePods(namesOrIDs []string, rmOptions entities.PodRmOptions, printIDs b
 				fmt.Println(r.Id)
 			}
 		} else {
+			fmt.Println("DAN", rmOptions.Force, r.Err, define.ErrNoSuchPod)


Luap99 · 2022-07-18T14:41:48Z

cmd/podman/volumes/rm.go

 		setExitCode(err)
 		return err
 	}
 	for _, r := range responses {
 		if r.Err == nil {
 			fmt.Println(r.Id)
 		} else {
+			if rmOptions.Force && strings.Contains(r.Err.Error(), define.ErrNoSuchVolume.Error()) {
+				fmt.Fprintf(os.Stderr, "Error: %v\n", r.Err)
+				return nil


rhatdan · 2022-07-18T14:51:35Z

In order to match Docker, we need to print the error and exit with 0.

Luap99 · 2022-07-19T13:43:08Z

In order to match Docker, we need to print the error and exit with 0.

I find that behaviour very confusion and I personally would be against matching docker here. Printing an error but still exiting with 0 is just stupid IMHO.

rhatdan · 2022-07-19T19:14:54Z

@crowleydi @WuFan @maknop WDYT?

maknop · 2022-07-20T22:48:53Z

I find that behaviour very confusion and I personally would be against matching docker here. Printing an error but still exiting with 0 is just stupid IMHO.

@Luap99 - Could you elaborate on why you think exiting with 0 and printing an error message would not be a good idea? I don't see why that would be an issue. Error messages will help us better understand what is going on if an error occurs. Additionally, I think matching Docker makes sense. It would be confusing to have the behavior not match for these tools.

Luap99 · 2022-07-21T08:24:35Z

@Luap99 - Could you elaborate on why you think exiting with 0 and printing an error message would not be a good idea? I don't see why that would be an issue. Error messages will help us better understand what is going on if an error occurs. Additionally, I think matching Docker makes sense. It would be confusing to have the behavior not match for these tools.

Because that goes against common sense. If a application tells me (a user) that there is an error but still exits 0 it says two different things at the same time.

Consider the following example from GNU rm(1):

$ rm t; echo $?
rm: cannot remove 't': No such file or directory
1
$ rm -f t; echo $?
0

rhatdan · 2022-07-21T09:46:01Z

Ok, I agree with @Luap99 Will removing printing the error.

rhatdan · 2022-07-25T10:11:33Z

/hold
I screwed up here and overpushed a different PR. I will clean this up later.

rhatdan · 2022-07-26T11:07:40Z

@vrothberg @Luap99 @edsantiago PTAL
This is fixed now.

vrothberg

I know @Luap99 asked not to print the error but if we really aim at Docker compat I fear we do not have choice.

vrothberg · 2022-07-26T11:28:27Z

test/system/500-networking.bats

+    run_podman 1 network rm bogus
+    is "$output" "Error: unable to find network with name or ID bogus: network not found" "Should print error"
+    run_podman network rm --force bogus
+    is "$output" "" "Should print no output"


That is not how Docker behaves:

~ $ sudo docker rm -f bogus Error: No such container: bogus

If we don't care about this compat, users could just use --ignore and we don't need this PR.

But I am not married to printing the error. If others feel strongly about it, feel free to merge. In the general case we should remain compatible where possible - sometimes bug compatible.

If we don't care about this compat, users could just use --ignore and we don't need this PR.

No right no we exit with code > 0. The goal is to exit with 0, so basically --force should imply --ignore IMO.

IIRC we decided --force not imply --ignore when adding --ignore as we were afraid of breaking backwards compat with existing Podman users. But I also think --force should imply it ... just as so many other tools do.

mheon · 2022-07-26T14:40:53Z

We break compat occasionally, when there's good reason to. I'm feeling that this is one of those cases. Printing error messages when a command exited 0 is nonsensical.

Is there a specific usecase we're aware of that actually printing the error messages serves? If we don't have a case where it actively breaks someone, I would be in favor of not printing the error.

edsantiago · 2022-07-26T16:57:53Z

One of the test failures (rmi -a) is because old-podman would say "Error: 2 errors occurred", new-podman does not:

$ old-podman rmi -a
Error: 2 errors occurred:
        * Image used by sha1: image is in use by a container
        * Image used by sha2: image is in use by a container
$ new-podman rmi -a
Error: image used by sha1: image is in use by a container
Error: image used by sha2: image is in use by a container

This Patch will cause podman COMMAND rm --force bogus not fail This is how Docker works, so Podman should follow this to allow existing scripts to convert from Docker to Podman. Fixes: containers#14612 Oprignal version of this patch came from wufan [email protected] Signed-off-by: Daniel J Walsh <[email protected]>

vrothberg

LGTM

edsantiago · 2022-07-27T17:15:31Z

LGTM. I, too, agree that the behavior this PR introduces (silence, exit 0) is the correct one even if another nameless client-daemon tool does it differently.

openshift-ci bot added the do-not-merge/release-note-label-needed Enforce release-note requirement, even if just None label Jul 18, 2022

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 18, 2022

rhatdan mentioned this pull request Jul 18, 2022

specifying --force,podman exit with 0 #14782

Closed

Luap99 reviewed Jul 18, 2022

View reviewed changes

rhatdan force-pushed the rm branch 2 times, most recently from 9a17726 to 95585e5 Compare July 18, 2022 17:15

openshift-ci bot added release-note and removed do-not-merge/release-note-label-needed Enforce release-note requirement, even if just None labels Jul 18, 2022

rhatdan force-pushed the rm branch 2 times, most recently from fb9e022 to 43af553 Compare July 19, 2022 09:05

rhatdan force-pushed the rm branch 5 times, most recently from d322c75 to f7bb9cc Compare July 24, 2022 22:12

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 25, 2022

rhatdan force-pushed the rm branch 4 times, most recently from faa5480 to 4b423b6 Compare July 26, 2022 11:06

rhatdan removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 26, 2022

vrothberg reviewed Jul 26, 2022

View reviewed changes

rhatdan force-pushed the rm branch from 4b423b6 to 75419c5 Compare July 26, 2022 20:00

vrothberg reviewed Jul 27, 2022

View reviewed changes

rhatdan added the lgtm Indicates that a PR is ready to be merged. label Jul 27, 2022

openshift-merge-robot merged commit 432348e into containers:main Jul 27, 2022

rhatdan deleted the rm branch December 1, 2022 22:01

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 Sep 20, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

When removing objects specifying --force,podman should exit with 0 #14959

When removing objects specifying --force,podman should exit with 0 #14959

rhatdan commented Jul 18, 2022 •

edited

Loading

openshift-ci bot commented Jul 18, 2022

rhatdan commented Jul 18, 2022

Luap99 left a comment

Luap99 Jul 18, 2022

Luap99 Jul 18, 2022

rhatdan commented Jul 18, 2022

Luap99 commented Jul 19, 2022

rhatdan commented Jul 19, 2022

maknop commented Jul 20, 2022

Luap99 commented Jul 21, 2022

rhatdan commented Jul 21, 2022

rhatdan commented Jul 25, 2022

rhatdan commented Jul 26, 2022

vrothberg left a comment

vrothberg Jul 26, 2022

vrothberg Jul 26, 2022

vrothberg Jul 26, 2022

Luap99 Jul 26, 2022

vrothberg Jul 27, 2022

mheon commented Jul 26, 2022

edsantiago commented Jul 26, 2022

vrothberg left a comment

edsantiago commented Jul 27, 2022

When removing objects specifying --force,podman should exit with 0 #14959

When removing objects specifying --force,podman should exit with 0 #14959

Conversation

rhatdan commented Jul 18, 2022 • edited Loading

Does this PR introduce a user-facing change?

openshift-ci bot commented Jul 18, 2022

rhatdan commented Jul 18, 2022

Luap99 left a comment

Choose a reason for hiding this comment

Luap99 Jul 18, 2022

Choose a reason for hiding this comment

Luap99 Jul 18, 2022

Choose a reason for hiding this comment

rhatdan commented Jul 18, 2022

Luap99 commented Jul 19, 2022

rhatdan commented Jul 19, 2022

maknop commented Jul 20, 2022

Luap99 commented Jul 21, 2022

rhatdan commented Jul 21, 2022

rhatdan commented Jul 25, 2022

rhatdan commented Jul 26, 2022

vrothberg left a comment

Choose a reason for hiding this comment

vrothberg Jul 26, 2022

Choose a reason for hiding this comment

vrothberg Jul 26, 2022

Choose a reason for hiding this comment

vrothberg Jul 26, 2022

Choose a reason for hiding this comment

Luap99 Jul 26, 2022

Choose a reason for hiding this comment

vrothberg Jul 27, 2022

Choose a reason for hiding this comment

mheon commented Jul 26, 2022

edsantiago commented Jul 26, 2022

vrothberg left a comment

Choose a reason for hiding this comment

edsantiago commented Jul 27, 2022

rhatdan commented Jul 18, 2022 •

edited

Loading