Make --publish-rm precedes --publish-add, so that add wins

`--publish-add 8081:81 --publish-add 8082:82 --publish-rm 80
--publish-rm 81/tcp --publish-rm 82/tcp` would thus result in 81 and
82 to be published.

Signed-off-by: Vincent Demeester <vincent@sbr.pm>

cli/command/service/update.go

@@ -630,15 +630,7 @@ func (r byPortConfig) Less(i, j int) bool {
 
 func portConfigToString(portConfig *swarm.PortConfig) string {
 	protocol := portConfig.Protocol
-	if protocol == "" {
-		protocol = "tcp"
-	}
-
 	mode := portConfig.PublishMode
-	if mode == "" {
-		mode = "ingress"
-	}
-
 	return fmt.Sprintf("%v:%v/%s/%s", portConfig.PublishedPort, portConfig.TargetPort, protocol, mode)
 }
 
@@ -663,28 +655,18 @@ func validatePublishRemove(val string) (string, error) {
 func updatePorts(flags *pflag.FlagSet, portConfig *[]swarm.PortConfig) error {
 	// The key of the map is `port/protocol`, e.g., `80/tcp`
 	portSet := map[string]swarm.PortConfig{}
-	// Check to see if there are any conflict in flags.
-	if flags.Changed(flagPublishAdd) {
-		ports := flags.Lookup(flagPublishAdd).Value.(*opts.PortOpt).Value()
 
-		for _, port := range ports {
+	// Build the current list of portConfig
-			if v, ok := portSet[portConfigToString(&port)]; ok && v != port {
-				return fmt.Errorf("conflicting port mapping between %v:%v/%s and %v:%v/%s", port.PublishedPort, port.TargetPort, port.Protocol, v.PublishedPort, v.TargetPort, v.Protocol)
-			}
-			portSet[portConfigToString(&port)] = port
-		}
-	}
-
-	// Override previous PortConfig in service if there is any duplicate
 	for _, entry := range *portConfig {
 		if _, ok := portSet[portConfigToString(&entry)]; !ok {
 			portSet[portConfigToString(&entry)] = entry
 		}
 	}
 
-	toRemove := flags.Lookup(flagPublishRemove).Value.(*opts.PortOpt).Value()
-
 	newPorts := []swarm.PortConfig{}
+
+	// Clean current ports
+	toRemove := flags.Lookup(flagPublishRemove).Value.(*opts.PortOpt).Value()
 portLoop:
 	for _, port := range portSet {
 		for _, pConfig := range toRemove {
@@ -698,6 +680,23 @@ portLoop:
 		newPorts = append(newPorts, port)
 	}
 
+	// Check to see if there are any conflict in flags.
+	if flags.Changed(flagPublishAdd) {
+		ports := flags.Lookup(flagPublishAdd).Value.(*opts.PortOpt).Value()
+
+		for _, port := range ports {
+			if v, ok := portSet[portConfigToString(&port)]; ok {
+				if v != port {
+					fmt.Println("v", v)
+					return fmt.Errorf("conflicting port mapping between %v:%v/%s and %v:%v/%s", port.PublishedPort, port.TargetPort, port.Protocol, v.PublishedPort, v.TargetPort, v.Protocol)
+				}
+				continue
+			}
+			//portSet[portConfigToString(&port)] = port
+			newPorts = append(newPorts, port)
+		}
+	}
+
 	// Sort the PortConfig to avoid unnecessary updates
 	sort.Sort(byPortConfig(newPorts))
 	*portConfig = newPorts

cli/command/service/update_test.go

@@ -220,28 +220,18 @@ func TestUpdatePorts(t *testing.T) {
 	assert.Equal(t, targetPorts[1], 1000)
 }
 
-func TestUpdatePortsDuplicateEntries(t *testing.T) {
+func TestUpdatePortsDuplicate(t *testing.T) {
 	// Test case for #25375
 	flags := newUpdateCommand(nil).Flags()
 	flags.Set("publish-add", "80:80")
 
 	portConfigs := []swarm.PortConfig{
-		{TargetPort: 80, PublishedPort: 80},
+		{
-	}
+			TargetPort:    80,
-
+			PublishedPort: 80,
-	err := updatePorts(flags, &portConfigs)
+			Protocol:      swarm.PortConfigProtocolTCP,
-	assert.Equal(t, err, nil)
+			PublishMode:   swarm.PortConfigPublishModeIngress,
-	assert.Equal(t, len(portConfigs), 1)
+		},
-	assert.Equal(t, portConfigs[0].TargetPort, uint32(80))
-}
-
-func TestUpdatePortsDuplicateKeys(t *testing.T) {
-	// Test case for #25375
-	flags := newUpdateCommand(nil).Flags()
-	flags.Set("publish-add", "80:80")
-
-	portConfigs := []swarm.PortConfig{
-		{TargetPort: 80, PublishedPort: 80},
 	}
 
 	err := updatePorts(flags, &portConfigs)
@@ -355,13 +345,19 @@ func TestUpdatePortsRmWithProtocol(t *testing.T) {
 	flags.Set("publish-rm", "82/udp")
 
 	portConfigs := []swarm.PortConfig{
-		{TargetPort: 80, PublishedPort: 8080, Protocol: swarm.PortConfigProtocolTCP},
+		{
+			TargetPort:    80,
+			PublishedPort: 8080,
+			Protocol:      swarm.PortConfigProtocolTCP,
+			PublishMode:   swarm.PortConfigPublishModeIngress,
+		},
 	}
 
 	err := updatePorts(flags, &portConfigs)
 	assert.Equal(t, err, nil)
-	assert.Equal(t, len(portConfigs), 1)
+	assert.Equal(t, len(portConfigs), 2)
-	assert.Equal(t, portConfigs[0].TargetPort, uint32(82))
+	assert.Equal(t, portConfigs[0].TargetPort, uint32(81))
+	assert.Equal(t, portConfigs[1].TargetPort, uint32(82))
 }
 
 // FIXME(vdemeester) port to opts.PortOpt

integration-cli/docker_cli_swarm_test.go

@@ -264,13 +264,13 @@ func (s *DockerSwarmSuite) TestSwarmPublishAdd(c *check.C) {
 		c.Assert(err, checker.IsNil, check.Commentf(out))
 		c.Assert(strings.TrimSpace(out), checker.Not(checker.Equals), "")
 
-		out, err = d.CmdRetryOutOfSequence("service", "update", "--publish-add", tc.publishAdd[0], tc.name)
+		out, err = d.cmdRetryOutOfSequence("service", "update", "--publish-add", tc.publishAdd[0], tc.name)
 		c.Assert(err, checker.IsNil, check.Commentf(out))
 
-		out, err = d.CmdRetryOutOfSequence("service", "update", "--publish-add", tc.publishAdd[1], tc.name)
+		out, err = d.cmdRetryOutOfSequence("service", "update", "--publish-add", tc.publishAdd[1], tc.name)
 		c.Assert(err, checker.IsNil, check.Commentf(out))
 
-		out, err = d.CmdRetryOutOfSequence("service", "update", "--publish-add", tc.publishAdd[2], "--publish-add", tc.publishAdd[3], tc.name)
+		out, err = d.cmdRetryOutOfSequence("service", "update", "--publish-add", tc.publishAdd[2], "--publish-add", tc.publishAdd[3], tc.name)
 		c.Assert(err, checker.NotNil, check.Commentf(out))
 
 		out, err = d.Cmd("service", "inspect", "--format", "{{ .Spec.EndpointSpec.Ports }}", tc.name)

opts/port.go

@@ -82,6 +82,14 @@ func (p *PortOpt) Set(value string) error {
 			return fmt.Errorf("missing mandatory field %q", portOptTargetPort)
 		}
 
+		if pConfig.PublishMode == "" {
+			pConfig.PublishMode = swarm.PortConfigPublishModeIngress
+		}
+
+		if pConfig.Protocol == "" {
+			pConfig.Protocol = swarm.PortConfigProtocolTCP
+		}
+
 		p.ports = append(p.ports, pConfig)
 	} else {
 		// short syntax
@@ -131,6 +139,7 @@ func ConvertPortToPortConfig(
 			Protocol:      swarm.PortConfigProtocol(strings.ToLower(port.Proto())),
 			TargetPort:    uint32(port.Int()),
 			PublishedPort: uint32(hostPort),
+			PublishMode:   swarm.PortConfigPublishModeIngress,
 		})
 	}
 	return ports

opts/port_test.go

@@ -16,8 +16,9 @@ func TestPortOptValidSimpleSyntax(t *testing.T) {
 			value: "80",
 			expected: []swarm.PortConfig{
 				{
-					Protocol:   "tcp",
+					Protocol:    "tcp",
-					TargetPort: 80,
+					TargetPort:  80,
+					PublishMode: swarm.PortConfigPublishModeIngress,
 				},
 			},
 		},
@@ -28,6 +29,7 @@ func TestPortOptValidSimpleSyntax(t *testing.T) {
 					Protocol:      "tcp",
 					TargetPort:    8080,
 					PublishedPort: 80,
+					PublishMode:   swarm.PortConfigPublishModeIngress,
 				},
 			},
 		},
@@ -38,6 +40,7 @@ func TestPortOptValidSimpleSyntax(t *testing.T) {
 					Protocol:      "tcp",
 					TargetPort:    80,
 					PublishedPort: 8080,
+					PublishMode:   swarm.PortConfigPublishModeIngress,
 				},
 			},
 		},
@@ -48,6 +51,7 @@ func TestPortOptValidSimpleSyntax(t *testing.T) {
 					Protocol:      "udp",
 					TargetPort:    8080,
 					PublishedPort: 80,
+					PublishMode:   swarm.PortConfigPublishModeIngress,
 				},
 			},
 		},
