diff --git a/api/client/service/update.go b/api/client/service/update.go index 74b0c85dfc..8ad3f58e90 100644 --- a/api/client/service/update.go +++ b/api/client/service/update.go @@ -103,13 +103,6 @@ func updateService(flags *pflag.FlagSet, spec *swarm.ServiceSpec) error { } } - updateListOpts := func(flag string, field *[]string) { - if flags.Changed(flag) { - value := flags.Lookup(flag).Value.(*opts.ListOpts) - *field = value.GetAll() - } - } - updateInt64Value := func(flag string, field *int64) { if flags.Changed(flag) { *field = flags.Lookup(flag).Value.(int64Value).Value() @@ -243,15 +236,10 @@ func anyChanged(flags *pflag.FlagSet, fields ...string) bool { func updatePlacement(flags *pflag.FlagSet, placement *swarm.Placement) { field, _ := flags.GetStringSlice(flagConstraintAdd) - constraints := &placement.Constraints placement.Constraints = append(placement.Constraints, field...) toRemove := buildToRemoveSet(flags, flagConstraintRemove) - for i, constraint := range placement.Constraints { - if _, exists := toRemove[constraint]; exists { - *constraints = append((*constraints)[:i], (*constraints)[i+1:]...) - } - } + placement.Constraints = removeItems(placement.Constraints, toRemove, itemKey) } func updateLabels(flags *pflag.FlagSet, field *map[string]string) { @@ -280,12 +268,7 @@ func updateEnvironment(flags *pflag.FlagSet, field *[]string) { *field = append(*field, value.GetAll()...) } toRemove := buildToRemoveSet(flags, flagEnvRemove) - for i, env := range *field { - key := envKey(env) - if _, exists := toRemove[key]; exists { - *field = append((*field)[:i], (*field)[i+1:]...) - } - } + *field = removeItems(*field, toRemove, envKey) } func envKey(value string) string { @@ -293,6 +276,10 @@ func envKey(value string) string { return kv[0] } +func itemKey(value string) string { + return value +} + func buildToRemoveSet(flags *pflag.FlagSet, flag string) map[string]struct{} { var empty struct{} toRemove := make(map[string]struct{}) @@ -308,17 +295,34 @@ func buildToRemoveSet(flags *pflag.FlagSet, flag string) map[string]struct{} { return toRemove } +func removeItems( + seq []string, + toRemove map[string]struct{}, + keyFunc func(string) string, +) []string { + newSeq := []string{} + for _, item := range seq { + if _, exists := toRemove[keyFunc(item)]; !exists { + newSeq = append(newSeq, item) + } + } + return newSeq +} + func updateMounts(flags *pflag.FlagSet, mounts *[]swarm.Mount) { if flags.Changed(flagMountAdd) { values := flags.Lookup(flagMountAdd).Value.(*MountOpt).Value() *mounts = append(*mounts, values...) } toRemove := buildToRemoveSet(flags, flagMountRemove) - for i, mount := range *mounts { - if _, exists := toRemove[mount.Target]; exists { - *mounts = append((*mounts)[:i], (*mounts)[i+1:]...) + + newMounts := []swarm.Mount{} + for _, mount := range *mounts { + if _, exists := toRemove[mount.Target]; !exists { + newMounts = append(newMounts, mount) } } + *mounts = newMounts } func updatePorts(flags *pflag.FlagSet, portConfig *[]swarm.PortConfig) { @@ -331,19 +335,27 @@ func updatePorts(flags *pflag.FlagSet, portConfig *[]swarm.PortConfig) { } } - if flags.Changed(flagPublishRemove) { - toRemove := flags.Lookup(flagPublishRemove).Value.(*opts.ListOpts).GetAll() + if !flags.Changed(flagPublishRemove) { + return + } + toRemove := flags.Lookup(flagPublishRemove).Value.(*opts.ListOpts).GetAll() + newPorts := []swarm.PortConfig{} +portLoop: + for _, port := range *portConfig { for _, rawTargetPort := range toRemove { targetPort := nat.Port(rawTargetPort) - for i, port := range *portConfig { - if string(port.Protocol) == targetPort.Proto() && - port.TargetPort == uint32(targetPort.Int()) { - *portConfig = append((*portConfig)[:i], (*portConfig)[i+1:]...) - break - } + if equalPort(targetPort, port) { + continue portLoop } } + newPorts = append(newPorts, port) } + *portConfig = newPorts +} + +func equalPort(targetPort nat.Port, port swarm.PortConfig) bool { + return (string(port.Protocol) == targetPort.Proto() && + port.TargetPort == uint32(targetPort.Int())) } func updateNetworks(flags *pflag.FlagSet, attachments *[]swarm.NetworkAttachmentConfig) { @@ -354,11 +366,13 @@ func updateNetworks(flags *pflag.FlagSet, attachments *[]swarm.NetworkAttachment } } toRemove := buildToRemoveSet(flags, flagNetworkRemove) - for i, network := range *attachments { - if _, exists := toRemove[network.Target]; exists { - *attachments = append((*attachments)[:i], (*attachments)[i+1:]...) + newNetworks := []swarm.NetworkAttachmentConfig{} + for _, network := range *attachments { + if _, exists := toRemove[network.Target]; !exists { + newNetworks = append(newNetworks, network) } } + *attachments = newNetworks } func updateReplicas(flags *pflag.FlagSet, serviceMode *swarm.ServiceMode) error { diff --git a/api/client/service/update_test.go b/api/client/service/update_test.go index 86321e3de7..104a9e53e1 100644 --- a/api/client/service/update_test.go +++ b/api/client/service/update_test.go @@ -35,6 +35,15 @@ func TestUpdateLabels(t *testing.T) { assert.Equal(t, labels["toadd"], "newlabel") } +func TestUpdateLabelsRemoveALabelThatDoesNotExist(t *testing.T) { + flags := newUpdateCommand(nil).Flags() + flags.Set("label-rm", "dne") + + labels := map[string]string{"foo": "theoldlabel"} + updateLabels(flags, &labels) + assert.Equal(t, len(labels), 1) +} + func TestUpdatePlacement(t *testing.T) { flags := newUpdateCommand(nil).Flags() flags.Set("constraint-add", "node=toadd") @@ -63,6 +72,18 @@ func TestUpdateEnvironment(t *testing.T) { assert.Equal(t, envs[1], "toadd=newenv") } +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") + + envs := []string{"foo=value"} + + updateEnvironment(flags, &envs) + assert.Equal(t, len(envs), 0) +} + func TestUpdateMounts(t *testing.T) { flags := newUpdateCommand(nil).Flags() flags.Set("mount-add", "type=volume,target=/toadd")