diff --git a/pkg/client/deploy/deploy.go b/pkg/client/deploy/deploy.go index 495787cf..f497ab39 100644 --- a/pkg/client/deploy/deploy.go +++ b/pkg/client/deploy/deploy.go @@ -1,11 +1,15 @@ package deploy import ( + "cmp" "context" "errors" "fmt" + "maps" + "slices" "strconv" "strings" + "unicode" "charm.land/lipgloss/v2" "charm.land/lipgloss/v2/table" @@ -50,25 +54,31 @@ type ServicePlan struct { // Format renders the service plan as a styled block with a spec diff and nested container operations. func (sp *ServicePlan) Format() string { // Determine service-level operation type and extract the old spec from container operations. - // Assume replace operations precede remove operations (rolling strategy) so the first replace operation - // (if exists) determines the old spec for the diff. Otherwise, fallback to the first remove operation. + // Containers may have different specs after a failed deployment. The first replaced container determines the old + // spec for the diff because a container updated in place already has the new image. Otherwise, fallback to the + // first updated container and then the first removed one. var hasRun, hasRemove bool - var oldSpec *api.ServiceSpec + var replacedSpec, updatedSpec, removedSpec *api.ServiceSpec for _, op := range sp.Operations { switch o := op.(type) { case *operation.RunContainerOperation: hasRun = true case *operation.ReplaceContainerOperation: - if oldSpec == nil { - oldSpec = &o.OldContainer.ServiceSpec + if replacedSpec == nil { + replacedSpec = &o.OldContainer.ServiceSpec + } + case *operation.UpdateContainerOperation: + if updatedSpec == nil { + updatedSpec = &o.Container.ServiceSpec } case *operation.RemoveContainerOperation: hasRemove = true - if oldSpec == nil { - oldSpec = &o.Container.ServiceSpec + if removedSpec == nil { + removedSpec = &o.Container.ServiceSpec } } } + oldSpec := cmp.Or(replacedSpec, updatedSpec, removedSpec) // Service line modifier and verb. var modifier, verb string @@ -91,7 +101,7 @@ func (sp *ServicePlan) Format() string { out.WriteString("\n") // Build spec diff table: columns are [modifier, attribute, value or change]. - // TODO: print diff for all changed attributes, not just image and replicas. + // TODO: print diff for all changed attributes, not just image, replicas, and labels. // Consider reusing the logic in EvalContainerSpecChange to return a structured diff. specTable := table.New(). Border(lipgloss.Border{}). @@ -141,17 +151,23 @@ func (sp *ServicePlan) Format() string { } } - // Stack " │ " tree prefixes vertically, then join horizontally with the table. - tableStr := specTable.String() - treePrefix := tui.Faint.Render(" │ ") - treeColRows := make([]string, specTable.GetData().Rows()) - for i := range treeColRows { - treeColRows[i] = treePrefix - } - treeCol := lipgloss.JoinVertical(lipgloss.Left, treeColRows...) + specDiff := specTable.String() + "\n" - out.WriteString(lipgloss.JoinHorizontal(lipgloss.Top, treeCol, tableStr)) - out.WriteString("\n") + // Render labels as nested blocks outside the table to not widen its attribute column. An existing service without + // an old spec only has new containers added to up-to-date ones so its labels didn't change. + if sp.IsNewService || oldSpec != nil { + var oldLabels, oldContainerLabels map[string]string + if oldSpec != nil { + oldLabels, oldContainerLabels = oldSpec.Labels, oldSpec.Container.Labels + } + specDiff += formatLabelsDiff("labels:", oldLabels, sp.Spec.Labels, sp.IsNewService) + specDiff += formatLabelsDiff("container labels:", oldContainerLabels, sp.Spec.Container.Labels, sp.IsNewService) + } + + treePrefix := tui.Faint.Render(" │ ") + for line := range strings.Lines(specDiff) { + out.WriteString(treePrefix + line) + } // Blank separator line before container operations. out.WriteString(tui.Faint.Render(" │")) @@ -272,6 +288,66 @@ func formatImageDiff(oldImage, newImage string) string { tui.FormatImage(newImage, tui.Green) } +// formatLabelsDiff renders a block with the attribute name followed by a nested line for each added, changed, or +// removed label, sorted by key. It returns an empty string if the labels are the same. +func formatLabelsDiff(attribute string, oldLabels, newLabels map[string]string, isNewService bool) string { + keys := slices.Concat(slices.Collect(maps.Keys(oldLabels)), slices.Collect(maps.Keys(newLabels))) + slices.Sort(keys) + + var lines strings.Builder + for _, key := range slices.Compact(keys) { + oldValue, inOld := oldLabels[key] + newValue, inNew := newLabels[key] + + var mod, diff string + switch { + case !inOld: + mod = tui.Green.Render("+") + // All attributes of a new service are new so only the green color marks them, without a modifier. + if isNewService { + mod = " " + } + diff = tui.Green.Render(key+tui.Faint.Render("=")) + tui.Green.Render(formatLabelValue(newValue)) + case !inNew: + mod = tui.Red.Render("-") + diff = tui.Red.Render(key+tui.Faint.Render("=")) + tui.Red.Render(formatLabelValue(oldValue)) + case oldValue != newValue: + mod = tui.Yellow.Render("~") + diff = key + + tui.Faint.Render("=") + + tui.Red.Render(formatLabelValue(oldValue)) + " " + + tui.Faint.Render("→") + " " + + tui.Green.Render(formatLabelValue(newValue)) + default: + continue + } + + lines.WriteString(" " + mod + " " + diff + "\n") + } + if lines.Len() == 0 { + return "" + } + + blockMod := tui.Yellow.Render("~") + switch { + case isNewService: + blockMod = " " + case len(newLabels) == 0: + blockMod = tui.Red.Render("-") + } + + return blockMod + " " + tui.Faint.Render(attribute) + "\n" + lines.String() +} + +// formatLabelValue returns the label value on a single line. It quotes and escapes empty values and values with +// non-printable characters such as newlines so that they're visible in the diff. Other values are returned as is. +func formatLabelValue(value string) string { + if value == "" || strings.ContainsFunc(value, func(r rune) bool { return !unicode.IsPrint(r) }) { + return strconv.Quote(value) + } + return value +} + // NewDeployment creates a new deployment for the given service specification. // If strategy is nil, a default RollingStrategy will be used. func NewDeployment(cli Client, spec api.ServiceSpec, strategy Strategy) *Deployment { diff --git a/pkg/client/deploy/deploy_test.go b/pkg/client/deploy/deploy_test.go index 5bf93d69..f481eb86 100644 --- a/pkg/client/deploy/deploy_test.go +++ b/pkg/client/deploy/deploy_test.go @@ -1,9 +1,11 @@ package deploy import ( + "strings" "testing" "github.com/charmbracelet/x/ansi" + "github.com/docker/docker/api/types/container" "github.com/psviderski/uncloud/pkg/api" "github.com/psviderski/uncloud/pkg/client/operation" "github.com/stretchr/testify/assert" @@ -59,3 +61,225 @@ func TestServicePlan_FormatSummary(t *testing.T) { }) } } + +func TestServicePlan_Format(t *testing.T) { + t.Parallel() + + newContainer := func(spec api.ServiceSpec) api.ServiceContainer { + return api.ServiceContainer{ + Container: api.Container{InspectResponse: container.InspectResponse{ + ContainerJSONBase: &container.ContainerJSONBase{ID: "0f6df6593e1b0123456789abcdef"}, + }}, + ServiceSpec: spec, + } + } + spec := func(image string, labels, containerLabels map[string]string) api.ServiceSpec { + return api.ServiceSpec{ + Name: "web", + Mode: api.ServiceModeReplicated, + Replicas: 1, + Labels: labels, + Container: api.ContainerSpec{Image: image, Labels: containerLabels}, + } + } + + tests := []struct { + name string + isNewService bool + spec api.ServiceSpec + ops func(spec api.ServiceSpec) []operation.Operation + expected []string + }{ + { + name: "service labels added, changed, and removed in place", + spec: spec("nginx:1.25", map[string]string{"env": "prod", "team": "platform", "same": "1"}, nil), + ops: func(api.ServiceSpec) []operation.Operation { + return []operation.Operation{&operation.UpdateContainerOperation{ + MachineName: "machine-1", + Container: newContainer(spec( + "nginx:1.25", map[string]string{"old": "legacy", "team": "backend", "same": "1"}, nil, + )), + }} + }, + expected: []string{ + "~ update service web", + " │ image: nginx:1.25", + " │ ~ labels:", + " │ + env=prod", + " │ - old=legacy", + " │ ~ team=backend → platform", + " │", + " ╰── ~ update container web/0f6df6593e1b on machine-1 (in place)", + }, + }, + { + name: "container labels changed with image on replace", + spec: spec("nginx:1.26", nil, map[string]string{"proxy": "on"}), + ops: func(new api.ServiceSpec) []operation.Operation { + return []operation.Operation{&operation.ReplaceContainerOperation{ + Spec: new, + MachineName: "machine-1", + OldContainer: newContainer(spec("nginx:1.25", nil, map[string]string{"proxy": "off"})), + }} + }, + expected: []string{ + "~ update service web", + " │ ~ image: nginx:1.25 → 1.26", + " │ ~ container labels:", + " │ ~ proxy=off → on", + " │", + " ╰── +/- replace container web/0f6df6593e1b on machine-1", + }, + }, + { + name: "no label blocks when labels are the same", + spec: spec("nginx:1.26", map[string]string{}, map[string]string{"proxy": "on"}), + ops: func(new api.ServiceSpec) []operation.Operation { + return []operation.Operation{&operation.ReplaceContainerOperation{ + Spec: new, + MachineName: "machine-1", + OldContainer: newContainer(spec("nginx:1.25", nil, map[string]string{"proxy": "on"})), + }} + }, + expected: []string{ + "~ update service web", + " │ ~ image: nginx:1.25 → 1.26", + " │", + " ╰── +/- replace container web/0f6df6593e1b on machine-1", + }, + }, + { + name: "replaced container determines old spec over updated one", + spec: spec("nginx:1.26", map[string]string{"team": "platform"}, nil), + ops: func(new api.ServiceSpec) []operation.Operation { + return []operation.Operation{ + &operation.UpdateContainerOperation{ + MachineName: "machine-1", + Container: newContainer(spec("nginx:1.26", map[string]string{"team": "backend"}, nil)), + }, + &operation.ReplaceContainerOperation{ + Spec: new, + MachineName: "machine-2", + OldContainer: newContainer(spec("nginx:1.25", map[string]string{"team": "backend"}, nil)), + }, + } + }, + expected: []string{ + "~ update service web", + " │ ~ image: nginx:1.25 → 1.26", + " │ ~ labels:", + " │ ~ team=backend → platform", + " │", + " ├── ~ update container web/0f6df6593e1b on machine-1 (in place)", + " ╰── +/- replace container web/0f6df6593e1b on machine-2", + }, + }, + { + name: "all labels removed", + spec: spec("nginx:1.25", map[string]string{}, nil), + ops: func(api.ServiceSpec) []operation.Operation { + return []operation.Operation{&operation.UpdateContainerOperation{ + MachineName: "machine-1", + Container: newContainer(spec("nginx:1.25", map[string]string{"a": "1", "b": "2"}, nil)), + }} + }, + expected: []string{ + "~ update service web", + " │ image: nginx:1.25", + " │ - labels:", + " │ - a=1", + " │ - b=2", + " │", + " ╰── ~ update container web/0f6df6593e1b on machine-1 (in place)", + }, + }, + { + name: "first labels added to existing service", + spec: spec("nginx:1.25", map[string]string{"a": "1"}, nil), + ops: func(api.ServiceSpec) []operation.Operation { + return []operation.Operation{&operation.UpdateContainerOperation{ + MachineName: "machine-1", + Container: newContainer(spec("nginx:1.25", nil, nil)), + }} + }, + expected: []string{ + "~ update service web", + " │ image: nginx:1.25", + " │ ~ labels:", + " │ + a=1", + " │", + " ╰── ~ update container web/0f6df6593e1b on machine-1 (in place)", + }, + }, + { + name: "empty and non-printable label values are escaped on a single line", + spec: spec("nginx:1.25", map[string]string{ + "added": "1\n2", + "cleared": "", + "json": `{"a":1}`, + "multi": "a\nb\nc", + "tab": "a\tb", + "trailing": "hello\n", + "unicode": "héllo wörld 🚀", + }, nil), + ops: func(api.ServiceSpec) []operation.Operation { + return []operation.Operation{&operation.UpdateContainerOperation{ + MachineName: "machine-1", + Container: newContainer(spec("nginx:1.25", map[string]string{ + "cleared": "x", + "multi": "a\nb", + "trailing": "hello", + }, nil)), + }} + }, + expected: []string{ + "~ update service web", + " │ image: nginx:1.25", + " │ ~ labels:", + ` │ + added="1\n2"`, + ` │ ~ cleared=x → ""`, + ` │ + json={"a":1}`, + ` │ ~ multi="a\nb" → "a\nb\nc"`, + ` │ + tab="a\tb"`, + ` │ ~ trailing=hello → "hello\n"`, + " │ + unicode=héllo wörld 🚀", + " │", + " ╰── ~ update container web/0f6df6593e1b on machine-1 (in place)", + }, + }, + { + name: "new service with labels", + isNewService: true, + spec: spec("nginx:1.25", map[string]string{"team": "platform"}, map[string]string{"proxy": "on"}), + ops: func(new api.ServiceSpec) []operation.Operation { + return []operation.Operation{&operation.RunContainerOperation{Spec: new, MachineName: "machine-1"}} + }, + expected: []string{ + "+ create service web", + " │ image: nginx:1.25", + " │ labels:", + " │ team=platform", + " │ container labels:", + " │ proxy=on", + " │", + " ╰── + run container web on machine-1", + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + plan := ServicePlan{ServiceName: "web", IsNewService: tt.isNewService, Spec: tt.spec} + plan.Operations = tt.ops(tt.spec) + + // The table pads cells to the column width so ignore trailing whitespace. + var lines []string + for line := range strings.Lines(ansi.Strip(plan.Format())) { + lines = append(lines, strings.TrimRight(line, " \n")) + } + assert.Equal(t, tt.expected, lines) + }) + } +}