#387 fix: switch to image id map and add additional tests

go.mod

@@ -53,11 +53,11 @@ require (
 	github.com/theupdateframework/notary v0.6.1 // indirect
 	github.com/zmap/zlint v1.0.2 // indirect
 	golang.org/x/net v0.0.0-20190522155817-f3200d17e092
+	golang.org/x/sys v0.0.0-20190412213103-97732733099d
+	google.golang.org/genproto v0.0.0-20190404172233-64821d5d2107
+	google.golang.org/grpc v1.21.0
 	gopkg.in/dancannon/gorethink.v3 v3.0.5 // indirect
 	gopkg.in/fatih/pool.v2 v2.0.0 // indirect
 	gopkg.in/gorethink/gorethink.v3 v3.0.5 // indirect
-	golang.org/x/sys v0.0.0-20190403152447-81d4e9dc473e
-	google.golang.org/genproto v0.0.0-20190401181712-f467c93bbac2
-	google.golang.org/grpc v1.21.0
 	gotest.tools v2.2.0+incompatible // indirect
 )

go.sum

@@ -298,6 +298,7 @@ golang.org/x/sys v0.0.0-20181107165924-66b7b1311ac8/go.mod h1:STP8DvDyc/dI5b8T5h
 golang.org/x/sys v0.0.0-20181116152217-5ac8a444bdc5/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY=
 golang.org/x/sys v0.0.0-20181122145206-62eef0e2fa9b/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY=
 golang.org/x/sys v0.0.0-20190215142949-d0b11bdaac8a/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY=
+golang.org/x/sys v0.0.0-20190403152447-81d4e9dc473e/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
 golang.org/x/sys v0.0.0-20190412213103-97732733099d h1:+R4KGOnez64A81RvjARKc4UT5/tI9ujCIVX+P5KiHuI=
 golang.org/x/sys v0.0.0-20190412213103-97732733099d/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
 golang.org/x/text v0.3.0 h1:g61tztE5qeGQ89tm6NTjjM9VPIm088od1l6aSorWRWg=
@@ -319,6 +320,7 @@ google.golang.org/appengine v1.4.0 h1:/wp5JvzpHIxhs/dumFmF7BXTf3Z+dd4uXta4kVyO50
 google.golang.org/appengine v1.4.0/go.mod h1:xpcJRLb0r/rnEns0DIKYYv+WjYCduHsrkT7/EB5XEv4=
 google.golang.org/genproto v0.0.0-20180817151627-c66870c02cf8/go.mod h1:JiN7NxoALGmiZfu7CAH4rXhgtRTLTxftemlI0sWmxmc=
 google.golang.org/genproto v0.0.0-20190307195333-5fe7a883aa19/go.mod h1:VzzqZJRnGkLBvHegQrXjBqPurQTc5/KpmUdxsrq26oE=
+google.golang.org/genproto v0.0.0-20190401181712-f467c93bbac2/go.mod h1:VzzqZJRnGkLBvHegQrXjBqPurQTc5/KpmUdxsrq26oE=
 google.golang.org/genproto v0.0.0-20190404172233-64821d5d2107 h1:xtNn7qFlagY2mQNFHMSRPjT2RkOV4OXM7P5TVy9xATo=
 google.golang.org/genproto v0.0.0-20190404172233-64821d5d2107/go.mod h1:VzzqZJRnGkLBvHegQrXjBqPurQTc5/KpmUdxsrq26oE=
 google.golang.org/grpc v1.17.0/go.mod h1:6QZJwpn2B+Zp71q/5VxRsJ6NXXVCE5NRUHRo+f3cWCs=

internal/actions/actions_suite_test.go

@@ -1,18 +1,16 @@
 package actions_test
 
 import (
-	"errors"
 	"github.com/containrrr/watchtower/internal/actions"
 	"testing"
 	"time"
 
 	"github.com/containrrr/watchtower/pkg/container"
 	"github.com/containrrr/watchtower/pkg/container/mocks"
-	"github.com/docker/docker/api/types"
 
-	t "github.com/containrrr/watchtower/pkg/types"
 	cli "github.com/docker/docker/client"
 
+	. "github.com/containrrr/watchtower/internal/actions/mocks"
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
 )
