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>

4 files changed, 88 insertions, 49 deletions

diff --git a/pkg/bindings/images/images.go b/pkg/bindings/images/images.go
index 4d8ae6a6e..034ade618 100644
--- a/pkg/bindings/images/images.go
+++ b/pkg/bindings/images/images.go
@@ -109,36 +109,6 @@ func Load(ctx context.Context, r io.Reader, name *string) (*entities.ImageLoadRe
 	return &report, response.Process(&report)
 }
 
-// Remove deletes an image from local storage.  The optional force parameter
-// will forcibly remove the image by removing all all containers, including
-// those that are Running, first.
-func Remove(ctx context.Context, images []string, opts entities.ImageRemoveOptions) (*entities.ImageRemoveReport, error) {
-	var report handlers.LibpodImagesRemoveReport
-	conn, err := bindings.GetClient(ctx)
-	if err != nil {
-		return nil, err
-	}
-	params := url.Values{}
-	params.Set("all", strconv.FormatBool(opts.All))
-	params.Set("force", strconv.FormatBool(opts.Force))
-	for _, i := range images {
-		params.Add("images", i)
-	}
-
-	response, err := conn.DoRequest(nil, http.MethodGet, "/images/remove", params)
-	if err != nil {
-		return nil, err
-	}
-	if err := response.Process(&report); err != nil {
-		return nil, err
-	}
-	var rmError error
-	if report.Error != "" {
-		rmError = errors.New(report.Error)
-	}
-	return &report.ImageRemoveReport, rmError
-}
-
 // Export saves an image from local storage as a tarball or image archive.  The optional format
 // parameter is used to change the format of the output.
 func Export(ctx context.Context, nameOrID string, w io.Writer, format *string, compress *bool) error {
diff --git a/pkg/bindings/images/rm.go b/pkg/bindings/images/rm.go
new file mode 100644
index 000000000..e3b5590df
--- /dev/null
+++ b/pkg/bindings/images/rm.go
@@ -0,0 +1,65 @@
+package images
+
+import (
+	"context"
+	"net/http"
+	"net/url"
+	"strconv"
+
+	"github.com/containers/libpod/pkg/api/handlers"
+	"github.com/containers/libpod/pkg/bindings"
+	"github.com/containers/libpod/pkg/domain/entities"
+	"github.com/containers/libpod/pkg/errorhandling"
+)
+
+// BachtRemove removes a batch of images from the local storage.
+func BatchRemove(ctx context.Context, images []string, opts entities.ImageRemoveOptions) (*entities.ImageRemoveReport, []error) {
+	// FIXME - bindings tests are missing for this endpoint. Once the CI is
+	// re-enabled for bindings, we need to add them.  At the time of writing,
+	// the tests don't compile.
+	var report handlers.LibpodImagesRemoveReport
+	conn, err := bindings.GetClient(ctx)
+	if err != nil {
+		return nil, []error{err}
+	}
+
+	params := url.Values{}
+	params.Set("all", strconv.FormatBool(opts.All))
+	params.Set("force", strconv.FormatBool(opts.Force))
+	for _, i := range images {
+		params.Add("images", i)
+	}
+
+	response, err := conn.DoRequest(nil, http.MethodDelete, "/images/remove", params)
+	if err != nil {
+		return nil, []error{err}
+	}
+	if err := response.Process(&report); err != nil {
+		return nil, []error{err}
+	}
+
+	return &report.ImageRemoveReport, errorhandling.StringsToErrors(report.Errors)
+}
+
+// Remove removes an image from the local storage.  Use force to remove an
+// image, even if it's used by containers.
+func Remove(ctx context.Context, nameOrID string, force bool) (*entities.ImageRemoveReport, error) {
+	var report handlers.LibpodImagesRemoveReport
+	conn, err := bindings.GetClient(ctx)
+	if err != nil {
+		return nil, err
+	}
+
+	params := url.Values{}
+	params.Set("force", strconv.FormatBool(force))
+	response, err := conn.DoRequest(nil, http.MethodDelete, "/images/%s/remove", params, nameOrID)
+	if err != nil {
+		return nil, err
+	}
+	if err := response.Process(&report); err != nil {
+		return nil, err
+	}
+
+	errs := errorhandling.StringsToErrors(report.Errors)
+	return &report.ImageRemoveReport, errorhandling.JoinErrors(errs)
+}
diff --git a/pkg/bindings/test/images_test.go b/pkg/bindings/test/images_test.go
index 58210efd0..9c8e82149 100644
--- a/pkg/bindings/test/images_test.go
+++ b/pkg/bindings/test/images_test.go
@@ -84,17 +84,20 @@ var _ = Describe("Podman images", func() {
 	// Test to validate the remove image api
 	It("remove image", func() {
 		// Remove invalid image should be a 404
-		_, err = images.Remove(bt.conn, "foobar5000", &bindings.PFalse)
+		response, err := images.Remove(bt.conn, "foobar5000", false)
 		Expect(err).ToNot(BeNil())
+		Expect(response).To(BeNil())
 		code, _ := bindings.CheckResponseCode(err)
 		Expect(code).To(BeNumerically("==", http.StatusNotFound))
 
 		// Remove an image by name, validate image is removed and error is nil
 		inspectData, err := images.GetImage(bt.conn, busybox.shortName, nil)
 		Expect(err).To(BeNil())
-		response, err := images.Remove(bt.conn, busybox.shortName, nil)
+		response, err = images.Remove(bt.conn, busybox.shortName, false)
 		Expect(err).To(BeNil())
-		Expect(inspectData.ID).To(Equal(response[0]["Deleted"]))
+		code, _ = bindings.CheckResponseCode(err)
+
+		Expect(inspectData.ID).To(Equal(response.Deleted[0]))
 		inspectData, err = images.GetImage(bt.conn, busybox.shortName, nil)
 		code, _ = bindings.CheckResponseCode(err)
 		Expect(code).To(BeNumerically("==", http.StatusNotFound))
@@ -104,30 +107,31 @@ var _ = Describe("Podman images", func() {
 		_, err = bt.RunTopContainer(&top, &bindings.PFalse, nil)
 		Expect(err).To(BeNil())
 		// we should now have a container called "top" running
-		containerResponse, err := containers.Inspect(bt.conn, "top", &bindings.PFalse)
+		containerResponse, err := containers.Inspect(bt.conn, "top", nil)
 		Expect(err).To(BeNil())
 		Expect(containerResponse.Name).To(Equal("top"))
 
 		// try to remove the image "alpine". This should fail since we are not force
 		// deleting hence image cannot be deleted until the container is deleted.
-		response, err = images.Remove(bt.conn, alpine.shortName, &bindings.PFalse)
+		response, err = images.Remove(bt.conn, alpine.shortName, false)
 		code, _ = bindings.CheckResponseCode(err)
-		Expect(code).To(BeNumerically("==", http.StatusInternalServerError))
+		Expect(code).To(BeNumerically("==", http.StatusConflict))
 
 		// Removing the image "alpine" where force = true
-		response, err = images.Remove(bt.conn, alpine.shortName, &bindings.PTrue)
+		response, err = images.Remove(bt.conn, alpine.shortName, true)
 		Expect(err).To(BeNil())
-
-		// Checking if both the images are gone as well as the container is deleted
-		inspectData, err = images.GetImage(bt.conn, busybox.shortName, nil)
+		// To be extra sure, check if the previously created container
+		// is gone as well.
+		_, err = containers.Inspect(bt.conn, "top", &bindings.PFalse)
 		code, _ = bindings.CheckResponseCode(err)
 		Expect(code).To(BeNumerically("==", http.StatusNotFound))
 
-		inspectData, err = images.GetImage(bt.conn, alpine.shortName, nil)
+		// Now make sure both images are gone.
+		inspectData, err = images.GetImage(bt.conn, busybox.shortName, nil)
 		code, _ = bindings.CheckResponseCode(err)
 		Expect(code).To(BeNumerically("==", http.StatusNotFound))
 
-		_, err = containers.Inspect(bt.conn, "top", &bindings.PFalse)
+		inspectData, err = images.GetImage(bt.conn, alpine.shortName, nil)
 		code, _ = bindings.CheckResponseCode(err)
 		Expect(code).To(BeNumerically("==", http.StatusNotFound))
 	})
@@ -209,7 +213,7 @@ var _ = Describe("Podman images", func() {
 
 	It("Load|Import Image", func() {
 		// load an image
-		_, err := images.Remove(bt.conn, alpine.name, nil)
+		_, err := images.Remove(bt.conn, alpine.name, false)
 		Expect(err).To(BeNil())
 		exists, err := images.Exists(bt.conn, alpine.name)
 		Expect(err).To(BeNil())
@@ -219,7 +223,7 @@ var _ = Describe("Podman images", func() {
 		Expect(err).To(BeNil())
 		names, err := images.Load(bt.conn, f, nil)
 		Expect(err).To(BeNil())
-		Expect(names.Name).To(Equal(alpine.name))
+		Expect(names.Names[0]).To(Equal(alpine.name))
 		exists, err = images.Exists(bt.conn, alpine.name)
 		Expect(err).To(BeNil())
 		Expect(exists).To(BeTrue())
@@ -227,7 +231,7 @@ var _ = Describe("Podman images", func() {
 		// load with a repo name
 		f, err = os.Open(filepath.Join(ImageCacheDir, alpine.tarballName))
 		Expect(err).To(BeNil())
-		_, err = images.Remove(bt.conn, alpine.name, nil)
+		_, err = images.Remove(bt.conn, alpine.name, false)
 		Expect(err).To(BeNil())
 		exists, err = images.Exists(bt.conn, alpine.name)
 		Expect(err).To(BeNil())
@@ -235,7 +239,7 @@ var _ = Describe("Podman images", func() {
 		newName := "quay.io/newname:fizzle"
 		names, err = images.Load(bt.conn, f, &newName)
 		Expect(err).To(BeNil())
-		Expect(names.Name).To(Equal(alpine.name))
+		Expect(names.Names[0]).To(Equal(alpine.name))
 		exists, err = images.Exists(bt.conn, newName)
 		Expect(err).To(BeNil())
 		Expect(exists).To(BeTrue())
@@ -243,7 +247,7 @@ var _ = Describe("Podman images", func() {
 		// load with a bad repo name should trigger a 500
 		f, err = os.Open(filepath.Join(ImageCacheDir, alpine.tarballName))
 		Expect(err).To(BeNil())
-		_, err = images.Remove(bt.conn, alpine.name, nil)
+		_, err = images.Remove(bt.conn, alpine.name, false)
 		Expect(err).To(BeNil())
 		exists, err = images.Exists(bt.conn, alpine.name)
 		Expect(err).To(BeNil())
@@ -271,7 +275,7 @@ var _ = Describe("Podman images", func() {
 
 	It("Import Image", func() {
 		// load an image
-		_, err = images.Remove(bt.conn, alpine.name, nil)
+		_, err = images.Remove(bt.conn, alpine.name, false)
 		Expect(err).To(BeNil())
 		exists, err := images.Exists(bt.conn, alpine.name)
 		Expect(err).To(BeNil())
diff --git a/pkg/bindings/test/manifests_test.go b/pkg/bindings/test/manifests_test.go
index 23c3d8194..4987dfe5b 100644
--- a/pkg/bindings/test/manifests_test.go
+++ b/pkg/bindings/test/manifests_test.go
@@ -47,7 +47,7 @@ var _ = Describe("Podman containers ", func() {
 		code, _ := bindings.CheckResponseCode(err)
 		Expect(code).To(BeNumerically("==", http.StatusInternalServerError))
 
-		_, err = images.Remove(bt.conn, id, nil)
+		_, err = images.Remove(bt.conn, id, false)
 		Expect(err).To(BeNil())
 
 		// create manifest list with images