Fix order of processing of some xx-add/xx-rm service update flags

Combining `-add` and `-rm` flags on `docker service update` should
be usable to explicitly replace existing options. The current order
of processing did not allow this, causing the `-rm` flag to remove
properties that were specified in `-add`. This behavior was inconsistent
with (for example) `--host-add` and `--host-rm`.

This patch updates the behavior to first remove properties, then
add new properties.

Note that there's still some improvements to make, to make the removal
more granulas (e.g. to make `--label-rm label=some-value` only remove
the label if value matches `some-value`); these changes are left for
a follow-up.

Before this change:
-----------------------------

Create a service with two env-vars

```bash
docker service create --env FOO=bar --env BAR=baz  --name=test nginx:alpine
docker service inspect --format '{{json .Spec.TaskTemplate.ContainerSpec.Env }}' test | jq .
[
  "FOO=bar",
  "BAR=baz"
]
```

Update the service, with the intent to replace the value of `FOO` for a new value

```bash
docker service update  --env-rm FOO --env-add FOO=updated-foo test
docker service inspect --format '{{json .Spec.TaskTemplate.ContainerSpec.Env }}' test | jq .
[
  "BAR=baz"
]
```

Create a service with two labels

```bash
docker service create --label FOO=bar --label BAR=baz  --name=test nginx:alpine
docker service inspect --format '{{json .Spec.Labels }}' test | jq .
{
  "BAR": "baz",
  "FOO": "bar"
}
```

Update the service, with the intent to replace the value of `FOO` for a new value

```bash
docker service update  --label-rm FOO --label-add FOO=updated-foo test
docker service inspect --format '{{json .Spec.Labels }}' test | jq .
{
  "BAR": "baz"
}
```

Create a service with two container labels

```bash
docker service create --container-label FOO=bar --container-label BAR=baz  --name=test nginx:alpine
docker service inspect --format '{{json .Spec.TaskTemplate.ContainerSpec.Labels }}' test | jq .
{
  "BAR": "baz",
  "FOO": "bar"
}
```

Update the service, with the intent to replace the value of `FOO` for a new value

```bash
docker service update  --container-label-rm FOO --container-label-add FOO=updated-foo test
docker service inspect --format '{{json .Spec.TaskTemplate.ContainerSpec.Labels }}' test | jq .
{
  "BAR": "baz",
}
```

With this patch applied:
--------------------------------

Create a service with two env-vars

```bash
docker service create --env FOO=bar --env BAR=baz  --name=test nginx:alpine
docker service inspect --format '{{json .Spec.TaskTemplate.ContainerSpec.Env }}' test | jq .
[
  "FOO=bar",
  "BAR=baz"
]
```

Update the service, and replace the value of `FOO` for a new value

```bash
docker service update  --env-rm FOO --env-add FOO=updated-foo test
docker service inspect --format '{{json .Spec.TaskTemplate.ContainerSpec.Env }}' test | jq .
[
  "BAR=baz",
  "FOO=updated-foo"
]
```

Create a service with two labels

```bash
docker service create --label FOO=bar --label BAR=baz  --name=test nginx:alpine
docker service inspect --format '{{json .Spec.Labels }}' test | jq .
{
  "BAR": "baz",
  "FOO": "bar"
}
```

Update the service, and replace the value of `FOO` for a new value

```bash
docker service update  --label-rm FOO --label-add FOO=updated-foo test
docker service inspect --format '{{json .Spec.Labels }}' test | jq .
{
  "BAR": "baz",
  "FOO": "updated-foo"
}
```

Create a service with two container labels

```bash
docker service create --container-label FOO=bar --container-label BAR=baz  --name=test nginx:alpine
docker service inspect --format '{{json .Spec.TaskTemplate.ContainerSpec.Labels }}' test | jq .
{
  "BAR": "baz",
  "FOO": "bar"
}
```

Update the service, and replace the value of `FOO` for a new value

```bash
docker service update  --container-label-rm FOO --container-label-add FOO=updated-foo test
docker service inspect --format '{{json .Spec.TaskTemplate.ContainerSpec.Labels }}' test | jq .
{
  "BAR": "baz",
  "FOO": "updated-foo"
}
```

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

cli/command/service/update.go

