From 2fc608cea6b5a68b60eaaf1f334168dfdded938b Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Tue, 4 Aug 2020 19:09:59 +0200 Subject: [PATCH] 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 --- cli/command/service/update.go | 28 +++++++------- cli/command/service/update_test.go | 62 ++++++++++++++++++++++-------- 2 files changed, 60 insertions(+), 30 deletions(-) diff --git a/cli/command/service/update.go b/cli/command/service/update.go index a36ea2e4f1df..78230e228121 100644 --- a/cli/command/service/update.go +++ b/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) { - toRemove := flags.Lookup(flagContainerLabelRemove).Value.(*opts.ListOpts).GetAll() +func updateLabels(flags *pflag.FlagSet, field *map[string]string) { + 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) { diff --git a/cli/command/service/update_test.go b/cli/command/service/update_test.go index a785d35f3db1..7a2511815bb3 100644 --- a/cli/command/service/update_test.go +++ b/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-rm", "toremove") + flags.Set("label-add", "add-beats-remove=value") + 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", - "tokeep": "value", + "to-keep": "value", + "to-remove": "value", + "to-replace": "value", + "to-update": "value", } updateLabels(flags, &labels) - assert.Check(t, is.Len(labels, 2)) - assert.Check(t, is.Equal("value", labels["tokeep"])) - assert.Check(t, is.Equal("newlabel", labels["toadd"])) + 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 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"} - updateLabels(flags, &labels) - assert.Check(t, is.Len(labels, 1)) + labels := map[string]string{ + "to-keep": "value", + "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) {