Enable label-take-precedence for the no-pull option

docs/arguments.md

@@ -238,9 +238,12 @@ Environment Variable: WATCHTOWER_MONITOR_ONLY
 
 Note that monitor-only can also be specified on a per-container basis with the `com.centurylinklabs.watchtower.monitor-only` label set on those containers.
 
+See [With label taking precedence over arguments](##With label taking precedence over arguments) for behavior when both agument and label are set
+
+
 ## With label taking precedence over arguments
 
-By default, arguments will take precedence over labels. This means that if you set `WATCHTOWER_MONITOR_ONLY` to true or use `--monitor-only`, a container with `com.centurylinklabs.watchtower.monitor-only` set to false will not be updated. If you set `WATCHTOWER_LABEL_TAKE_PRECEDENCE` to true or use `--label-take-precedence`, then the container will also be updated
+By default, arguments will take precedence over labels. This means that if you set `WATCHTOWER_MONITOR_ONLY` to true or use `--monitor-only`, a container with `com.centurylinklabs.watchtower.monitor-only` set to false will not be updated. If you set `WATCHTOWER_LABEL_TAKE_PRECEDENCE` to true or use `--label-take-precedence`, then the container will also be updated. This also apply to the no pull option. if you set `WATCHTOWER_NO_PULL` to true or use `--no-pull`, a container with `com.centurylinklabs.watchtower.no-pull` set to false will not pull the new image. If you set `WATCHTOWER_LABEL_TAKE_PRECEDENCE` to true or use `--label-take-precedence`, then the container will pull image
 
 ```text
             Argument: --label-take-precedence
@@ -275,6 +278,8 @@ Environment Variable: WATCHTOWER_NO_PULL
 Note that no-pull can also be specified on a per-container basis with the
 `com.centurylinklabs.watchtower.no-pull` label set on those containers.
 
+See [With label taking precedence over arguments](##With label taking precedence over arguments) for behavior when both agument and label are set
+
 ## Without sending a startup message
 Do not send a message after watchtower started. Otherwise there will be an info-level notification.
 

internal/actions/mocks/client.go

@@ -86,7 +86,7 @@ func (client MockClient) ExecuteCommand(_ t.ContainerID, command string, _ int)
 }
 
 // IsContainerStale is true if not explicitly stated in TestData for the mock client
-func (client MockClient) IsContainerStale(cont t.Container) (bool, t.ImageID, error) {
+func (client MockClient) IsContainerStale(cont t.Container, params t.UpdateParams) (bool, t.ImageID, error) {
 	stale, found := client.TestData.Staleness[cont.Name()]
 	if !found {
 		stale = true

internal/actions/update.go

@@ -33,7 +33,7 @@ func Update(client container.Client, params types.UpdateParams) (types.Report, e
 	staleCheckFailed := 0
 
 	for i, targetContainer := range containers {
-		stale, newestImage, err := client.IsContainerStale(targetContainer)
+		stale, newestImage, err := client.IsContainerStale(targetContainer, params)
 		shouldUpdate := stale && !params.NoRestart && !targetContainer.IsMonitorOnly(params)
 		if err == nil && shouldUpdate {
 			// Check to make sure we have all the necessary information for recreating the container

pkg/container/client.go

@@ -30,7 +30,7 @@ type Client interface {
 	StopContainer(t.Container, time.Duration) error
 	StartContainer(t.Container) (t.ContainerID, error)
 	RenameContainer(t.Container, string) error
-	IsContainerStale(t.Container) (stale bool, latestImage t.ImageID, err error)
+	IsContainerStale(t.Container, t.UpdateParams) (stale bool, latestImage t.ImageID, err error)
 	ExecuteCommand(containerID t.ContainerID, command string, timeout int) (SkipUpdate bool, err error)
 	RemoveImageByID(t.ImageID) error
 	WarnOnHeadPullFailed(container t.Container) bool
@@ -308,10 +308,10 @@ func (client dockerClient) RenameContainer(c t.Container, newName string) error
 	return client.api.ContainerRename(bg, string(c.ID()), newName)
 }
 
-func (client dockerClient) IsContainerStale(container t.Container) (stale bool, latestImage t.ImageID, err error) {
+func (client dockerClient) IsContainerStale(container t.Container, params t.UpdateParams) (stale bool, latestImage t.ImageID, err error) {
 	ctx := context.Background()
 
-	if !client.PullImages || container.IsNoPull() {
+	if container.IsNoPull(params) {
 		log.Debugf("Skipping image pull.")
 	} else if err := client.PullImage(ctx, container); err != nil {
 		return false, container.SafeImageID(), err

pkg/container/container.go

@@ -133,14 +133,14 @@ func (c Container) Enabled() (bool, bool) {
 // the monitor-only label, the monitor-only argument and the label-take-precedence argument.
 func (c Container) IsMonitorOnly(params wt.UpdateParams) bool {
 	var containerMonitorOnlyLabel bool
-	
-	MonitorOnlyLabelIsDefined := false
+
+	LabelIsDefined := false
 
 	rawBool, ok := c.getLabelValue(monitorOnlyLabel)
 	if ok {
 		parsedBool, err := strconv.ParseBool(rawBool)
 		if err == nil {
-			MonitorOnlyLabelIsDefined = true
+			LabelIsDefined = true
 			containerMonitorOnlyLabel = parsedBool
 		} else {
 			// Defaulting to false
@@ -152,9 +152,9 @@ func (c Container) IsMonitorOnly(params wt.UpdateParams) bool {
 	}
 
 	// in case MonitorOnly argument is true, the results change if the container monitor-only label is explicitly set to false if the label-take-precedence is true
-	if params.MonitorOnly  {
-		if (MonitorOnlyLabelIsDefined) {
-			if params.LabelPrecedence  {
+	if params.MonitorOnly {
+		if LabelIsDefined {
+			if params.LabelPrecedence {
 				return containerMonitorOnlyLabel
 			} else {
 				return true
@@ -168,20 +168,42 @@ func (c Container) IsMonitorOnly(params wt.UpdateParams) bool {
 
 }
 
-// IsNoPull returns the value of the no-pull label. If the label is not set
-// then false is returned.
-func (c Container) IsNoPull() bool {
+// IsNoPull returns whether the image should be pulled based on values of
+// the no-pull label, the no-pull argument and the label-take-precedence argument.
+func (c Container) IsNoPull(params wt.UpdateParams) bool {
+	var containerNoPullLabel bool
+
+	LabelIsDefined := false
+
 	rawBool, ok := c.getLabelValue(noPullLabel)
-	if !ok {
-		return false
+	if ok {
+		parsedBool, err := strconv.ParseBool(rawBool)
+		if err == nil {
+			LabelIsDefined = true
+			containerNoPullLabel = parsedBool
+		} else {
+			// Defaulting to false
+			containerNoPullLabel = false
+		}
+	} else {
+		// Defaulting to false
+		containerNoPullLabel = false
 	}
 
-	parsedBool, err := strconv.ParseBool(rawBool)
-	if err != nil {
-		return false
+	// in case NoPull argument is true, the results change if the container no-pull label is explicitly set to false if the label-take-precedence is true
+	if params.NoPull {
+		if LabelIsDefined {
+			if params.LabelPrecedence {
+				return containerNoPullLabel
+			} else {
+				return true
+			}
+		} else {
+			return true
+		}
+	} else {
+		return containerNoPullLabel
 	}
-
-	return parsedBool
 }
 
 // Scope returns the value of the scope UID label and if the label

pkg/container/container_test.go

@@ -1,6 +1,7 @@
 package container
 
 import (
+	"github.com/containrrr/watchtower/pkg/types"
 	"github.com/docker/go-connections/nat"
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
@@ -215,34 +216,72 @@ var _ = Describe("the container", func() {
 		})
 
 		When("checking no-pull label", func() {
-			When("no-pull label is true", func() {
-				c := MockContainer(WithLabels(map[string]string{
-					"com.centurylinklabs.watchtower.no-pull": "true",
-				}))
-				It("should return true", func() {
-					Expect(c.IsNoPull()).To(Equal(true))
+			When("no-pull argument is not set", func() {
+				When("no-pull label is true", func() {
+					c := MockContainer(WithLabels(map[string]string{
+						"com.centurylinklabs.watchtower.no-pull": "true",
+					}))
+					It("should return true", func() {
+						Expect(c.IsNoPull(types.UpdateParams{})).To(Equal(true))
+					})
+				})
+				When("no-pull label is false", func() {
+					c := MockContainer(WithLabels(map[string]string{
+						"com.centurylinklabs.watchtower.no-pull": "false",
+					}))
+					It("should return false", func() {
+						Expect(c.IsNoPull(types.UpdateParams{})).To(Equal(false))
+					})
+				})
+				When("no-pull label is set to an invalid value", func() {
+					c := MockContainer(WithLabels(map[string]string{
+						"com.centurylinklabs.watchtower.no-pull": "maybe",
+					}))
+					It("should return false", func() {
+						Expect(c.IsNoPull(types.UpdateParams{})).To(Equal(false))
+					})
+				})
+				When("no-pull label is unset", func() {
+					c = MockContainer(WithLabels(map[string]string{}))
+					It("should return false", func() {
+						Expect(c.IsNoPull(types.UpdateParams{})).To(Equal(false))
+					})
 				})
 			})
-			When("no-pull label is false", func() {
-				c := MockContainer(WithLabels(map[string]string{
-					"com.centurylinklabs.watchtower.no-pull": "false",
-				}))
-				It("should return false", func() {
-					Expect(c.IsNoPull()).To(Equal(false))
+			When("no-pull argument is set to true", func() {
+				When("no-pull label is true", func() {
+					c := MockContainer(WithLabels(map[string]string{
+						"com.centurylinklabs.watchtower.no-pull": "true",
+					}))
+					It("should return true", func() {
+						Expect(c.IsNoPull(types.UpdateParams{NoPull: true})).To(Equal(true))
+					})
 				})
-			})
-			When("no-pull label is set to an invalid value", func() {
-				c := MockContainer(WithLabels(map[string]string{
-					"com.centurylinklabs.watchtower.no-pull": "maybe",
-				}))
-				It("should return false", func() {
-					Expect(c.IsNoPull()).To(Equal(false))
+				When("no-pull label is false", func() {
+					c := MockContainer(WithLabels(map[string]string{
+						"com.centurylinklabs.watchtower.no-pull": "false",
+					}))
+					It("should return true", func() {
+						Expect(c.IsNoPull(types.UpdateParams{NoPull: true})).To(Equal(true))
+					})
 				})
-			})
-			When("no-pull label is unset", func() {
-				c = MockContainer(WithLabels(map[string]string{}))
-				It("should return false", func() {
-					Expect(c.IsNoPull()).To(Equal(false))
+				When("label-take-precedence argument is set to true", func() {
+					When("no-pull label is true", func() {
+						c := MockContainer(WithLabels(map[string]string{
+							"com.centurylinklabs.watchtower.no-pull": "true",
+						}))
+						It("should return true", func() {
+							Expect(c.IsNoPull(types.UpdateParams{LabelPrecedence: true, NoPull: true})).To(Equal(true))
+						})
+					})
+					When("no-pull label is false", func() {
+						c := MockContainer(WithLabels(map[string]string{
+							"com.centurylinklabs.watchtower.no-pull": "false",
+						}))
+						It("should return false", func() {
+							Expect(c.IsNoPull(types.UpdateParams{LabelPrecedence: true, NoPull: true})).To(Equal(false))
+						})
+					})
 				})
 			})
 		})

pkg/types/container.go

@@ -67,7 +67,7 @@ type Container interface {
 	VerifyConfiguration() error
 	SetStale(bool)
 	IsStale() bool
-	IsNoPull() bool
+	IsNoPull(UpdateParams) bool
 	SetLinkedToRestarting(bool)
 	IsLinkedToRestarting() bool
 	PreUpdateTimeout() int

pkg/types/update_params.go

@@ -11,6 +11,7 @@ type UpdateParams struct {
 	NoRestart       bool
 	Timeout         time.Duration
 	MonitorOnly     bool
+	NoPull			bool
 	LifecycleHooks  bool
 	RollingRestart  bool
 	LabelPrecedence bool