From dc33fc1ff433fcc70efc22f5cea9b87c6ec64a3b Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Mon, 20 Jun 2016 12:57:57 -0400 Subject: [PATCH] Add remove flags for service update with unit tests Signed-off-by: Daniel Nephin --- api/client/service/opts.go | 8 +- api/client/service/update.go | 138 +++++++++++++----- api/client/service/update_test.go | 80 ++++++++++ .../docker_cli_service_update_test.go | 2 +- 4 files changed, 187 insertions(+), 41 deletions(-) diff --git a/api/client/service/opts.go b/api/client/service/opts.go index d0b4e7e846..fcf7f87df8 100644 --- a/api/client/service/opts.go +++ b/api/client/service/opts.go @@ -461,7 +461,7 @@ func addServiceFlags(cmd *cobra.Command, opts *serviceOptions) { flags.StringVar(&opts.name, flagName, "", "Service name") flags.VarP(&opts.labels, flagLabel, "l", "Service labels") - flags.VarP(&opts.env, "env", "e", "Set environment variables") + flags.VarP(&opts.env, flagEnv, "e", "Set environment variables") flags.StringVarP(&opts.workdir, "workdir", "w", "", "Working directory inside the container") flags.StringVarP(&opts.user, flagUser, "u", "", "Username or UID") flags.Var(&opts.mounts, flagMount, "Attach a mount to the service") @@ -494,14 +494,20 @@ func addServiceFlags(cmd *cobra.Command, opts *serviceOptions) { const ( flagConstraint = "constraint" flagEndpointMode = "endpoint-mode" + flagEnv = "env" + flagEnvRemove = "remove-env" flagLabel = "label" + flagLabelRemove = "remove-label" flagLimitCPU = "limit-cpu" flagLimitMemory = "limit-memory" flagMode = "mode" flagMount = "mount" + flagMountRemove = "remove-mount" flagName = "name" flagNetwork = "network" + flagNetworkRemove = "remove-network" flagPublish = "publish" + flagPublishRemove = "remove-publish" flagReplicas = "replicas" flagReserveCPU = "reserve-cpu" flagReserveMemory = "reserve-memory" diff --git a/api/client/service/update.go b/api/client/service/update.go index 6aa7c10e73..b08b150a87 100644 --- a/api/client/service/update.go +++ b/api/client/service/update.go @@ -2,6 +2,7 @@ package service import ( "fmt" + "strings" "time" "golang.org/x/net/context" @@ -34,6 +35,11 @@ func newUpdateCommand(dockerCli *client.DockerCli) *cobra.Command { flags.String("image", "", "Service image tag") flags.String("args", "", "Service command args") addServiceFlags(cmd, opts) + flags.StringSlice(flagEnvRemove, []string{}, "Remove an environment variable") + flags.StringSlice(flagLabelRemove, []string{}, "The key of a label to remove") + flags.StringSlice(flagMountRemove, []string{}, "The mount target for a mount to remove") + flags.StringSlice(flagPublishRemove, []string{}, "The target port to remove") + flags.StringSlice(flagNetworkRemove, []string{}, "The name of a network to remove") return cmd } @@ -136,7 +142,7 @@ func updateService(flags *pflag.FlagSet, spec *swarm.ServiceSpec) error { updateLabels(flags, &spec.Labels) updateString("image", &cspec.Image) updateStringToSlice(flags, "args", &cspec.Args) - updateListOpts("env", &cspec.Env) + updateEnvironment(flags, &cspec.Env) updateString("workdir", &cspec.Dir) updateString(flagUser, &cspec.User) updateMounts(flags, &cspec.Mounts) @@ -169,7 +175,10 @@ func updateService(flags *pflag.FlagSet, spec *swarm.ServiceSpec) error { updateDurationOpt((flagRestartWindow), task.RestartPolicy.Window) } - // TODO: The constraints field is fixed in #23773 + if flags.Changed(flagConstraint) { + task.Placement = &swarm.Placement{} + updateSlice(flagConstraint, &task.Placement.Constraints) + } if err := updateReplicas(flags, &spec.Mode); err != nil { return err @@ -209,20 +218,6 @@ func updateStringToSlice(flags *pflag.FlagSet, flag string, field *[]string) err return err } -func updateLabels(flags *pflag.FlagSet, field *map[string]string) { - if !flags.Changed(flagLabel) { - return - } - - values := flags.Lookup(flagLabel).Value.(*opts.ListOpts).GetAll() - - localLabels := map[string]string{} - for key, value := range runconfigopts.ConvertKVStringsToMap(values) { - localLabels[key] = value - } - *field = localLabels -} - func anyChanged(flags *pflag.FlagSet, fields ...string) bool { for _, flag := range fields { if flags.Changed(flag) { @@ -232,42 +227,107 @@ func anyChanged(flags *pflag.FlagSet, fields ...string) bool { return false } -// TODO: should this override by destination path, or does swarm handle that? -func updateMounts(flags *pflag.FlagSet, mounts *[]swarm.Mount) { - if !flags.Changed(flagMount) { - return +func updateLabels(flags *pflag.FlagSet, field *map[string]string) { + if flags.Changed(flagLabel) { + if field == nil { + *field = map[string]string{} + } + + values := flags.Lookup(flagLabel).Value.(*opts.ListOpts).GetAll() + for key, value := range runconfigopts.ConvertKVStringsToMap(values) { + (*field)[key] = value + } } - *mounts = flags.Lookup(flagMount).Value.(*MountOpt).Value() + if field != nil && flags.Changed(flagLabelRemove) { + toRemove, _ := flags.GetStringSlice(flagLabelRemove) + for _, label := range toRemove { + delete(*field, label) + } + } +} + +func updateEnvironment(flags *pflag.FlagSet, field *[]string) { + if flags.Changed(flagEnv) { + value := flags.Lookup(flagEnv).Value.(*opts.ListOpts) + *field = append(*field, value.GetAll()...) + } + if flags.Changed(flagEnvRemove) { + toRemove, _ := flags.GetStringSlice(flagEnvRemove) + for _, envRemove := range toRemove { + for i, env := range *field { + key := envKey(env) + if key == envRemove { + *field = append((*field)[:i], (*field)[i+1:]...) + } + } + } + } +} + +func envKey(value string) string { + kv := strings.SplitN(value, "=", 2) + return kv[0] +} + +func updateMounts(flags *pflag.FlagSet, mounts *[]swarm.Mount) { + if flags.Changed(flagMount) { + values := flags.Lookup(flagMount).Value.(*MountOpt).Value() + *mounts = append(*mounts, values...) + } + if flags.Changed(flagMountRemove) { + toRemove, _ := flags.GetStringSlice(flagMountRemove) + for _, mountTarget := range toRemove { + for i, mount := range *mounts { + if mount.Target == mountTarget { + *mounts = append((*mounts)[:i], (*mounts)[i+1:]...) + } + } + } + } } -// TODO: should this override by name, or does swarm handle that? func updatePorts(flags *pflag.FlagSet, portConfig *[]swarm.PortConfig) { - if !flags.Changed(flagPublish) { - return + if flags.Changed(flagPublish) { + values := flags.Lookup(flagPublish).Value.(*opts.ListOpts).GetAll() + ports, portBindings, _ := nat.ParsePortSpecs(values) + + for port := range ports { + *portConfig = append(*portConfig, convertPortToPortConfig(port, portBindings)...) + } } - values := flags.Lookup(flagPublish).Value.(*opts.ListOpts).GetAll() - ports, portBindings, _ := nat.ParsePortSpecs(values) - - var localPortConfig []swarm.PortConfig - for port := range ports { - localPortConfig = append(localPortConfig, convertPortToPortConfig(port, portBindings)...) + if flags.Changed(flagPublishRemove) { + toRemove, _ := flags.GetStringSlice(flagPublishRemove) + 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:]...) + } + } + } } - *portConfig = localPortConfig } func updateNetworks(flags *pflag.FlagSet, attachments *[]swarm.NetworkAttachmentConfig) { - if !flags.Changed(flagNetwork) { - return + if flags.Changed(flagNetwork) { + networks, _ := flags.GetStringSlice(flagNetwork) + for _, network := range networks { + *attachments = append(*attachments, swarm.NetworkAttachmentConfig{Target: network}) + } } - networks, _ := flags.GetStringSlice(flagNetwork) - - var localAttachments []swarm.NetworkAttachmentConfig - for _, network := range networks { - localAttachments = append(localAttachments, swarm.NetworkAttachmentConfig{Target: network}) + if flags.Changed(flagNetworkRemove) { + toRemove, _ := flags.GetStringSlice(flagNetworkRemove) + for _, networkTarget := range toRemove { + for i, network := range *attachments { + if network.Target == networkTarget { + *attachments = append((*attachments)[:i], (*attachments)[i+1:]...) + } + } + } } - *attachments = localAttachments } 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 130e8b3e23..7f1797b06b 100644 --- a/api/client/service/update_test.go +++ b/api/client/service/update_test.go @@ -18,3 +18,83 @@ func TestUpdateServiceArgs(t *testing.T) { updateService(flags, spec) assert.EqualStringSlice(t, cspec.Args, []string{"the", "new args"}) } + +func TestUpdateLabels(t *testing.T) { + flags := newUpdateCommand(nil).Flags() + flags.Set("label", "toadd=newlabel") + flags.Set("remove-label", "toremove") + + labels := map[string]string{ + "toremove": "thelabeltoremove", + "tokeep": "value", + } + + updateLabels(flags, &labels) + assert.Equal(t, len(labels), 2) + assert.Equal(t, labels["tokeep"], "value") + assert.Equal(t, labels["toadd"], "newlabel") +} + +func TestUpdateEnvironment(t *testing.T) { + flags := newUpdateCommand(nil).Flags() + flags.Set("env", "toadd=newenv") + flags.Set("remove-env", "toremove") + + envs := []string{ + "toremove=theenvtoremove", + "tokeep=value", + } + + updateEnvironment(flags, &envs) + assert.Equal(t, len(envs), 2) + assert.Equal(t, envs[0], "tokeep=value") + assert.Equal(t, envs[1], "toadd=newenv") +} + +func TestUpdateMounts(t *testing.T) { + flags := newUpdateCommand(nil).Flags() + flags.Set("mount", "type=volume,target=/toadd") + flags.Set("remove-mount", "/toremove") + + mounts := []swarm.Mount{ + {Target: "/toremove", Type: swarm.MountType("BIND")}, + {Target: "/tokeep", Type: swarm.MountType("BIND")}, + } + + updateMounts(flags, &mounts) + assert.Equal(t, len(mounts), 2) + assert.Equal(t, mounts[0].Target, "/tokeep") + assert.Equal(t, mounts[1].Target, "/toadd") +} + +func TestUpdateNetworks(t *testing.T) { + flags := newUpdateCommand(nil).Flags() + flags.Set("network", "toadd") + flags.Set("remove-network", "toremove") + + attachments := []swarm.NetworkAttachmentConfig{ + {Target: "toremove", Aliases: []string{"foo"}}, + {Target: "tokeep"}, + } + + updateNetworks(flags, &attachments) + assert.Equal(t, len(attachments), 2) + assert.Equal(t, attachments[0].Target, "tokeep") + assert.Equal(t, attachments[1].Target, "toadd") +} + +func TestUpdatePorts(t *testing.T) { + flags := newUpdateCommand(nil).Flags() + flags.Set("publish", "1000:1000") + flags.Set("remove-publish", "333/udp") + + portConfigs := []swarm.PortConfig{ + {TargetPort: 333, Protocol: swarm.PortConfigProtocol("udp")}, + {TargetPort: 555}, + } + + updatePorts(flags, &portConfigs) + assert.Equal(t, len(portConfigs), 2) + assert.Equal(t, portConfigs[0].TargetPort, uint32(555)) + assert.Equal(t, portConfigs[1].TargetPort, uint32(1000)) +} diff --git a/integration-cli/docker_cli_service_update_test.go b/integration-cli/docker_cli_service_update_test.go index b5e6d35c08..680cb16245 100644 --- a/integration-cli/docker_cli_service_update_test.go +++ b/integration-cli/docker_cli_service_update_test.go @@ -22,7 +22,7 @@ func (s *DockerSwarmSuite) TestServiceUpdatePort(c *check.C) { waitAndAssert(c, defaultReconciliationTimeout, d.checkActiveContainerCount, checker.Equals, 1) // Update the service: changed the port mapping from 8080:8081 to 8082:8083. - _, err = d.Cmd("service", "update", "-p", "8082:8083", serviceName) + _, err = d.Cmd("service", "update", "-p", "8082:8083", "--remove-publish", "8081", serviceName) c.Assert(err, checker.IsNil) // Inspect the service and verify port mapping