@@ -58,11 +62,13 @@ func TestPortOptValidSimpleSyntax(t *testing.T) {
 					Protocol:      "tcp",
 					TargetPort:    8080,
 					PublishedPort: 80,
+					PublishMode:   swarm.PortConfigPublishModeIngress,
 				},
 				{
 					Protocol:      "tcp",
 					TargetPort:    8081,
 					PublishedPort: 81,
+					PublishMode:   swarm.PortConfigPublishModeIngress,
 				},
 			},
 		},
@@ -73,16 +79,19 @@ func TestPortOptValidSimpleSyntax(t *testing.T) {
 					Protocol:      "udp",
 					TargetPort:    8080,
 					PublishedPort: 80,
+					PublishMode:   swarm.PortConfigPublishModeIngress,
 				},
 				{
 					Protocol:      "udp",
 					TargetPort:    8081,
 					PublishedPort: 81,
+					PublishMode:   swarm.PortConfigPublishModeIngress,
 				},
 				{
 					Protocol:      "udp",
 					TargetPort:    8082,
 					PublishedPort: 82,
+					PublishMode:   swarm.PortConfigPublishModeIngress,
 				},
 			},
 		},
@@ -106,7 +115,9 @@ func TestPortOptValidComplexSyntax(t *testing.T) {
 			value: "target=80",
 			expected: []swarm.PortConfig{
 				{
-					TargetPort: 80,
+					TargetPort:  80,
+					Protocol:    "tcp",
+					PublishMode: swarm.PortConfigPublishModeIngress,
 				},
 			},
 		},
