Delete does not fail when image is not found

[finishes #137370769]

Signed-off-by: Claudia Beresford <[email protected]>

commands/delete.go

@@ -3,6 +3,7 @@ package commands // import "code.cloudfoundry.org/grootfs/commands"
 import (
  "errors"
  "fmt"
+ "os"
  "path/filepath"
 
  "code.cloudfoundry.org/grootfs/commands/config"
@@ -42,10 +43,12 @@ var DeleteCommand = cli.Command{
 
  storePath := cfg.BaseStorePath
  idOrPath := ctx.Args().First()
- id, err := idfinder.FindID(storePath, idOrPath)
+ userID := os.Getuid()
+ id, err := idfinder.FindID(storePath, userID, idOrPath)
  if err != nil {
  logger.Error("find-id-failed", err, lager.Data{"id": idOrPath, "storePath": storePath})
- return cli.NewExitError(err.Error(), 1)
+ fmt.Println(err)
+ return nil
  }
 
  if id == idOrPath {

commands/idfinder/idfinder.go

@@ -2,26 +2,41 @@ package idfinder
 
 import (
  "fmt"
+ "os"
  "path/filepath"
  "regexp"
  "strings"
 
  "code.cloudfoundry.org/grootfs/store"
 )
 
-func FindID(storePath, pathOrID string) (string, error) {
+func FindID(storePath string, userID int, pathOrID string) (string, error) {
+ var (
+ imageID string
+ imagePath string
+ usrID string
+ )
+
+ usrID = fmt.Sprintf("%d", userID)
  if !strings.HasPrefix(pathOrID, "/") {
- return pathOrID, nil
+ imageID = pathOrID
+ imagePath = filepath.Join(storePath, usrID, store.IMAGES_DIR_NAME, imageID)
+ } else {
+ imagePathRegex := filepath.Join(storePath, usrID, store.IMAGES_DIR_NAME, "(.*)")
+ pathRegexp := regexp.MustCompile(imagePathRegex)
+ matches := pathRegexp.FindStringSubmatch(pathOrID)
+ if len(matches) != 2 {
+ return "", fmt.Errorf("path `%s` is outside store path", pathOrID)
+ }
+ imageID = matches[1]
+ imagePath = pathOrID
  }
 
- pathRegexp := regexp.MustCompile(filepath.Join(storePath, "[0-9]*", store.IMAGES_DIR_NAME, "(.*)"))
- matches := pathRegexp.FindStringSubmatch(pathOrID)
-
- if len(matches) != 2 {
- return "", fmt.Errorf("path `%s` is outside store path", pathOrID)
+ if !exists(imagePath) {
+ return "", fmt.Errorf("image `%s` was not found", imageID)
  }
 
- return matches[1], nil
+ return imageID, nil
 }
 
 func FindSubStorePath(storePath, path string) (string, error) {
@@ -34,3 +49,12 @@ func FindSubStorePath(storePath, path string) (string, error) {
 
  return filepath.Join(storePath, matches[1]), nil
 }
+
+func exists(imagePath string) bool {
+ if _, err := os.Stat(imagePath); err != nil {
+ if os.IsNotExist(err) {
+ return false
+ }
+ }
+ return true
+}

commands/idfinder/idfinder_test.go

@@ -1,48 +1,81 @@
 package idfinder_test
 
 import (
+ "io/ioutil"
+ "os"
+ "path"
+ "path/filepath"
+
  "code.cloudfoundry.org/grootfs/commands/idfinder"
+ "code.cloudfoundry.org/grootfs/store"
 
  . "github.com/onsi/ginkgo"
  . "github.com/onsi/gomega"
 )
 
 var _ = Describe("Idfinder", func() {
+ var (
+ storePath string
+ imageDir string
+ imageId string
+ err error
+ )
+
+ BeforeEach(func() {
+ imageId = "1234-my-id"
+ storePath, err = ioutil.TempDir("", "")
+ imageDir = filepath.Join(storePath, "0", store.IMAGES_DIR_NAME)
+ Expect(os.MkdirAll(imageDir, 0777)).To(Succeed())
+ Expect(err).NotTo(HaveOccurred())
+
+ Expect(ioutil.WriteFile(path.Join(imageDir, imageId), []byte("hello-world"), 0644)).To(Succeed())
+ })
+
+ AfterEach(func() {
+ Expect(os.RemoveAll(storePath)).To(Succeed())
+ })
+
  Context("FindID", func() {
  Context("when a ID is provided", func() {
  It("returns the ID", func() {
- id, err := idfinder.FindID("/hello/store/path", "1234-my-id")
+ id, err := idfinder.FindID(storePath, 0, imageId)
  Expect(err).NotTo(HaveOccurred())
- Expect(id).To(Equal("1234-my-id"))
+ Expect(id).To(Equal(imageId))
  })
  })
 
  Context("when a path is provided", func() {
  It("returns the ID", func() {
- id, err := idfinder.FindID("/hello/store/path", "/hello/store/path/1200/images/1234-my-id")
+ id, err := idfinder.FindID(storePath, 0, filepath.Join(imageDir, imageId))
  Expect(err).NotTo(HaveOccurred())
- Expect(id).To(Equal("1234-my-id"))
+ Expect(id).To(Equal(imageId))
  })
 
  Context("when the path is not within the store path", func() {
  It("returns an error", func() {
- _, err := idfinder.FindID("/hello/store/path", "/hello/not-store/path/images/1234-my-id")
+ _, err := idfinder.FindID("/hello/not-store/path", 0, filepath.Join("/hello/not-store/path/images", imageId))
  Expect(err).To(MatchError("path `/hello/not-store/path/images/1234-my-id` is outside store path"))
  })
  })
  })
+
+ It("returns an error when the image does not exist", func() {
+ _, err := idfinder.FindID(storePath, 0, filepath.Join(storePath, "0", store.IMAGES_DIR_NAME, "not-here"))
+ Expect(err).To(HaveOccurred())
+ Expect(err).To(MatchError(ContainSubstring("image `not-here` was not found")))
+ })
  })
 
  Context("SubStorePath", func() {
  It("returns the correct sub store path", func() {
- storePath, err := idfinder.FindSubStorePath("/hello/store/path", "/hello/store/path/1200/images/1234-my-id")
+ subStorePath, err := idfinder.FindSubStorePath(storePath, filepath.Join(imageDir, imageId))
  Expect(err).NotTo(HaveOccurred())
- Expect(storePath).To(Equal("/hello/store/path/1200"))
+ Expect(subStorePath).To(Equal(filepath.Join(storePath, "0")))
  })
 
  Context("when the path is not valid", func() {
  It("returns an error", func() {
- _, err := idfinder.FindSubStorePath("/hello/store/path", "/hello/store/path/images/1234-my-id")
+ _, err := idfinder.FindSubStorePath(storePath, "/hello/store/path/images/1234-my-id")
  Expect(err).To(MatchError(ContainSubstring("unable to match substore in path `/hello/store/path/images/1234-my-id`")))
  })
  })

commands/stats.go

@@ -41,7 +41,8 @@ var StatsCommand = cli.Command{
 
  storePath := cfg.BaseStorePath
  idOrPath := ctx.Args().First()
- id, err := idfinder.FindID(storePath, idOrPath)
+ userID := os.Getuid()
+ id, err := idfinder.FindID(storePath, userID, idOrPath)
  if err != nil {
  logger.Error("find-id-failed", err, lager.Data{"id": idOrPath, "storePath": storePath})
  return cli.NewExitError(err.Error(), 1)

integration/groot/delete_test.go

@@ -71,7 +71,7 @@ var _ = Describe("Delete", func() {
  logBuffer := gbytes.NewBuffer()
  err := Runner.WithStore("/invalid-store").WithStderr(logBuffer).
  Delete("/path/to/random-id")
- Expect(err).To(HaveOccurred())
+ Expect(err).ToNot(HaveOccurred())
  Expect(logBuffer).To(gbytes.Say(`"id":"/path/to/random-id"`))
  })
  })
@@ -82,24 +82,37 @@ var _ = Describe("Delete", func() {
  Expect(image.Path).NotTo(BeAnExistingFile())
  })
 
+ Context("when a path to an image does not exist", func() {
+ It("succeeds but logs a warning", func() {
+ fakePath := path.Join(StorePath, CurrentUserID, "images/non_existing")
+ Expect(fakePath).NotTo(BeAnExistingFile())
+
+ cmd := exec.Command(GrootFSBin, "--store", StorePath, "delete", fakePath)
+ sess, err := gexec.Start(cmd, GinkgoWriter, GinkgoWriter)
+ Expect(err).ToNot(HaveOccurred())
+ Eventually(sess).Should(gexec.Exit(0))
+ Eventually(sess.Out).Should(gbytes.Say("image `non_existing` was not found"))
+ })
+ })
+
  Context("when the path provided doesn't belong to the `--store` provided", func() {
  It("returns an error", func() {
  cmd := exec.Command(GrootFSBin, "--store", StorePath, "delete", "/Iamnot/in/the/storage/images/1234/rootfs")
  sess, err := gexec.Start(cmd, GinkgoWriter, GinkgoWriter)
  Expect(err).ToNot(HaveOccurred())
- Eventually(sess).Should(gexec.Exit(1))
+ Eventually(sess).Should(gexec.Exit(0))
  Eventually(sess.Out).Should(gbytes.Say("path `/Iamnot/in/the/storage/images/1234/rootfs` is outside store path"))
  })
  })
  })
 
  Context("when the image ID doesn't exist", func() {
- It("returns an error", func() {
+ It("succeeds but logs a warning", func() {
  cmd := exec.Command(GrootFSBin, "--store", StorePath, "delete", "non-existing-id")
  sess, err := gexec.Start(cmd, GinkgoWriter, GinkgoWriter)
  Expect(err).ToNot(HaveOccurred())
- Eventually(sess).Should(gexec.Exit(1))
- Eventually(sess.Out).Should(gbytes.Say("image not found"))
+ Eventually(sess).Should(gexec.Exit(0))
+ Eventually(sess.Out).Should(gbytes.Say("image `non-existing-id` was not found"))
  })
  })
 

integration/groot/stats_test.go

@@ -97,15 +97,15 @@ var _ = Describe("Stats", func() {
  Context("when the last parameter is a image id", func() {
  It("returns an error", func() {
  _, err := Runner.Stats("invalid-id")
- Expect(err).To(MatchError(ContainSubstring("image not found: invalid-id")))
+ Expect(err).To(MatchError(ContainSubstring("image `invalid-id` was not found")))
  })
  })
 
  Context("when the last parameter is a path", func() {
  It("returns an error", func() {
  invalidImagePath := filepath.Join(StorePath, CurrentUserID, store.IMAGES_DIR_NAME, "not-here")
  _, err := Runner.Stats(invalidImagePath)
- Expect(err).To(MatchError(ContainSubstring("image not found: not-here")))
+ Expect(err).To(MatchError(ContainSubstring("image `not-here` was not found")))
  })
 
  Context("when the path provided doesn't belong to the `--store` provided", func() {

integration/root/delete_test.go

@@ -1,6 +1,7 @@
 package root_test
 
 import (
+ "fmt"
  "io/ioutil"
  "os/exec"
  "path"
@@ -11,6 +12,7 @@ import (
 
  . "github.com/onsi/ginkgo"
  . "github.com/onsi/gomega"
+ "github.com/onsi/gomega/gbytes"
  "github.com/onsi/gomega/gexec"
 )
 
@@ -26,7 +28,7 @@ var _ = Describe("Delete", func() {
  })
 
  Context("when trying to delete a image from a different user", func() {
- It("returns an error", func() {
+ It("doesn't return an error but logs a warning", func() {
  deleteCmd := exec.Command(
  GrootFSBin,
  "--log-level", "debug",
@@ -44,7 +46,8 @@ var _ = Describe("Delete", func() {
 
  session, err := gexec.Start(deleteCmd, GinkgoWriter, GinkgoWriter)
  Expect(err).NotTo(HaveOccurred())
- Eventually(session).Should(gexec.Exit(1))
+ Eventually(session).Should(gexec.Exit(0))
+ Eventually(session.Out).Should(gbytes.Say(fmt.Sprintf("path `%s` is outside store path", image.Path)))
  Expect(image.Path).To(BeADirectory())
  })
  })