feat: do not redeploy global service if spec hasn't changed

internal/api/container.go

@@ -58,10 +58,24 @@ func (c *Container) ServicePorts() ([]PortSpec, error) {
 	return ports, nil
 }
 
-func (c *Container) ServiceSpec() ServiceSpec {
+// ServiceSpec constructs a service spec from the container's configuration.
-	// TODO: migrate api.Container type to use ContainerJSON to make it possible to construct
+func (c *Container) ServiceSpec() (ServiceSpec, error) {
-	// a ServiceSpec from a Container.
+	ports, err := c.ServicePorts()
-	return ServiceSpec{}
+	if err != nil {
+		return ServiceSpec{}, fmt.Errorf("get service ports: %w", err)
+	}
+
+	return ServiceSpec{
+		Container: ContainerSpec{
+			Command: c.Config.Cmd,
+			Image:   c.Config.Image,
+			Init:    c.HostConfig.Init,
+			Volumes: c.HostConfig.Binds,
+		},
+		Mode:  c.ServiceMode(),
+		Name:  c.ServiceName(),
+		Ports: ports,
+	}, nil
 }
 
 // Healthy determines if the container is running and healthy.

internal/api/container_test.go

@@ -6,9 +6,55 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"net/netip"
+	"reflect"
 	"testing"
 )
 
+func TestContainer_ServiceSpec(t *testing.T) {
+	t.Parallel()
+
+	init := true
+	ctr := &Container{ContainerJSON: types.ContainerJSON{
+		ContainerJSONBase: &types.ContainerJSONBase{
+			HostConfig: &container.HostConfig{
+				Binds: []string{"/host/path:/container/path"},
+				Init:  &init,
+			},
+		},
+		Config: &container.Config{
+			Cmd:   []string{"/app/server"},
+			Image: "app:latest",
+			Labels: map[string]string{
+				LabelServiceID:    "test-service-id",
+				LabelServiceName:  "test-service-name",
+				LabelServicePorts: "app.example.com:8000/https",
+			},
+		},
+	}}
+
+	expectedSpec := ServiceSpec{
+		Container: ContainerSpec{
+			Command: []string{"/app/server"},
+			Image:   "app:latest",
+			Init:    &init,
+			Volumes: []string{"/host/path:/container/path"},
+		},
+		Name: "test-service-name",
+		Ports: []PortSpec{
+			{
+				Hostname:      "app.example.com",
+				ContainerPort: 8000,
+				Protocol:      ProtocolHTTPS,
+				Mode:          PortModeIngress,
+			},
+		},
+	}
+
+	spec, err := ctr.ServiceSpec()
+	require.NoError(t, err)
+	assert.True(t, reflect.DeepEqual(spec, expectedSpec))
+}
+
 func TestContainer_Healthy(t *testing.T) {
 	t.Parallel()
 

internal/api/service.go

@@ -39,6 +39,7 @@ func (s *ServiceSpec) Validate() error {
 }
 
 func (s *ServiceSpec) Equals(spec ServiceSpec) bool {
+	// TODO: ignore order of ports.
 	return reflect.DeepEqual(*s, spec)
 }
 

internal/cli/client/container.go

@@ -47,8 +47,8 @@ func (cli *Client) CreateContainer(
 			api.LabelManaged:     "",
 		},
 	}
