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

thaJeztah · 2020-08-04T17:27:20Z

(noticed this while reviewing #2663. I thought we fixed these, but apparently not for all 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

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

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

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

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

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

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

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

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

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

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

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

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"
}

- What I did

- How I did it

- How to verify it

- Description for the changelog

Fix order of processing of `--label-add/--label-rm`, `--container-label-add/--container-label-rm`, and `--env-add/--env-rm` flags on  `docker service update` to allow replacing existing values.

- A picture of a cute animal (not mandatory but encouraged)

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>

thaJeztah · 2020-08-04T17:30:38Z

@silvin-lubecki @vdemeester ptal

codecov-commenter · 2020-08-04T17:34:17Z

Codecov Report

Merging #2668 into master will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2668      +/-   ##
==========================================
+ Coverage   58.13%   58.16%   +0.03%     
==========================================
  Files         295      295              
  Lines       21207    21207              
==========================================
+ Hits        12328    12336       +8     
+ Misses       7970     7963       -7     
+ Partials      909      908       -1

silvin-lubecki · 2020-08-04T20:26:09Z

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) {


question: should we test field to?

if field != nil && *field != nil

Good one, I easily get confused by these 🤔

yeah pointer on pointer is mind blowing 😁

I'll look for the extra check in a follow-up

silvin-lubecki

Looks good 👍

thaJeztah added impact/changelog status/2-code-review area/swarm labels Aug 4, 2020

silvin-lubecki reviewed Aug 4, 2020

View reviewed changes

silvin-lubecki approved these changes Aug 4, 2020

View reviewed changes

thaJeztah added this to the 20.03.0 milestone Aug 5, 2020

silvin-lubecki merged commit 51d96c7 into docker:master Aug 6, 2020

thaJeztah deleted the fix_add_rm_handling_order branch September 10, 2020 00:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

thaJeztah commented Aug 4, 2020

thaJeztah commented Aug 4, 2020

codecov-commenter commented Aug 4, 2020

silvin-lubecki Aug 4, 2020

thaJeztah Aug 5, 2020

silvin-lubecki Aug 5, 2020

thaJeztah Aug 6, 2020

silvin-lubecki left a comment

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

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

Conversation

thaJeztah commented Aug 4, 2020

Before this change:

With this patch applied:

thaJeztah commented Aug 4, 2020

codecov-commenter commented Aug 4, 2020

Codecov Report

silvin-lubecki Aug 4, 2020

Choose a reason for hiding this comment

thaJeztah Aug 5, 2020

Choose a reason for hiding this comment

silvin-lubecki Aug 5, 2020

Choose a reason for hiding this comment

thaJeztah Aug 6, 2020

Choose a reason for hiding this comment

silvin-lubecki left a comment

Choose a reason for hiding this comment