refactor: include service name in deployment plan

internal/cli/client/deploy.go

@@ -19,7 +19,8 @@ type Deployment struct {
 }
 
 type Plan struct {
-	ServiceID string
+	ServiceID   string
+	ServiceName string
 	SequenceOperation
 }
 
@@ -31,6 +32,7 @@ var ErrNoMatchingMachines = errors.New("no machines match the filter")
 
 // NewDeployment creates a new deployment for the given service specification.
 // If strategy is nil, a default RollingStrategy will be used.
+// TODO(refactor): do not return error
 func (cli *Client) NewDeployment(spec api.ServiceSpec, strategy Strategy) (*Deployment, error) {
 	if strategy == nil {
 		strategy = &RollingStrategy{}
@@ -103,11 +105,11 @@ func (d *Deployment) Validate(ctx context.Context) error {
 // It will create a new plan if one hasn't been created yet. The deployment will either create a new service or update
 // the existing one to match the desired specification.
 // TODO: forbid to run the same deployment more than once.
-func (d *Deployment) Run(ctx context.Context) (string, error) {
+func (d *Deployment) Run(ctx context.Context) (Plan, error) {
 	plan, err := d.Plan(ctx)
 	if err != nil {
-		return "", fmt.Errorf("create plan: %w", err)
+		return plan, fmt.Errorf("create plan: %w", err)
 	}
 
-	return plan.ServiceID, plan.Execute(ctx, d.cli)
+	return plan, plan.Execute(ctx, d.cli)
 }

internal/cli/client/service.go

@@ -68,14 +68,13 @@ func (cli *Client) RunService(
 			return fmt.Errorf("create deployment: %w", err)
 		}
 
-		serviceID, err := deploy.Run(ctx)
+		plan, err := deploy.Run(ctx)
 		if err != nil {
 			return err
 		}
 
-		resp.ID = serviceID
+		resp.ID = plan.ServiceID
-		// TODO: get the service name from the plan when it's available.
+		resp.Name = plan.ServiceName
-		resp.Name = spec.Name
 
 		return nil
 	}, cli.progressOut(), fmt.Sprintf("Running service %s (%s mode)", spec.Name, spec.Mode))

internal/cli/client/strategy.go

@@ -34,13 +34,14 @@ func (s *RollingStrategy) Type() string {
 func (s *RollingStrategy) Plan(
 	ctx context.Context, cli *Client, svc *api.Service, spec api.ServiceSpec,
 ) (Plan, error) {
+	// We can assume that the spec is valid at this point because it has been validated by the deployment.
 	switch spec.Mode {
 	case api.ServiceModeReplicated:
 		return s.planReplicated(ctx, cli, svc, spec)
 	case api.ServiceModeGlobal:
 		return s.planGlobal(ctx, cli, svc, spec)
 	default:
-		return Plan{}, fmt.Errorf("unsupported service mode: %s", spec.Mode)
+		return Plan{}, fmt.Errorf("unsupported service mode: '%s'", spec.Mode)
 	}
 }
 
@@ -50,17 +51,9 @@ func (s *RollingStrategy) Plan(
 func (s *RollingStrategy) planReplicated(
 	ctx context.Context, cli *Client, svc *api.Service, spec api.ServiceSpec,
 ) (Plan, error) {
-	var plan Plan
+	plan, err := newEmptyPlan(svc, spec)
-
+	if err != nil {
-	// Generate a new service ID for the first service deployment if it doesn't exist yet.
+		return plan, err
-	if svc != nil {
-		plan.ServiceID = svc.ID
-	} else {
-		var err error
-		plan.ServiceID, err = secret.NewID()
-		if err != nil {
-			return plan, fmt.Errorf("generate service ID: %w", err)
-		}
 	}
 
 	machines, err := cli.ListMachines(ctx)
@@ -217,23 +210,19 @@ func (s *RollingStrategy) planReplicated(
 func (s *RollingStrategy) planGlobal(
 	ctx context.Context, cli *Client, svc *api.Service, spec api.ServiceSpec,
 ) (Plan, error) {
-	var plan Plan
+	plan, err := newEmptyPlan(svc, spec)
+	if err != nil {
+		return plan, err
+	}
+
 	// Map machineID to service containers on that machine. For the global mode, there should be at most one
 	// container per machine but we use a slice to handle multiple containers that may exist due to a bug
 	// or interruption in the previous deployment.
 	containersOnMachine := make(map[string][]api.MachineContainer)
 	if svc != nil {
-		plan.ServiceID = svc.ID
 		for _, c := range svc.Containers {
 			containersOnMachine[c.MachineID] = append(containersOnMachine[c.MachineID], c)
 		}
-	} else {
-		// Generate a new service ID for the first service deployment.
-		var err error
-		plan.ServiceID, err = secret.NewID()
-		if err != nil {
-			return plan, fmt.Errorf("generate service ID: %w", err)
-		}
 	}
 
 	machines, err := cli.ListMachines(ctx)
@@ -362,3 +351,23 @@ func reconcileGlobalContainer(
 
 	return ops, nil
 }
+
+// newEmptyPlan creates a new empty plan for a service deployment with initialised service ID and name.
+func newEmptyPlan(svc *api.Service, spec api.ServiceSpec) (Plan, error) {
+	var plan Plan
+
+	// Generate a new service ID for the initial service deployment if it doesn't exist yet.
+	if svc != nil {
+		plan.ServiceID = svc.ID
+		plan.ServiceName = svc.Name
+	} else {
+		var err error
+		plan.ServiceID, err = secret.NewID()
+		if err != nil {
+			return plan, fmt.Errorf("generate service ID: %w", err)
+		}
+		plan.ServiceName = spec.Name
+	}
+
+	return plan, nil
+}

test/e2e/service_test.go

@@ -65,11 +65,13 @@ func TestDeployment(t *testing.T) {
 
 		plan, err := deploy.Plan(ctx)
 		require.NoError(t, err)
+		assert.NotEmpty(t, plan.ServiceID)
+		assert.NotEmpty(t, plan.ServiceName)
 		assert.Len(t, plan.SequenceOperation.Operations, 3) // 3 run
 
-		svcID, err := deploy.Run(ctx)
+		runPlan, err := deploy.Run(ctx)
 		require.NoError(t, err)
-		assert.NotEmpty(t, svcID)
+		assert.Equal(t, plan, runPlan)
 
 		svc, err := cli.InspectService(ctx, name)
 		require.NoError(t, err)
@@ -110,9 +112,8 @@ func TestDeployment(t *testing.T) {
 		require.NoError(t, err)
 		assert.Len(t, plan.SequenceOperation.Operations, 6) // 3 run + 3 remove
 
-		svcID, err = deploy.Run(ctx)
+		_, err = deploy.Run(ctx)
 		require.NoError(t, err)
-		assert.NotEmpty(t, svcID)
 
 		svc, err = cli.InspectService(ctx, name)
 		require.NoError(t, err)
@@ -149,9 +150,8 @@ func TestDeployment(t *testing.T) {
 		require.NoError(t, err)
 		assert.Len(t, plan.SequenceOperation.Operations, 9) // 3 stop + 3 run + 3 remove
 
-		svcID, err = deploy.Run(ctx)
+		_, err = deploy.Run(ctx)
 		require.NoError(t, err)
-		assert.NotEmpty(t, svcID)
 
 		svc, err = cli.InspectService(ctx, name)
 		require.NoError(t, err)
@@ -171,9 +171,8 @@ func TestDeployment(t *testing.T) {
 		require.NoError(t, err)
 		assert.Len(t, plan.SequenceOperation.Operations, 0) // no-op
 
-		svcID, err = deploy.Run(ctx)
+		_, err = deploy.Run(ctx)
 		require.NoError(t, err)
-		assert.NotEmpty(t, svcID)
 
 		svc, err = cli.InspectService(ctx, name)
 		require.NoError(t, err)
@@ -435,9 +434,9 @@ func TestDeployment(t *testing.T) {
 		require.NoError(t, err)
 		assert.Len(t, plan.SequenceOperation.Operations, 2) // 2 run operations for 2 replicas
 
-		svcID, err := deploy.Run(ctx)
+		runPlan, err := deploy.Run(ctx)
 		require.NoError(t, err)
-		assert.NotEmpty(t, svcID)
+		assert.Equal(t, plan, runPlan)
 
 		// Verify service was created with correct settings.
 		svc, err := cli.InspectService(ctx, name)