-	if spec.Mode == api.ServiceModeGlobal {
+	if spec.Mode != "" {
-		config.Labels[api.LabelServiceMode] = api.ServiceModeGlobal
+		config.Labels[api.LabelServiceMode] = spec.Mode
 	}
 
 	if len(spec.Ports) > 0 {

internal/cli/client/strategy.go

@@ -120,7 +120,10 @@ func reconcileGlobalContainer(
 			continue
 		}
 
-		svcSpec := c.Container.ServiceSpec()
+		svcSpec, err := c.Container.ServiceSpec()
+		if err != nil {
+			return nil, fmt.Errorf("get service spec: %w", err)
+		}
 		if svcSpec.Equals(spec) {
 			// The container is already running with the same spec.
 			upToDate = true

test/e2e/service_test.go

@@ -3,7 +3,6 @@ package e2e
 import (
 	"context"
 	"errors"
-	"fmt"
 	"github.com/docker/docker/api/types/container"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -37,13 +36,14 @@ func TestDeployment(t *testing.T) {
 			require.ErrorIs(t, err, client.ErrNotFound)
 		})
 
-		deploy, err := cli.NewDeployment(api.ServiceSpec{
+		spec := api.ServiceSpec{
 			Name: name,
 			Mode: api.ServiceModeGlobal,
 			Container: api.ContainerSpec{
 				Image: "portainer/pause:latest",
 			},
-		}, nil)
+		}
+		deploy, err := cli.NewDeployment(spec, nil)
 		require.NoError(t, err)
 
 		err = deploy.Validate(ctx)
@@ -53,7 +53,6 @@ func TestDeployment(t *testing.T) {
 		require.NoError(t, err)
 		assert.IsType(t, &client.SequenceOperation{}, plan)
 		assert.Len(t, plan.(*client.SequenceOperation).Operations, 3) // 3 run
-		fmt.Println("# First plan:", plan)
 
 		err = deploy.Run(ctx)
 		require.NoError(t, err)
@@ -64,8 +63,12 @@ func TestDeployment(t *testing.T) {
 		assert.Equal(t, api.ServiceModeGlobal, svc.Mode)
 		assert.Len(t, svc.Containers, 3)
 
+		svcSpec, err := svc.Containers[0].Container.ServiceSpec()
+		require.NoError(t, err)
+		assert.True(t, svcSpec.Equals(spec))
+
 		// Deploy a published port.
-		deploy, err = cli.NewDeployment(api.ServiceSpec{
+		specWithPort := api.ServiceSpec{
 			Name: name,
 			Mode: api.ServiceModeGlobal,
 			Container: api.ContainerSpec{
@@ -79,14 +82,14 @@ func TestDeployment(t *testing.T) {
 					Mode:          api.PortModeHost,
 				},
 			},
-		}, nil)
+		}
+		deploy, err = cli.NewDeployment(specWithPort, nil)
 		require.NoError(t, err)
 
 		plan, err = deploy.Plan(ctx)
 		require.NoError(t, err)
 		assert.IsType(t, &client.SequenceOperation{}, plan)
 		assert.Len(t, plan.(*client.SequenceOperation).Operations, 6) // 3 run + 3 remove
-		fmt.Println("# Second plan:", plan)
 
 		err = deploy.Run(ctx)
 		require.NoError(t, err)
@@ -97,9 +100,13 @@ func TestDeployment(t *testing.T) {
 		assert.Equal(t, api.ServiceModeGlobal, svc.Mode)
 		assert.Len(t, svc.Containers, 3)
 
+		svcSpec, err = svc.Containers[0].Container.ServiceSpec()
+		require.NoError(t, err)
+		assert.True(t, svcSpec.Equals(specWithPort))
+
 		// Deploy the same conflicting port but with container spec changes
 		init := true
-		spec := api.ServiceSpec{
+		specWithPortAndInit := api.ServiceSpec{
 			Name: name,
 			Mode: api.ServiceModeGlobal,
 			Container: api.ContainerSpec{
@@ -115,14 +122,13 @@ func TestDeployment(t *testing.T) {
 				},
 			},
 		}
-		deploy, err = cli.NewDeployment(spec, nil)
+		deploy, err = cli.NewDeployment(specWithPortAndInit, nil)
 		require.NoError(t, err)
 
 		plan, err = deploy.Plan(ctx)
 		require.NoError(t, err)
 		assert.IsType(t, &client.SequenceOperation{}, plan)
 		assert.Len(t, plan.(*client.SequenceOperation).Operations, 9) // 3 stop + 3 run + 3 remove
-		fmt.Println("# Third plan:", plan)
 
 		err = deploy.Run(ctx)
 		require.NoError(t, err)
@@ -133,24 +139,27 @@ func TestDeployment(t *testing.T) {
 		assert.Equal(t, api.ServiceModeGlobal, svc.Mode)
 		assert.Len(t, svc.Containers, 3)
 
+		svcSpec, err = svc.Containers[0].Container.ServiceSpec()
+		require.NoError(t, err)
+		assert.True(t, svcSpec.Equals(specWithPortAndInit))
+
 		// Deploying the same spec should be a no-op.
-		//deploy, err = cli.NewDeployment(spec, nil)
+		deploy, err = cli.NewDeployment(specWithPortAndInit, nil)
-		//require.NoError(t, err)
+		require.NoError(t, err)
-		//
+
-		//plan, err = deploy.Plan(ctx)
+		plan, err = deploy.Plan(ctx)
-		//require.NoError(t, err)
+		require.NoError(t, err)
-		//assert.IsType(t, &client.SequenceOperation{}, plan)
+		assert.IsType(t, &client.SequenceOperation{}, plan)
-		//assert.Len(t, plan.(*client.SequenceOperation).Operations, 0) // no-op
+		assert.Len(t, plan.(*client.SequenceOperation).Operations, 0) // no-op
-		//fmt.Println("# Forth plan:", plan)
+
-		//
+		err = deploy.Run(ctx)
-		//err = deploy.Run(ctx)
+		require.NoError(t, err)
-		//require.NoError(t, err)
+
-		//
+		svc, err = cli.InspectService(ctx, name)
-		//svc, err = cli.InspectService(ctx, name)
+		require.NoError(t, err)
-		//require.NoError(t, err)
+		assert.Equal(t, name, svc.Name)
-		//assert.Equal(t, name, svc.Name)
+		assert.Equal(t, api.ServiceModeGlobal, svc.Mode)
-		//assert.Equal(t, api.ServiceModeGlobal, svc.Mode)
+		assert.Len(t, svc.Containers, 3)
-		//assert.Len(t, svc.Containers, 3)
 	})
 }