Skip to content
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.

Sign up for GitHub

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

specifying --force,podman exit with 0 #14782

Closed
wants to merge 6 commits into from
Closed

specifying --force,podman exit with 0 #14782

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
57bcd20
specifying --force,podman exit with 0
aonoa Jun 30, 2022
a197ff9
Merge branch 'containers:main' into main
aonoa Jul 6, 2022
f8d2145
consider more and add test
aonoa Jul 6, 2022
ef3546f
Merge code
aonoa Jul 12, 2022
e1c9286
Merge branch 'containers:main' into main
aonoa Jul 12, 2022
cab23bb
Merge code
aonoa Jul 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions cmd/podman/containers/rm.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,9 @@ func removeContainers(namesOrIDs []string, rmOptions entities.RmOptions, setExit
}
if setExit {
setExitCode(r.Err)
if rmOptions.Force && registry.GetExitCode() == 1 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should only happen if the error is container does not exist, and this is the only error returned.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recommend moving this into setExitCode() this will make the code much cleaner, same for the other places.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem with that is the NOEXIST errors are all different.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How are they all different? The setExitCode() already take care of setting this, so all what needs to be done is check if --force is set

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am saying the current code is incorrect since errors other then NOTEXIST should be reported --force rm should only not fail if the Object does not exist.

registry.SetExitCode(define.ExecErrorCodeIgnore)
}
}
errs = append(errs, r.Err)
} else {
Expand Down
5 changes: 4 additions & 1 deletion cmd/podman/images/rm.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,9 @@ package images

import (
"fmt"

"github.com/containers/podman/v4/cmd/podman/common"
"github.com/containers/podman/v4/cmd/podman/registry"
"github.com/containers/podman/v4/libpod/define"
"github.com/containers/podman/v4/pkg/domain/entities"
"github.com/containers/podman/v4/pkg/errorhandling"
"github.com/pkg/errors"
Expand Down Expand Up @@ -82,6 +82,9 @@ func rm(cmd *cobra.Command, args []string) error {
}
}
registry.SetExitCode(report.ExitCode)
if imageOpts.Force && registry.GetExitCode() == 1 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should only happen if the error is image does not exist, and this is the only error returned.

registry.SetExitCode(define.ExecErrorCodeIgnore)
}
}

return errorhandling.JoinErrors(rmErrors)
Expand Down
3 changes: 3 additions & 0 deletions cmd/podman/networks/rm.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,9 @@ func networkRm(cmd *cobra.Command, args []string) error {
fmt.Println(r.Name)
} else {
setExitCode(r.Err)
if networkRmOptions.Force && registry.GetExitCode() == 1 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should only happen if the error is network does not exist, and this is the only error returned.

registry.SetExitCode(define.ExecErrorCodeIgnore)
}
errs = append(errs, r.Err)
}
}
Expand Down
3 changes: 3 additions & 0 deletions cmd/podman/pods/rm.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,9 @@ func removePods(namesOrIDs []string, rmOptions entities.PodRmOptions, printIDs b
responses, err := registry.ContainerEngine().PodRm(context.Background(), namesOrIDs, rmOptions)
if err != nil {
setExitCode(err)
if rmOptions.Force && registry.GetExitCode() == 1 {
registry.SetExitCode(define.ExecErrorCodeIgnore)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should only happen if the error is pod does not exist, and this is the only error returned.

}
return err
}

Expand Down
3 changes: 3 additions & 0 deletions cmd/podman/root.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,9 @@ func Execute() {
fmt.Fprintln(os.Stderr, "Cannot connect to Podman. Please verify your connection to the Linux system using `podman system connection list`, or try `podman machine init` and `podman machine start` to manage a new Linux VM")
}
}
if registry.GetExitCode() == define.ExecErrorCodeIgnore {
registry.SetExitCode(0)
}
Comment on lines +109 to +111
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not use a special code for this, this will fail when a container exits with 128. Just set the exit code to 0 when you change it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I also think definition 128 is not good, but any error will make the return value not 0

podman/cmd/podman/root.go

Line 100 in 3426d56

if err := rootCmd.ExecuteContext(registry.GetContextWithOptions()); err != nil {

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you could make the setExitCode function return the error and then set it to nil if it should not error

Or maybe more easy make --force imply --ignore, that should work al tleast for the commands who have --ignore.

fmt.Fprintln(os.Stderr, formatError(err))
}
os.Exit(registry.GetExitCode())
Expand Down
3 changes: 3 additions & 0 deletions cmd/podman/volumes/rm.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,9 @@ func rm(cmd *cobra.Command, args []string) error {
fmt.Println(r.Id)
} else {
setExitCode(r.Err)
if rmOptions.Force && registry.GetExitCode() == 1 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should only happen if the error is volume does not exist, and this is the only error returned.

registry.SetExitCode(define.ExecErrorCodeIgnore)
}
errs = append(errs, r.Err)
}
}
Expand Down
2 changes: 2 additions & 0 deletions libpod/define/exec_codes.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ const (
ExecErrorCodeCannotInvoke = 126
// ExecErrorCodeNotFound is the error code to return when a command cannot be found
ExecErrorCodeNotFound = 127
// ExecErrorCodeIgnore is the error code to can be simply ignored
ExecErrorCodeIgnore = 128
)

// TranslateExecErrorToExitCode takes an error and checks whether it
Expand Down