diff --git a/internal/cli/client/deploy.go b/internal/cli/client/deploy.go index 14992bc6..b7e53d5c 100644 --- a/internal/cli/client/deploy.go +++ b/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) } diff --git a/internal/cli/client/service.go b/internal/cli/client/service.go index 17be70dc..f1de86e7 100644 --- a/internal/cli/client/service.go +++ b/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 - // TODO: get the service name from the plan when it's available. - resp.Name = spec.Name + resp.ID = plan.ServiceID + resp.Name = plan.ServiceName return nil }, cli.progressOut(), fmt.Sprintf("Running service %s (%s mode)", spec.Name, spec.Mode)) diff --git a/internal/cli/client/strategy.go b/internal/cli/client/strategy.go index fbf18844..05a3fb87 100644 --- a/internal/cli/client/strategy.go +++ b/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 - - // Generate a new service ID for the first service deployment if it doesn't exist yet. - 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) - } + plan, err := newEmptyPlan(svc, spec) + if err != nil { + return plan, 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 +} diff --git a/test/e2e/service_test.go b/test/e2e/service_test.go index 16fe7f1a..f2053058 100644 --- a/test/e2e/service_test.go +++ b/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)