Merge pull request #2668 from thaJeztah/fix_add_rm_handling_order

Fix order of processing of some xx-add/xx-rm service update flags
docker · Aug 6, 2020 · 51d96c7 · 51d96c7
2 parents 5209eb6 + 2fc608c
commit 51d96c7
Show file tree

Hide file tree

Showing 2 changed files with 60 additions and 30 deletions.
diff --git 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
@@ -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) {