@@ -640,6 +640,12 @@ func updatePlacementPreferences(flags *pflag.FlagSet, placement *swarm.Placement
 }
 
 func updateContainerLabels(flags *pflag.FlagSet, field *map[string]string) {
+	if *field != nil && flags.Changed(flagContainerLabelRemove) {
+		toRemove := flags.Lookup(flagContainerLabelRemove).Value.(*opts.ListOpts).GetAll()
+		for _, label := range toRemove {
+			delete(*field, label)
+		}
+	}
 	if flags.Changed(flagContainerLabelAdd) {
 		if *field == nil {
 			*field = map[string]string{}
@@ -650,16 +656,15 @@ func updateContainerLabels(flags *pflag.FlagSet, field *map[string]string) {
 			(*field)[key] = value
 		}
 	}
+}
 
-	if *field != nil && flags.Changed(flagContainerLabelRemove) {
+func updateLabels(flags *pflag.FlagSet, field *map[string]string) {
-		toRemove := flags.Lookup(flagContainerLabelRemove).Value.(*opts.ListOpts).GetAll()
+	if *field != nil && flags.Changed(flagLabelRemove) {
+		toRemove := flags.Lookup(flagLabelRemove).Value.(*opts.ListOpts).GetAll()
 		for _, label := range toRemove {
 			delete(*field, label)
 		}
 	}
-}
-
-func updateLabels(flags *pflag.FlagSet, field *map[string]string) {
 	if flags.Changed(flagLabelAdd) {
 		if *field == nil {
 			*field = map[string]string{}
@@ -670,13 +675,6 @@ func updateLabels(flags *pflag.FlagSet, field *map[string]string) {
 			(*field)[key] = value
 		}
 	}
-
-	if *field != nil && flags.Changed(flagLabelRemove) {
-		toRemove := flags.Lookup(flagLabelRemove).Value.(*opts.ListOpts).GetAll()
-		for _, label := range toRemove {
-			delete(*field, label)
-		}
-	}
 }
 
 func updateSysCtls(flags *pflag.FlagSet, field *map[string]string) {
@@ -699,6 +697,9 @@ func updateSysCtls(flags *pflag.FlagSet, field *map[string]string) {
 }
 
 func updateEnvironment(flags *pflag.FlagSet, field *[]string) {
+	toRemove := buildToRemoveSet(flags, flagEnvRemove)
+	*field = removeItems(*field, toRemove, envKey)
+
 	if flags.Changed(flagEnvAdd) {
 		envSet := map[string]string{}
 		for _, v := range *field {
@@ -715,9 +716,6 @@ func updateEnvironment(flags *pflag.FlagSet, field *[]string) {
 			*field = append(*field, v)
 		}
 	}
-
-	toRemove := buildToRemoveSet(flags, flagEnvRemove)
-	*field = removeItems(*field, toRemove, envKey)
 }
 
 func getUpdatedSecrets(apiClient client.SecretAPIClient, flags *pflag.FlagSet, secrets []*swarm.SecretReference) ([]*swarm.SecretReference, error) {

cli/command/service/update_test.go

@@ -34,27 +34,58 @@ func TestUpdateServiceArgs(t *testing.T) {
 
 func TestUpdateLabels(t *testing.T) {
 	flags := newUpdateCommand(nil).Flags()
-	flags.Set("label-add", "toadd=newlabel")
+	flags.Set("label-add", "add-beats-remove=value")
-	flags.Set("label-rm", "toremove")
+	flags.Set("label-add", "to-add=value")
+	flags.Set("label-add", "to-update=new-value")
+	flags.Set("label-add", "to-replace=new-value")
+	flags.Set("label-rm", "add-beats-remove")
+	flags.Set("label-rm", "to-remove")
+	flags.Set("label-rm", "to-replace")
+	flags.Set("label-rm", "no-such-label")
 
 	labels := map[string]string{
-		"toremove": "thelabeltoremove",
+		"to-keep":    "value",
-		"tokeep":   "value",
+		"to-remove":  "value",
+		"to-replace": "value",
+		"to-update":  "value",
 	}
 
 	updateLabels(flags, &labels)
-	assert.Check(t, is.Len(labels, 2))
+	assert.DeepEqual(t, labels, map[string]string{
-	assert.Check(t, is.Equal("value", labels["tokeep"]))
+		"add-beats-remove": "value",
-	assert.Check(t, is.Equal("newlabel", labels["toadd"]))
+		"to-add":           "value",
+		"to-keep":          "value",
+		"to-replace":       "new-value",
+		"to-update":        "new-value",
+	})
 }
 
-func TestUpdateLabelsRemoveALabelThatDoesNotExist(t *testing.T) {
+func TestUpdateContainerLabels(t *testing.T) {
 	flags := newUpdateCommand(nil).Flags()
-	flags.Set("label-rm", "dne")
+	flags.Set("container-label-add", "add-beats-remove=value")
+	flags.Set("container-label-add", "to-add=value")
+	flags.Set("container-label-add", "to-update=new-value")
+	flags.Set("container-label-add", "to-replace=new-value")
+	flags.Set("container-label-rm", "add-beats-remove")
+	flags.Set("container-label-rm", "to-remove")
+	flags.Set("container-label-rm", "to-replace")
+	flags.Set("container-label-rm", "no-such-label")
 
-	labels := map[string]string{"foo": "theoldlabel"}
+	labels := map[string]string{
-	updateLabels(flags, &labels)
+		"to-keep":    "value",
-	assert.Check(t, is.Len(labels, 1))
+		"to-remove":  "value",
+		"to-replace": "value",
+		"to-update":  "value",
+	}
+
+	updateContainerLabels(flags, &labels)
+	assert.DeepEqual(t, labels, map[string]string{
+		"add-beats-remove": "value",
+		"to-add":           "value",
+		"to-keep":          "value",
+		"to-replace":       "new-value",
+		"to-update":        "new-value",
+	})
 }
 
 func TestUpdatePlacementConstraints(t *testing.T) {
@@ -115,14 +146,15 @@ func TestUpdateEnvironment(t *testing.T) {
 
 func TestUpdateEnvironmentWithDuplicateValues(t *testing.T) {
 	flags := newUpdateCommand(nil).Flags()
-	flags.Set("env-add", "foo=newenv")
-	flags.Set("env-add", "foo=dupe")
 	flags.Set("env-rm", "foo")
+	flags.Set("env-add", "foo=first")
+	flags.Set("env-add", "foo=second")
 
 	envs := []string{"foo=value"}
 
 	updateEnvironment(flags, &envs)
-	assert.Check(t, is.Len(envs, 0))
+	assert.Check(t, is.Len(envs, 1))
+	assert.Equal(t, envs[0], "foo=second")
 }
 
 func TestUpdateEnvironmentWithDuplicateKeys(t *testing.T) {