@@ -24,7 +22,7 @@ func TestActions(t *testing.T) {
 
 var _ = Describe("the actions package", func() {
 	var dockerClient cli.CommonAPIClient
-	var client mockClient
+	var client MockClient
 	BeforeSuite(func() {
 		server := mocks.NewMockAPIServer()
 		dockerClient, _ = cli.NewClientWithOpts(
@@ -32,12 +30,15 @@ var _ = Describe("the actions package", func() {
 			cli.WithHTTPClient(server.Client()))
 	})
 	BeforeEach(func() {
-		client = mockClient{
+		pullImages := false
-			api:           dockerClient,
+		removeVolumes := false
-			pullImages:    false,
+
-			removeVolumes: false,
+		client = CreateMockClient(
-			TestData:      &TestData{},
+			&TestData {},
-		}
+			dockerClient,
+			pullImages,
+			removeVolumes,
+		)
 	})
 
 	Describe("the check prerequisites method", func() {
@@ -51,7 +52,7 @@ var _ = Describe("the actions package", func() {
 		When("given an array of one", func() {
 			It("should not do anything", func() {
 				client.TestData.Containers = []container.Container{
-					createMockContainer(
+					CreateMockContainer(
 						"test-container",
 						"test-container",
 						"watchtower",
@@ -63,27 +64,30 @@ var _ = Describe("the actions package", func() {
 		})
 		When("given multiple containers", func() {
 			BeforeEach(func() {
-				client = mockClient{
+				pullImages := false
-					api:           dockerClient,
+				removeVolumes := false
-					pullImages:    false,
+				client = CreateMockClient(
-					removeVolumes: false,
+					&TestData{
-					TestData: &TestData{
 						NameOfContainerToKeep: "test-container-02",
 						Containers: []container.Container{
-							createMockContainer(
+							CreateMockContainer(
 								"test-container-01",
 								"test-container-01",
 								"watchtower",
 								time.Now().AddDate(0, 0, -1)),
-							createMockContainer(
+							CreateMockContainer(
 								"test-container-02",
 								"test-container-02",
 								"watchtower",
 								time.Now()),
 						},
 					},
-				}
+					dockerClient,
+					pullImages,
+					removeVolumes,
+				)
 			})
+
 			It("should stop all but the latest one", func() {
 				err := actions.CheckForMultipleWatchtowerInstances(client, false)
 				Expect(err).NotTo(HaveOccurred())
@@ -91,96 +95,40 @@ var _ = Describe("the actions package", func() {
 		})
 		When("deciding whether to cleanup images", func() {
 			BeforeEach(func() {
-				client = mockClient{
+				pullImages := false
-					api:           dockerClient,
+				removeVolumes := false
-					pullImages:    false,
+
-					removeVolumes: false,
+				client = CreateMockClient(
-					TestData: &TestData{
+					&TestData{
 						Containers: []container.Container{
-							createMockContainer(
+							CreateMockContainer(
 								"test-container-01",
 								"test-container-01",
 								"watchtower",
 								time.Now().AddDate(0, 0, -1)),
-							createMockContainer(
+							CreateMockContainer(
 								"test-container-02",
 								"test-container-02",
 								"watchtower",
 								time.Now()),
 						},
 					},
-				}
+					dockerClient,
+					pullImages,
+					removeVolumes,
+				)
 			})
 			It("should try to delete the image if the cleanup flag is true", func() {
 				err := actions.CheckForMultipleWatchtowerInstances(client, true)
 				Expect(err).NotTo(HaveOccurred())
-				Expect(client.TestData.TriedToRemoveImage).To(BeTrue())
+				Expect(client.TestData.TriedToRemoveImage()).To(BeTrue())
 			})
 			It("should not try to delete the image if the cleanup flag is false", func() {
 				err := actions.CheckForMultipleWatchtowerInstances(client, false)
 				Expect(err).NotTo(HaveOccurred())
-				Expect(client.TestData.TriedToRemoveImage).To(BeFalse())
+				Expect(client.TestData.TriedToRemoveImage()).To(BeFalse())
 			})
 		})
 	})
 })
 
-func createMockContainer(id string, name string, image string, created time.Time) container.Container {
-	content := types.ContainerJSON{
-		ContainerJSONBase: &types.ContainerJSONBase{
-			ID:      id,
-			Image:   image,
-			Name:    name,
-			Created: created.String(),
-		},
-	}
-	return *container.NewContainer(&content, nil)
-}
-
-type mockClient struct {
-	TestData      *TestData
-	api           cli.CommonAPIClient
-	pullImages    bool
-	removeVolumes bool
-}
-
-type TestData struct {
-	TriedToRemoveImage    bool
-	NameOfContainerToKeep string
-	Containers            []container.Container
-}
-
-func (client mockClient) ListContainers(f t.Filter) ([]container.Container, error) {
-	return client.TestData.Containers, nil
-}
-
-func (client mockClient) StopContainer(c container.Container, d time.Duration) error {
-	if c.Name() == client.TestData.NameOfContainerToKeep {
-		return errors.New("tried to stop the instance we want to keep")
-	}
-	return nil
-}
-func (client mockClient) StartContainer(c container.Container) (string, error) {
-	panic("Not implemented")
-}
-
-func (client mockClient) RenameContainer(c container.Container, s string) error {
-	panic("Not implemented")
-}
-
-func (client mockClient) RemoveImage(c container.Container) error {
-	client.TestData.TriedToRemoveImage = true
-	return nil
-}
-
-func (client mockClient) GetContainer(containerID string) (container.Container, error) {
-	return container.Container{}, nil
-}
-
-func (client mockClient) ExecuteCommand(containerID string, command string) error {
-	return nil
-}
-
-func (client mockClient) IsContainerStale(c container.Container) (bool, error) {
-	panic("Not implemented")
-}

internal/actions/check.go

@@ -51,7 +51,7 @@ func cleanupExcessWatchtowers(containers []container.Container, client container
 		}
 
 		if cleanup {
-			if err := client.RemoveImage(c); err != nil {
+			if err := client.RemoveImageByID(c.ImageID()); err != nil {
 				// logging the original here as we're just returning a count
 				logrus.Error(err)
 				cleanupErrors++

internal/actions/mocks/client.go

@@ -0,0 +1,72 @@
+package mocks
+
+import (
+	"errors"
+	"github.com/containrrr/watchtower/pkg/container"
+	"time"
+
+	t "github.com/containrrr/watchtower/pkg/types"
+	cli "github.com/docker/docker/client"
+)
+
+type MockClient struct {
+	TestData      *TestData
+	api           cli.CommonAPIClient
+	pullImages    bool
+	removeVolumes bool
+}
+
+type TestData struct {
+	TriedToRemoveImageCount int
+	NameOfContainerToKeep string
+	Containers            []container.Container
+}
+
+func (testdata *TestData) TriedToRemoveImage() bool {
+	return testdata.TriedToRemoveImageCount > 0
+}
+
+func CreateMockClient(data *TestData, api cli.CommonAPIClient, pullImages bool, removeVolumes bool) MockClient {
+	return MockClient {
+		data,
+		api,
+		pullImages,
+		removeVolumes,
+	}
+}
+
+func (client MockClient) ListContainers(f t.Filter) ([]container.Container, error) {
+	return client.TestData.Containers, nil
+}
+
+func (client MockClient) StopContainer(c container.Container, d time.Duration) error {
+	if c.Name() == client.TestData.NameOfContainerToKeep {
+		return errors.New("tried to stop the instance we want to keep")
+	}
+	return nil
+}
+func (client MockClient) StartContainer(c container.Container) (string, error) {
+	return "", nil
+}
+
+func (client MockClient) RenameContainer(c container.Container, s string) error {
+	return nil
+}
+
+func (client MockClient) RemoveImageByID(id string) error {
+	client.TestData.TriedToRemoveImageCount++
+	return nil
+}
+
+func (client MockClient) GetContainer(containerID string) (container.Container, error) {
+	return container.Container{}, nil
+}
+
+func (client MockClient) ExecuteCommand(containerID string, command string) error {
+	return nil
+}
+
+func (client MockClient) IsContainerStale(c container.Container) (bool, error) {
+	return true, nil
+}
+

internal/actions/mocks/container.go

@@ -0,0 +1,28 @@
+package mocks
+
+import (
+	"github.com/containrrr/watchtower/pkg/container"
+	"github.com/docker/docker/api/types"
+	container2 "github.com/docker/docker/api/types/container"
+	"time"
+)
+
+func CreateMockContainer(id string, name string, image string, created time.Time) container.Container {
+	content := types.ContainerJSON{
+		ContainerJSONBase: &types.ContainerJSONBase{
+			ID:      id,
+			Image:   image,
+			Name:    name,
+			Created: created.String(),
+		},
+		Config: &container2.Config{
+			Labels: make(map[string]string),
+		},
+	}
+	return *container.NewContainer(
+		&content,
+		&types.ImageInspect{
+			ID: image,
+		},
+	)
+}

internal/actions/update.go

@@ -73,17 +73,18 @@ func stopStaleContainer(container container.Container, client container.Client,
 }
 
 func restartContainersInSortedOrder(containers []container.Container, client container.Client, params UpdateParams) {
-	toDelete := make(map[container.Container]bool)
+	imageIDs := make(map[string]bool)
+
 	for _, container := range containers {
 		if !container.Stale {
 			continue
 		}
 		restartStaleContainer(container, client, params)
-		toDelete[container] = true
+		imageIDs[container.ImageID()] = true
 	}
 	if params.Cleanup {
-		for cont := range toDelete {
+		for imageID := range imageIDs {
-			if err := client.RemoveImage(cont); err != nil {
+			if err := client.RemoveImageByID(imageID); err != nil {
 				log.Error(err)
 			}
 		}

internal/actions/update_test.go

@@ -0,0 +1,84 @@
+package actions_test
+
+import (
+	"github.com/containrrr/watchtower/internal/actions"
+	"github.com/containrrr/watchtower/pkg/container"
+	"github.com/containrrr/watchtower/pkg/container/mocks"
+	cli "github.com/docker/docker/client"
+	"time"
+
+	. "github.com/containrrr/watchtower/internal/actions/mocks"
+	. "github.com/onsi/ginkgo"
+	. "github.com/onsi/gomega"
+)
+
+
+var _ = Describe("the update action", func() {
+	var dockerClient cli.CommonAPIClient
+	var client MockClient
+
+	BeforeEach(func() {
+		server := mocks.NewMockAPIServer()
+		dockerClient, _ = cli.NewClientWithOpts(
+			cli.WithHost(server.URL),
+			cli.WithHTTPClient(server.Client()))
+	})
+
+
+	When("watchtower has been instructed to clean up", func() {
+		BeforeEach(func() {
+			pullImages := false
+			removeVolumes := false
+			client = CreateMockClient(
+				&TestData{
+					NameOfContainerToKeep: "test-container-02",
+					Containers: []container.Container{
+						CreateMockContainer(
+							"test-container-01",
+							"test-container-01",
+							"fake-image:latest",
+							time.Now().AddDate(0, 0, -1)),
+						CreateMockContainer(
+							"test-container-02",
+							"test-container-02",
+							"fake-image:latest",
+							time.Now()),
+						CreateMockContainer(
+							"test-container-02",
+							"test-container-02",
+							"fake-image:latest",
+							time.Now()),
+					},
+				},
+				dockerClient,
+				pullImages,
+				removeVolumes,
+			)
+		})
+
+		When("there are multiple containers using the same image", func() {
+			It("should only try to remove the image once", func() {
+
+				err := actions.Update(client, actions.UpdateParams{ Cleanup: true })
+				Expect(err).NotTo(HaveOccurred())
+				Expect(client.TestData.TriedToRemoveImageCount).To(Equal(1))
+			})
+		})
+		When("there are multiple containers using different images", func() {
+			It("should try to remove each of them", func() {
+				client.TestData.Containers = append(
+					client.TestData.Containers,
+					CreateMockContainer(
+						"unique-test-container",
+						"unique-test-container",
+						"unique-fake-image:latest",
+						time.Now(),
+					),
+				)
+				err := actions.Update(client, actions.UpdateParams{ Cleanup: true })
+				Expect(err).NotTo(HaveOccurred())
+				Expect(client.TestData.TriedToRemoveImageCount).To(Equal(2))
+			})
+		})
+	})
+})

pkg/container/client.go

@@ -29,7 +29,8 @@ type Client interface {
 	RenameContainer(Container, string) error
 	IsContainerStale(Container) (bool, error)
 	ExecuteCommand(containerID string, command string) error
-	RemoveImage(Container) error
+	RemoveImageByID(string) error
+
 }
 
 // NewClient returns a new Client instance which can be used to interact with
@@ -156,7 +157,7 @@ func (client dockerClient) StopContainer(c Container, timeout time.Duration) err
 
 	// Wait for container to be removed. In this case an error is a good thing
 	if err := client.waitForStopOrTimeout(c, timeout); err == nil {
-		return fmt.Errorf("Container %s (%s) could not be removed", c.Name(), c.ID())
+		return fmt.Errorf("container %s (%s) could not be removed", c.Name(), c.ID())
 	}
 
 	return nil
@@ -279,10 +280,16 @@ func (client dockerClient) IsContainerStale(c Container) (bool, error) {
 	return false, nil
 }
 
-func (client dockerClient) RemoveImage(c Container) error {
+func (client dockerClient) RemoveImageByID(id string) error {
-	imageID := c.ImageID()
+	log.Infof("Removing image %s", id)
-	log.Infof("Removing image %s", imageID)
+
-	_, err := client.api.ImageRemove(context.Background(), imageID, types.ImageRemoveOptions{Force: true})
+	_, err := client.api.ImageRemove(
+		context.Background(),
+		id,
+		types.ImageRemoveOptions{
+			Force: true,
+		})
+
 	return err
 }