@@ -114,8 +125,9 @@ func TestPortOptValidComplexSyntax(t *testing.T) {
 			value: "target=80,protocol=tcp",
 			expected: []swarm.PortConfig{
 				{
-					Protocol:   "tcp",
+					Protocol:    "tcp",
-					TargetPort: 80,
+					TargetPort:  80,
+					PublishMode: swarm.PortConfigPublishModeIngress,
 				},
 			},
 		},
@@ -126,6 +138,7 @@ func TestPortOptValidComplexSyntax(t *testing.T) {
 					Protocol:      "tcp",
 					TargetPort:    80,
 					PublishedPort: 8080,
+					PublishMode:   swarm.PortConfigPublishModeIngress,
 				},
 			},
 		},
@@ -136,6 +149,7 @@ func TestPortOptValidComplexSyntax(t *testing.T) {
 					Protocol:      "tcp",
 					TargetPort:    8080,
 					PublishedPort: 80,
+					PublishMode:   swarm.PortConfigPublishModeIngress,
 				},
 			},
 		},
@@ -157,6 +171,7 @@ func TestPortOptValidComplexSyntax(t *testing.T) {
 					TargetPort:    80,
 					PublishedPort: 8080,
 					PublishMode:   "host",
+					Protocol:      "tcp",
 				},
 			},
 		},
@@ -167,6 +182,7 @@ func TestPortOptValidComplexSyntax(t *testing.T) {
 					TargetPort:    80,
 					PublishedPort: 8080,
 					PublishMode:   "ingress",
+					Protocol:      "tcp",
 				},
 			},
 		},