image removal: refactor part 2

Continue the refactoring of image removal.  I didn't manage to break all
the following changes into smaller and easier to digest commits due to
time constraints:

 * Return an error slice instead of a single error. Use multierror only
   in the client/frontend.  Reflect that in the types.

 * Use the batch image removal in the client while preserving the more
   rest-idiomatic single-image removal endpoint.

 * Add a new handler for the single-image removal endpoint to make it
   share the same code as the batch endpoint.

 * Expose bindings for the single and batch endpoints, so we can
   properly test them.

 * Add several convenience functions for error handling to
   pkg/errorhandling.

 * Set the correct error type in libpod to set the exit code to 2 when
   one or more containers are using an image.

 * Massage the bindings tests a bit and tackle compilation errors.

Signed-off-by: Valentin Rothberg <rothberg@redhat.com>

2 files changed, 48 insertions, 65 deletions

diff --git a/pkg/domain/infra/abi/images.go b/pkg/domain/infra/abi/images.go
index 0af06fb89..7ab5131f0 100644
--- a/pkg/domain/infra/abi/images.go
+++ b/pkg/domain/infra/abi/images.go
@@ -21,7 +21,6 @@ import (
 	domainUtils "github.com/containers/libpod/pkg/domain/utils"
 	"github.com/containers/libpod/pkg/util"
 	"github.com/containers/storage"
-	"github.com/hashicorp/go-multierror"
 	imgspecv1 "github.com/opencontainers/image-spec/specs-go/v1"
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
@@ -423,8 +422,10 @@ func (ir *ImageEngine) Tree(ctx context.Context, nameOrId string, opts entities.
 	return &entities.ImageTreeReport{Tree: results}, nil
 }
 
-// Remove removes one or more images from local storage.
-func (ir *ImageEngine) Remove(ctx context.Context, images []string, opts entities.ImageRemoveOptions) (report *entities.ImageRemoveReport, finalError error) {
+// removeErrorsToExitCode returns an exit code for the specified slice of
+// image-removal errors. The error codes are set according to the documented
+// behaviour in the Podman man pages.
+func removeErrorsToExitCode(rmErrors []error) int {
 	var (
 		// noSuchImageErrors indicates that at least one image was not found.
 		noSuchImageErrors bool
@@ -434,59 +435,53 @@ func (ir *ImageEngine) Remove(ctx context.Context, images []string, opts entitie
 		// otherErrors indicates that at least one error other than the two
 		// above occured.
 		otherErrors bool
-		// deleteError is a multierror to conveniently collect errors during
-		// removal. We really want to delete as many images as possible and not
-		// error out immediately.
-		deleteError *multierror.Error
 	)
 
-	report = &entities.ImageRemoveReport{}
+	if len(rmErrors) == 0 {
+		return 0
+	}
 
-	// Set the removalCode and the error after all work is done.
-	defer func() {
-		switch {
-		// 2
-		case inUseErrors:
-			// One of the specified images has child images or is
-			// being used by a container.
-			report.ExitCode = 2
-		// 1
-		case noSuchImageErrors && !(otherErrors || inUseErrors):
-			// One of the specified images did not exist, and no other
-			// failures.
-			report.ExitCode = 1
-		// 0
+	for _, e := range rmErrors {
+		switch errors.Cause(e) {
+		case define.ErrNoSuchImage:
+			noSuchImageErrors = true
+		case define.ErrImageInUse, storage.ErrImageUsedByContainer:
+			inUseErrors = true
 		default:
-			// Nothing to do.
-		}
-		if deleteError != nil {
-			// go-multierror has a trailing new line which we need to remove to normalize the string.
-			finalError = deleteError.ErrorOrNil()
-			finalError = errors.New(strings.TrimSpace(finalError.Error()))
+			otherErrors = true
 		}
+	}
+
+	switch {
+	case inUseErrors:
+		// One of the specified images has child images or is
+		// being used by a container.
+		return 2
+	case noSuchImageErrors && !(otherErrors || inUseErrors):
+		// One of the specified images did not exist, and no other
+		// failures.
+		return 1
+	default:
+		return 125
+	}
+}
+
+// Remove removes one or more images from local storage.
+func (ir *ImageEngine) Remove(ctx context.Context, images []string, opts entities.ImageRemoveOptions) (report *entities.ImageRemoveReport, rmErrors []error) {
+	report = &entities.ImageRemoveReport{}
+
+	// Set the exit code at very end.
+	defer func() {
+		report.ExitCode = removeErrorsToExitCode(rmErrors)
 	}()
 
 	// deleteImage is an anonymous function to conveniently delete an image
 	// without having to pass all local data around.
 	deleteImage := func(img *image.Image) error {
 		results, err := ir.Libpod.RemoveImage(ctx, img, opts.Force)
-		switch errors.Cause(err) {
-		case nil:
-			break
-		case define.ErrNoSuchImage:
-			inUseErrors = true // ExitCode is expected
-		case storage.ErrImageUsedByContainer:
-			inUseErrors = true // Important for exit codes in Podman.
-			return errors.New(
-				fmt.Sprintf("A container associated with containers/storage, i.e. via Buildah, CRI-O, etc., may be associated with this image: %-12.12s\n", img.ID()))
-		case define.ErrImageInUse:
-			inUseErrors = true
-			return err
-		default:
-			otherErrors = true // Important for exit codes in Podman.
+		if err != nil {
 			return err
 		}
-
 		report.Deleted = append(report.Deleted, results.Deleted)
 		report.Untagged = append(report.Untagged, results.Untagged...)
 		return nil
@@ -499,9 +494,7 @@ func (ir *ImageEngine) Remove(ctx context.Context, images []string, opts entitie
 		for {
 			storageImages, err := ir.Libpod.ImageRuntime().GetRWImages()
 			if err != nil {
-				deleteError = multierror.Append(deleteError,
-					errors.Wrapf(err, "unable to query local images"))
-				otherErrors = true // Important for exit codes in Podman.
+				rmErrors = append(rmErrors, err)
 				return
 			}
 			// No images (left) to remove, so we're done.
@@ -510,9 +503,7 @@ func (ir *ImageEngine) Remove(ctx context.Context, images []string, opts entitie
 			}
 			// Prevent infinity loops by making a delete-progress check.
 			if previousImages == len(storageImages) {
-				otherErrors = true // Important for exit codes in Podman.
-				deleteError = multierror.Append(deleteError,
-					errors.New("unable to delete all images, check errors and re-run image removal if needed"))
+				rmErrors = append(rmErrors, errors.New("unable to delete all images, check errors and re-run image removal if needed"))
 				break
 			}
 			previousImages = len(storageImages)
@@ -520,15 +511,15 @@ func (ir *ImageEngine) Remove(ctx context.Context, images []string, opts entitie
 			for _, img := range storageImages {
 				isParent, err := img.IsParent(ctx)
 				if err != nil {
-					otherErrors = true // Important for exit codes in Podman.
-					deleteError = multierror.Append(deleteError, err)
+					rmErrors = append(rmErrors, err)
+					continue
 				}
 				// Skip parent images.
 				if isParent {
 					continue
 				}
 				if err := deleteImage(img); err != nil {
-					deleteError = multierror.Append(deleteError, err)
+					rmErrors = append(rmErrors, err)
 				}
 			}
 		}
@@ -539,21 +530,13 @@ func (ir *ImageEngine) Remove(ctx context.Context, images []string, opts entitie
 	// Delete only the specified images.
 	for _, id := range images {
 		img, err := ir.Libpod.ImageRuntime().NewFromLocal(id)
-		switch errors.Cause(err) {
-		case nil:
-			break
-		case image.ErrNoSuchImage:
-			noSuchImageErrors = true // Important for exit codes in Podman.
-			fallthrough
-		default:
-			deleteError = multierror.Append(deleteError, errors.Wrapf(err, "failed to remove image '%s'", id))
+		if err != nil {
+			rmErrors = append(rmErrors, err)
 			continue
 		}
-
 		err = deleteImage(img)
 		if err != nil {
-			otherErrors = true // Important for exit codes in Podman.
-			deleteError = multierror.Append(deleteError, err)
+			rmErrors = append(rmErrors, err)
 		}
 	}
 
diff --git a/pkg/domain/infra/tunnel/images.go b/pkg/domain/infra/tunnel/images.go
index dcc5fc3e7..00893194c 100644
--- a/pkg/domain/infra/tunnel/images.go
+++ b/pkg/domain/infra/tunnel/images.go
@@ -20,8 +20,8 @@ func (ir *ImageEngine) Exists(_ context.Context, nameOrId string) (*entities.Boo
 	return &entities.BoolReport{Value: found}, err
 }
 
-func (ir *ImageEngine) Remove(ctx context.Context, imagesArg []string, opts entities.ImageRemoveOptions) (*entities.ImageRemoveReport, error) {
-	return images.Remove(ir.ClientCxt, imagesArg, opts)
+func (ir *ImageEngine) Remove(ctx context.Context, imagesArg []string, opts entities.ImageRemoveOptions) (*entities.ImageRemoveReport, []error) {
+	return images.BatchRemove(ir.ClientCxt, imagesArg, opts)
 }
 
 func (ir *ImageEngine) List(ctx context.Context, opts entities.ImageListOptions) ([]*entities.ImageSummary, error) {