diff --git a/cmd/sync/prompt.go b/cmd/sync/prompt.go index df449bea2..83d4e9783 100644 --- a/cmd/sync/prompt.go +++ b/cmd/sync/prompt.go @@ -22,6 +22,7 @@ const ( archiveFlag = "archive" conflictFlag = "conflict" contentFlag = "content" + diffFlag = "diff" dryRunFlag = "dry-run" formatFlag = "format" modelConfigKeyFlag = "model-config-key" @@ -93,6 +94,7 @@ func runPrompt(client resources.Client, buildAction actionBuilder) func(*cobra.C } yes, _ := cmd.Flags().GetBool(yesFlag) noInput, _ := cmd.Flags().GetBool(noInputFlag) + diff, _ := cmd.Flags().GetBool(diffFlag) outputKind := cliflags.GetOutputKind(cmd) err = syncprompt.NewRunner(client).Run(syncprompt.Options{ @@ -104,6 +106,7 @@ func runPrompt(client resources.Client, buildAction actionBuilder) func(*cobra.C ConflictPolicy: policy, Yes: yes, NoInput: noInput, + Diff: diff, Context: cmd.Context(), Input: cmd.InOrStdin(), Output: cmd.OutOrStdout(), @@ -157,6 +160,7 @@ func newAddCmd(client resources.Client) *cobra.Command { }), } cmd.Flags().Bool(dryRunFlag, false, "Preview local files without creating them") + cmd.Flags().Bool(diffFlag, false, "Show full diffs and file content in the plan") cmd.Flags().Bool(noInputFlag, false, "Fail instead of prompting for missing input") return cmd } @@ -269,6 +273,7 @@ func addApplyFlags(cmd *cobra.Command, dryRun bool) { if dryRun { cmd.Flags().Bool(dryRunFlag, false, "Preview synchronization changes without applying them") } + cmd.Flags().Bool(diffFlag, false, "Show full diffs in the plan") cmd.Flags().String(conflictFlag, "", "Default conflict resolution: launchdarkly, local, or abort") cmd.Flags().StringArray(resolveFlag, nil, "Resolve one variation conflict (project-key/config-key/variation-key=choice)") cmd.Flags().Bool(yesFlag, false, "Apply changes without confirmation") diff --git a/cmd/sync/prompt_test.go b/cmd/sync/prompt_test.go index 702961d18..bf7c20086 100644 --- a/cmd/sync/prompt_test.go +++ b/cmd/sync/prompt_test.go @@ -64,6 +64,33 @@ func TestPromptCommandHelpIncludesExamples(t *testing.T) { } } +func TestPromptCommandsRegisterDiffFlag(t *testing.T) { + command := NewPromptCmd(nil) + tests := map[string]struct { + path []string + help string + }{ + "prompts": {help: "Show full diffs in the plan"}, + "watch": {path: []string{"watch"}, help: "Show full diffs in the plan"}, + "add": {path: []string{"add"}, help: "Show full diffs and file content in the plan"}, + } + + for name, test := range tests { + t.Run(name, func(t *testing.T) { + target := command + if len(test.path) != 0 { + var err error + target, _, err = command.Find(test.path) + require.NoError(t, err) + } + + flag := target.Flags().Lookup(diffFlag) + require.NotNil(t, flag) + assert.Equal(t, test.help, flag.Usage) + }) + } +} + func TestAttachSubcommandsAcceptOptionalKeyAndTarget(t *testing.T) { command := NewPromptCmd(nil) tool, _, err := command.Find([]string{"attach", "tool"}) diff --git a/internal/sync/bootstrap/bootstrap.go b/internal/sync/bootstrap/bootstrap.go index 3eefd8d71..86f1e23bf 100644 --- a/internal/sync/bootstrap/bootstrap.go +++ b/internal/sync/bootstrap/bootstrap.go @@ -3,6 +3,7 @@ package bootstrap import ( + "bytes" "cmp" "errors" "fmt" @@ -42,6 +43,8 @@ type Options struct { // Initial is true when the workspace has no .launchdarkly directory. Initial bool DryRun bool + // Diff shows the content of each file that a dry run lists. + Diff bool // Selections are the variations to add. If it is empty, Run asks the user. Selections []syncdomain.ResourceID NoInput bool @@ -228,7 +231,7 @@ func finishSelection(options Options, files []synclocal.VariationFile) error { if err != nil { return err } - writePreviews(options.Output, previews) + writePreviews(options.Output, previews, options.Diff) return nil } @@ -323,23 +326,45 @@ func writeSummary(output io.Writer, initial bool, count int) { _ = syncconsole.New(output).Printf("%s %d %s in %s.\n", verb, count, noun, syncdomain.RootDir) } -// writePreviews prints each file that a dry run would create. -func writePreviews(output io.Writer, previews []synclocal.RenderedFile) { +// writePreviews lists each file that the add creates, with its number of +// lines. With showContent, the content of each file follows the list. +func writePreviews(output io.Writer, previews []synclocal.RenderedFile, showContent bool) { console := syncconsole.New(output) - const border = "============================================================" + noun := "file" + if len(previews) != 1 { + noun += "s" + } + _ = console.Printf("%d %s to create:\n", len(previews), noun) + + paths := make([]string, len(previews)) + width := 0 for index, preview := range previews { - if index != 0 { - _ = console.Line("") - } - _ = console.Line(border) - _ = console.Printf( - "File %d of %d\nWould create: %s\n", index+1, len(previews), path.Join(syncdomain.RootDir, preview.Path), - ) - _ = console.Line("------------------------------------------------------------") - _ = console.WriteBytes(preview.Content) - if len(preview.Content) == 0 || preview.Content[len(preview.Content)-1] != '\n' { - _ = console.Line("") + paths[index] = path.Join(syncdomain.RootDir, preview.Path) + width = max(width, len(paths[index])) + } + for index, preview := range previews { + _ = console.Printf(" %-*s +%d\n", width, paths[index], lineCount(preview.Content)) + } + if !showContent { + return + } + for index, preview := range previews { + _ = console.Printf("\n%s\n", paths[index]) + for _, line := range strings.Split(strings.TrimSuffix(string(preview.Content), "\n"), "\n") { + if line != "" { + line = " " + line + } + _ = console.Line(line) } - _ = console.Line(border) } } + +// lineCount returns the number of lines in a file. A last line without a +// final newline counts. +func lineCount(content []byte) int { + count := bytes.Count(content, []byte("\n")) + if len(content) != 0 && content[len(content)-1] != '\n' { + count++ + } + return count +} diff --git a/internal/sync/bootstrap/bootstrap_test.go b/internal/sync/bootstrap/bootstrap_test.go index 0dd068072..55ebef9e9 100644 --- a/internal/sync/bootstrap/bootstrap_test.go +++ b/internal/sync/bootstrap/bootstrap_test.go @@ -111,7 +111,7 @@ func TestRunUsesExplicitVariationSelectorsWithoutTerminal(t *testing.T) { }) require.NoError(t, err) - assert.Contains(t, output.String(), "Would create: .launchdarkly/project/configs/config/variation.prompt.md") + assert.Contains(t, output.String(), " .launchdarkly/project/configs/config/variation.prompt.md +9") } func TestFinishSelectionDryRunDoesNotCreateFiles(t *testing.T) { @@ -138,20 +138,8 @@ func TestFinishSelectionDryRunDoesNotCreateFiles(t *testing.T) { require.NoError(t, err) assert.Equal( t, - `============================================================ -File 1 of 1 -Would create: .launchdarkly/project/configs/config/variation.prompt.md ------------------------------------------------------------- ---- -formatVersion: 1 -upsert: true -mode: agent -key: variation -name: Variation ---- - -Be helpful. -============================================================ + `1 file to create: + .launchdarkly/project/configs/config/variation.prompt.md +9 `, output.String(), ) @@ -384,32 +372,32 @@ func TestWriteSummary(t *testing.T) { assert.Contains(t, output.String(), "No variations added") } -func TestWritePreviewsPrintsEveryFile(t *testing.T) { - var output bytes.Buffer - - writePreviews(&output, []synclocal.RenderedFile{ - {Path: "project/configs/config/first.prompt.md", Content: []byte("first\n")}, - {Path: "project/configs/config/second.prompt.md", Content: []byte("second\n")}, - }) +func TestWritePreviewsListsEveryFileWithItsSize(t *testing.T) { + previews := []synclocal.RenderedFile{ + {Path: "project/tools/search.json", Content: []byte("{\n}")}, + {Path: "project/configs/config/first.prompt.md", Content: []byte("---\nkey: first\n---\n\nfirst\n")}, + } - assert.Equal( - t, - `============================================================ -File 1 of 2 -Would create: .launchdarkly/project/configs/config/first.prompt.md ------------------------------------------------------------- -first -============================================================ - -============================================================ -File 2 of 2 -Would create: .launchdarkly/project/configs/config/second.prompt.md ------------------------------------------------------------- -second -============================================================ -`, - output.String(), - ) + var list, content bytes.Buffer + writePreviews(&list, previews, false) + writePreviews(&content, previews, true) + + assert.Equal(t, `2 files to create: + .launchdarkly/project/tools/search.json +2 + .launchdarkly/project/configs/config/first.prompt.md +5 +`, list.String()) + assert.Equal(t, list.String()+` +.launchdarkly/project/tools/search.json + { + } + +.launchdarkly/project/configs/config/first.prompt.md + --- + key: first + --- + + first +`, content.String()) } type fakeCatalog struct { diff --git a/internal/sync/prompt/acceptance_test.go b/internal/sync/prompt/acceptance_test.go index 70e4d3811..53736e44d 100644 --- a/internal/sync/prompt/acceptance_test.go +++ b/internal/sync/prompt/acceptance_test.go @@ -782,7 +782,7 @@ func TestPromptStaleWorkingCopyPullsInsteadOfReverting(t *testing.T) { assert.False(t, slices.ContainsFunc(api.requests, func(request string) bool { return strings.HasPrefix(request, "PATCH ") && strings.Contains(request, "/variations/") }), "sync wrote the older local file back to LaunchDarkly") - assert.Contains(t, review, "Another working copy synced this variation after your sync.lock.") + assert.Contains(t, review, "Note: Another working copy synced variation production/support/default after your sync.lock.") resources, err := synclocal.CompileWorkspace(root) require.NoError(t, err) require.Len(t, resources, 1) diff --git a/internal/sync/prompt/action.go b/internal/sync/prompt/action.go index 8549479f5..da3c0f2f2 100644 --- a/internal/sync/prompt/action.go +++ b/internal/sync/prompt/action.go @@ -92,7 +92,10 @@ type Options struct { // Yes applies the plan without a confirmation prompt. Yes bool // NoInput makes each prompt for missing input an error. - NoInput bool + NoInput bool + // Diff shows the full diff of each planned change, and the content of + // each file that add creates. + Diff bool Context context.Context Input io.Reader Output io.Writer diff --git a/internal/sync/prompt/conflict.go b/internal/sync/prompt/conflict.go index 06beaae72..aa2ff9503 100644 --- a/internal/sync/prompt/conflict.go +++ b/internal/sync/prompt/conflict.go @@ -6,6 +6,7 @@ import ( "fmt" "io" "slices" + "strings" syncconsole "github.com/launchdarkly/ldcli/internal/sync/console" syncinteractive "github.com/launchdarkly/ldcli/internal/sync/interactive" @@ -77,8 +78,7 @@ var errConflictAborted = errors.New("sync conflict left unresolved") // conflictGroup is one conflict choice. Variations that share a changed // attachment are in one group, because the attachment can have only one state. type conflictGroup struct { - resources []PlannedResource - attachment bool + resources []PlannedResource } type conflictResolutionResult struct { @@ -107,6 +107,7 @@ func resolveConflicts( return conflictResolutionResult{}, nil } + _ = syncconsole.New(options.ErrorOutput).Line(planSummary(plan)) result := conflictResolutionResult{resolutions: make(map[ResourceID]ConflictResolution)} for _, group := range groups { if err := writeConflict(options.ErrorOutput, group); err != nil { @@ -149,23 +150,33 @@ func resolveConflicts( return result, nil } -// writeConflict shows the diff of each resource in a group that has one. +// writeConflict shows one line that names the resources of the group, and +// then the diff of each resource that has one. func writeConflict(output io.Writer, group conflictGroup) error { - visible := slices.DeleteFunc(slices.Clone(group.resources), func(resource PlannedResource) bool { - return len(resource.Diff) == 0 - }) - if len(visible) == 0 { - visible = group.resources[:1] - } - if err := writePlanReview(output, "plaintext", Plan{Resources: visible}, terminalWidth(output)); err != nil { - return err + console := syncconsole.New(output) + header := fmt.Sprintf("\nConflict: %s changed on both sides.", group.resources[0].ID.Label()) + if others := group.resources[1:]; len(others) != 0 { + labels := make([]string, 0, len(others)) + for _, resource := range others { + labels = append(labels, resource.ID.Label()) + } + header += fmt.Sprintf(" It shares a changed tool or skill with %s, so one choice covers them all.", strings.Join(labels, ", ")) } - if group.attachment && len(group.resources) > 1 { - console := syncconsole.New(output) - _ = console.Printf("This attachment conflict affects %d variations:\n", len(group.resources)) - for _, resource := range group.resources { - _ = console.Printf("- %s\n", resource.ID) + _ = console.Line(header) + + width := terminalWidth(output) + for _, resource := range group.resources { + if len(resource.Diff) == 0 { + continue + } + if len(group.resources) > 1 { + _ = console.Printf("\n %s\n", resource.ID.Label()) + } + rendered, err := renderVariationDiff(resource.Diff, outputPlaintext, width, diffPresentation(resource.Action)) + if err != nil { + return err } + _ = console.Write(rendered) } return nil } @@ -194,7 +205,7 @@ func groupConflicts(plan Plan) []conflictGroup { } } - group := conflictGroup{attachment: len(members) > 1} + var group conflictGroup for _, index := range members { group.resources = append(group.resources, plan.Resources[index]) } diff --git a/internal/sync/prompt/conflict_test.go b/internal/sync/prompt/conflict_test.go index d00ab37a5..ce80552b7 100644 --- a/internal/sync/prompt/conflict_test.go +++ b/internal/sync/prompt/conflict_test.go @@ -139,6 +139,25 @@ func TestResolveConflictsShowsDiffBeforePrompt(t *testing.T) { assert.Less(t, strings.Index(rendered, "LaunchDarkly now"), strings.Index(rendered, "Using local.")) } +func TestWriteConflictNamesTheResourcesThatOneChoiceCovers(t *testing.T) { + conflict := divergentPlan(t).Resources[0] + shared := conflict + shared.ID.LookupKey = "support/shared" + shared.Action = ActionUpdateServer + + var single, group bytes.Buffer + require.NoError(t, writeConflict(&single, conflictGroup{resources: []PlannedResource{conflict}})) + require.NoError(t, writeConflict(&group, conflictGroup{resources: []PlannedResource{conflict, shared}})) + + assert.True(t, strings.HasPrefix(single.String(), + "\nConflict: variation production/support/default changed on both sides.\n\n Variation content (changed)\n")) + assert.True(t, strings.HasPrefix(group.String(), + "\nConflict: variation production/support/default changed on both sides. "+ + "It shares a changed tool or skill with variation production/support/shared, so one choice covers them all.\n")) + assert.Contains(t, group.String(), "\n variation production/support/shared\n") + assert.Equal(t, 2, strings.Count(group.String(), "Variation content (changed)")) +} + func TestResolveConflictsRequiresTerminalEvenWithYes(t *testing.T) { plan := divergentPlan(t) input := strings.NewReader("") @@ -214,7 +233,6 @@ func TestGroupConflictsDeduplicatesSharedAttachment(t *testing.T) { groups := groupConflicts(Plan{Resources: []PlannedResource{first, second}}) require.Len(t, groups, 1) - assert.True(t, groups[0].attachment) assert.Len(t, groups[0].resources, 2) } @@ -247,7 +265,6 @@ func TestGroupConflictsConnectsMixedChangesAcrossSharedAttachments(t *testing.T) groups := groupConflicts(Plan{Resources: resources}) require.Len(t, groups, 1) - assert.True(t, groups[0].attachment) assert.Len(t, groups[0].resources, 4) } diff --git a/internal/sync/prompt/output.go b/internal/sync/prompt/output.go index 1714c4e6b..9aa605a91 100644 --- a/internal/sync/prompt/output.go +++ b/internal/sync/prompt/output.go @@ -1,12 +1,18 @@ package prompt import ( + "bytes" + "cmp" "encoding/json" "fmt" "io" + "maps" + "slices" + "strings" "github.com/charmbracelet/lipgloss" + syncdomain "github.com/launchdarkly/ldcli/internal/sync" syncconsole "github.com/launchdarkly/ldcli/internal/sync/console" ) @@ -45,14 +51,16 @@ func newResourceOutput(id ResourceID) resourceOutput { return resourceOutput{ResourceKind: string(id.Kind), ProjectKey: id.ProjectKey, LookupKey: id.LookupKey} } -// writePlanOutput writes the plan in the selected output kind. -func writePlanOutput(out io.Writer, outputKind string, plan Plan) error { +// writePlanOutput writes the plan in the selected output kind. Plain text and +// Markdown show a full diff for a conflict, and for every row when showDiffs +// is true. +func writePlanOutput(out io.Writer, outputKind string, plan Plan, showDiffs bool) error { outputKind, err := checkOutputKind(outputKind) if err != nil { return err } if outputKind != outputJSON { - return writePlanReview(out, outputKind, plan, terminalWidth(out)) + return writePlanReview(out, outputKind, plan, terminalWidth(out), showDiffs) } type planResourceOutput struct { @@ -77,8 +85,10 @@ func writePlanOutput(out io.Writer, outputKind string, plan Plan) error { return writeJSON(out, map[string]any{"resources": resources}) } -// writeOutcomeOutput writes the result of each action, which includes failures. -func writeOutcomeOutput(out io.Writer, outputKind string, outcomes []ResourceOutcome) error { +// writeOutcomeOutput writes the result of each action, which includes +// failures. Plain text and Markdown take the reason of each result from the +// plan that produced the outcomes. +func writeOutcomeOutput(out io.Writer, outputKind string, plan Plan, outcomes []ResourceOutcome) error { outputKind, err := checkOutputKind(outputKind) if err != nil { return err @@ -95,18 +105,37 @@ func writeOutcomeOutput(out io.Writer, outputKind string, outcomes []ResourceOut return writeJSON(out, map[string]any{"resources": resources}) } - console := syncconsole.New(out) - if outputKind == outputMarkdown { - _ = console.Line("## Sync results") - } else { - _ = console.Line("Sync results:") + planned := make(map[ResourceID]PlannedResource, len(plan.Resources)) + for _, resource := range plan.Resources { + planned[resource.ID] = resource } + var rows []reviewRow + var failures []ResourceOutcome + inSync := 0 for _, outcome := range outcomes { - _ = console.Printf("- %s action=%s status=%s\n", outcome.ID, outcome.Action, outcome.Status) - if outcome.Error != "" { - _ = console.Printf(" Error: %s\n", outcome.Error) + resource := planned[outcome.ID] + switch { + case outcome.Status != OutcomeSucceeded: + failures = append(failures, outcome) + case outcome.Action == ActionInSync: + inSync++ + default: + rows = append(rows, reviewRow{ + kind: outcome.ID.Kind, + cells: []string{actionWordFor(resource).result, outcome.ID.String(), planReason(resource)}, + }) } } + + console := syncconsole.New(out) + if len(rows) != 0 { + writeRows(console, outputKind, []string{"Result", "Resource", "Reason"}, rows, terminalWidth(out)) + _ = console.Line("") + } + for _, failure := range failures { + _ = console.Printf("Failed: %s: %s\n", failure.ID.Label(), failure.Error) + } + _ = console.Line(resultTotals(len(rows), len(failures), inSync)) return nil } @@ -122,68 +151,375 @@ func checkOutputKind(outputKind string) (string, error) { } } -// writePlanReview writes the plan for a person to read. It groups the -// resources by project and config, and shows each action, error, and diff. -func writePlanReview(out io.Writer, outputKind string, plan Plan, width int) error { +// writePlanReview writes the plan for a person to read: a summary line, and +// then one row for each change under a heading for its resource kind. Rows +// that are in sync count only in the summary. Each row shows the size of its +// diff. The full diff follows a conflict, and every row when showDiffs is true. +func writePlanReview(out io.Writer, outputKind string, plan Plan, width int, showDiffs bool) error { console := syncconsole.New(out) if len(plan.Resources) == 0 { - _ = console.Line("No prompt variations are tracked.") + _ = console.Line("No prompt variations are synced in this repository. To start, run ldcli sync prompts add.") return nil } - markdown := outputKind == outputMarkdown - detail := func(label, text string) { - if markdown { - _ = console.Printf("%s: %s\n", label, text) - } else { - _ = console.Printf(" %s: %s\n", label, text) + _ = console.Line(planSummary(plan)) + + color := outputKind != outputMarkdown && width > 0 + var rows []reviewRow + for _, resource := range plan.Resources { + if resource.Action == ActionInSync { + continue + } + presentation := diffPresentation(resource.Action) + added, removed, err := diffSize(resource.Diff, presentation) + if err != nil { + return err + } + row := reviewRow{ + kind: resource.ID.Kind, + cells: []string{actionWordFor(resource).plan, resource.ID.String(), planReason(resource), diffSizeText(added, removed, color)}, + } + if len(resource.Diff) != 0 && (showDiffs || resource.Action == ActionConflict) { + if row.diff, err = renderVariationDiff(resource.Diff, outputKind, width, presentation); err != nil { + return err + } } + rows = append(rows, row) } + if len(rows) != 0 { + _ = console.Line("") + writeRows(console, outputKind, []string{"Action", "Resource", "Reason", "Size"}, rows, width) + } + return nil +} - currentProject, currentConfig := "", "" +// writePlanNotes writes one line for each resource that another working copy +// synced after this working copy wrote its sync.lock file. +func writePlanNotes(out io.Writer, plan Plan) { + console := syncconsole.New(out) for _, resource := range plan.Resources { - if resource.ID.ProjectKey != currentProject { - if currentProject != "" { - _ = console.Line("") - } - currentProject, currentConfig = resource.ID.ProjectKey, "" - if markdown { - _ = console.Printf("## Project `%s`\n", currentProject) - } else { - _ = console.Printf("%s\n", reviewHeading("Project: "+currentProject, width)) - } + if resource.SyncedElsewhere { + _ = console.Printf( + "Note: Another working copy synced %s after your sync.lock. %s\n", resource.ID.Label(), staleSuggestion(resource), + ) } + } +} - action := actionDescription(resource.Action) - configKey, variationKey, err := resource.ID.VariationKeys() - switch { - case markdown: - _ = console.Printf("\n### Variation `%s`\n\nAction: **%s**\n", resource.ID.LookupKey, action) - case err != nil: - _ = console.Printf("\n %s\n Action: %s\n", reviewHeading("Variation: "+resource.ID.LookupKey, width), action) +// planSummary describes the plan in one line, for example +// "3 changes: 1 prompt push, 2 prompt pull. 1 conflict. 4 in sync." +func planSummary(plan Plan) string { + type change struct { + kind syncdomain.Kind + word int + } + counts := make(map[change]int) + var changes, conflicts, failures, inSync int + for _, resource := range plan.Resources { + switch resource.Action { + case ActionInSync: + inSync++ + case ActionConflict: + conflicts++ + case ActionError: + failures++ default: - if configKey != currentConfig { - currentConfig = configKey - _ = console.Printf("\n %s\n", reviewHeading("Config: "+configKey, width)) + changes++ + counts[change{kind: resource.ID.Kind, word: actionWordIndex(resource)}]++ + } + } + + var clauses []string + if changes != 0 { + var phrases []string + for _, key := range slices.SortedFunc(maps.Keys(counts), func(left, right change) int { + return cmp.Or(strings.Compare(string(left.kind), string(right.kind)), cmp.Compare(left.word, right.word)) + }) { + phrases = append(phrases, fmt.Sprintf("%d %s %s", counts[key], kindNoun(key.kind), actionWords[key.word].plan)) + } + clauses = append(clauses, fmt.Sprintf("%s: %s.", countOf(changes, "change"), strings.Join(phrases, ", "))) + } + if conflicts != 0 { + clauses = append(clauses, countOf(conflicts, "conflict")+".") + } + if failures != 0 { + clauses = append(clauses, countOf(failures, "error")+".") + } + if len(clauses) == 0 { + clauses = append(clauses, "No changes.") + } + if inSync != 0 { + clauses = append(clauses, fmt.Sprintf("%d in sync.", inSync)) + } + return strings.Join(clauses, " ") +} + +// resultTotals describes the results in one line, for example +// "3 applied, 1 failed. 4 in sync." +func resultTotals(applied, failed, inSync int) string { + totals := fmt.Sprintf("%d applied", applied) + if failed != 0 { + totals += fmt.Sprintf(", %d failed", failed) + } + totals += "." + if inSync != 0 { + totals += fmt.Sprintf(" %d in sync.", inSync) + } + return totals +} + +// actionWord is how a plan row and a result row name one action. A push that +// changes only attachment pins is a repin. +type actionWord struct { + action Action + repin bool + plan string + result string +} + +// actionWords lists the word of each action. A plan summary counts the +// changes in this order. +var actionWords = []actionWord{ + {action: ActionCreateServer, plan: "create", result: "created"}, + {action: ActionUpdateServer, plan: "push", result: "pushed"}, + {action: ActionUpdateServer, repin: true, plan: "repin", result: "repinned"}, + {action: ActionUpdateLocal, plan: "pull", result: "pulled"}, + {action: ActionDeleteLocal, plan: "delete", result: "deleted"}, + {action: ActionUpdateManifest, plan: "record", result: "recorded"}, + {action: ActionRemoveManifest, plan: "forget", result: "forgot"}, + {action: ActionConflict, plan: "conflict", result: "not applied"}, + {action: ActionError, plan: "error", result: "not applied"}, +} + +func actionWordIndex(resource PlannedResource) int { + repin := repinsOnly(resource) + return slices.IndexFunc(actionWords, func(word actionWord) bool { + return word.action == resource.Action && word.repin == repin + }) +} + +func actionWordFor(resource PlannedResource) actionWord { + if index := actionWordIndex(resource); index >= 0 { + return actionWords[index] + } + return actionWord{} +} + +// repinsOnly reports whether a push changes only the tool and skill versions +// that LaunchDarkly pins, and no content. +func repinsOnly(resource PlannedResource) bool { + return resource.Action == ActionUpdateServer && + resource.ServerHasStaleAttachmentPins && + resource.LocalFingerprint == resource.ServerFingerprint +} + +// planReason gives the short reason of a plan row, for example +// "instructions changed in LaunchDarkly". +func planReason(resource PlannedResource) string { + switch { + case resource.Action == ActionCreateServer: + return "new" + case repinsOnly(resource): + return repinReason(resource.Diff["attachment versions"]) + case resource.Action == ActionUpdateServer: + return changedFieldsReason(resource.Diff, "changed") + case resource.Action == ActionUpdateLocal && resource.Local == nil: + return "local file missing" + case resource.Action == ActionUpdateLocal: + return changedFieldsReason(resource.Diff, "changed in LaunchDarkly") + case resource.Action == ActionDeleteLocal: + return "deleted in LaunchDarkly" + case resource.Action == ActionUpdateManifest: + return "same as LaunchDarkly" + case resource.Action == ActionRemoveManifest: + return "gone on both sides" + case resource.Action == ActionConflict: + return "changed on both sides" + case resource.Action == ActionError: + return shortCause(resource.Error) + default: + return "" + } +} + +// changedFieldsReason names the fields that a diff changes, for example +// "instructions, tools changed". +func changedFieldsReason(diff variationDiffFields, verb string) string { + var fields []string + if part, ok := diff["variation"]; ok { + fields = append(fields, changedKeys(part.Before, part.After)...) + } + for _, part := range []string{"tools", "skills"} { + if _, ok := diff[part]; ok { + fields = append(fields, part) + } + } + if len(fields) == 0 { + return verb + } + return strings.Join(fields, ", ") + " " + verb +} + +// changedKeys returns the top-level keys of two JSON objects whose values differ. +func changedKeys(before, after json.RawMessage) []string { + var beforeFields, afterFields map[string]json.RawMessage + _ = json.Unmarshal(before, &beforeFields) + _ = json.Unmarshal(after, &afterFields) + keys := slices.Collect(maps.Keys(beforeFields)) + for key := range afterFields { + if _, ok := beforeFields[key]; !ok { + keys = append(keys, key) + } + } + keys = slices.DeleteFunc(keys, func(key string) bool { return bytes.Equal(beforeFields[key], afterFields[key]) }) + slices.Sort(keys) + return keys +} + +// repinReason names each tool and skill that a repin moves, for example +// "search to v4". +func repinReason(versions variationFieldDiff) string { + var latest map[string]map[string]int + _ = json.Unmarshal(versions.After, &latest) + var pins []string + for _, group := range []string{"tools", "skills"} { + for _, key := range slices.Sorted(maps.Keys(latest[group])) { + pins = append(pins, fmt.Sprintf("%s to v%d", key, latest[group][key])) + } + } + return strings.Join(pins, ", ") +} + +// shortCause returns the first clause of an error message, so that a plan +// row stays short. Plan.BlockingError, which follows the plan, has the full +// error of every row. +func shortCause(message string) string { + for _, separator := range []string{"; ", ". "} { + message, _, _ = strings.Cut(message, separator) + } + return message +} + +// diffSizeText formats a diff size, for example "+3 -1". A zero part is left +// out. When color is true, each part has the color of its diff lines. +func diffSizeText(added, removed int, color bool) string { + var parts []string + if added != 0 { + parts = append(parts, styleDiffLine(fmt.Sprintf("+%d", added), color)) + } + if removed != 0 { + parts = append(parts, styleDiffLine(fmt.Sprintf("-%d", removed), color)) + } + return strings.Join(parts, " ") +} + +// reviewRow is one row of a plan or of its results. The cells are the action +// word, the resource, the reason, and for a plan the diff size. diff is the +// rendered diff that follows the row, or empty. +type reviewRow struct { + kind syncdomain.Kind + cells []string + diff string +} + +// writeRows writes the rows under one heading for each resource kind. The rows +// must be in plan order, which keeps each kind together. +func writeRows(console syncconsole.Writer, outputKind string, headers []string, rows []reviewRow, width int) { + var groups [][]reviewRow + for start := 0; start < len(rows); { + end := start + 1 + for end < len(rows) && rows[end].kind == rows[start].kind { + end++ + } + groups = append(groups, rows[start:end]) + start = end + } + if outputKind == outputMarkdown { + writeMarkdownRows(console, headers, groups) + } else { + writeTextRows(console, len(headers), groups, width) + } +} + +// writeTextRows lines up the columns of every row. Each diff follows its row, +// with a blank line before the next row. +func writeTextRows(console syncconsole.Writer, columns int, groups [][]reviewRow, width int) { + widths := make([]int, columns) + for _, group := range groups { + for _, row := range group { + for column, cell := range row.cells { + widths[column] = max(widths[column], lipgloss.Width(cell)) } - _ = console.Printf("\n %s\n Action: %s\n", reviewHeading("Variation: "+variationKey, width), action) } + } - if resource.SyncedElsewhere { - detail("Note", "Another working copy synced this variation after your sync.lock.") - detail("Suggestion", staleSuggestion(resource)) + afterDiff := false + for _, group := range groups { + if afterDiff { + _ = console.Line("") } - if resource.Error != "" { - detail("Error", resource.Error) + _ = console.Line(reviewHeading(kindHeading(group[0].kind), width)) + for index, row := range group { + if index > 0 && group[index-1].diff != "" { + _ = console.Line("") + } + var line strings.Builder + for column, cell := range row.cells { + line.WriteString(cell) + if column < len(row.cells)-1 { + line.WriteString(strings.Repeat(" ", widths[column]-lipgloss.Width(cell)+3)) + } + } + _ = console.Printf(" %s\n", strings.TrimRight(line.String(), " ")) + _ = console.Write(row.diff) } - if len(resource.Diff) != 0 { - rendered, err := renderVariationDiff(resource.Diff, outputKind, width, diffPresentation(resource.Action)) - if err != nil { - return err + afterDiff = group[len(group)-1].diff != "" + } +} + +// writeMarkdownRows writes one table for each kind. The diffs follow the table. +func writeMarkdownRows(console syncconsole.Writer, headers []string, groups [][]reviewRow) { + for index, group := range groups { + if index > 0 { + _ = console.Line("") + } + _ = console.Printf("### %s\n\n| %s |\n|%s\n", + kindHeading(group[0].kind), strings.Join(headers, " | "), strings.Repeat(" --- |", len(headers))) + for _, row := range group { + cells := make([]string, len(row.cells)) + for column, cell := range row.cells { + cells[column] = strings.ReplaceAll(cell, "|", `\|`) + } + _ = console.Printf("| %s |\n", strings.Join(cells, " | ")) + } + for _, row := range group { + if row.diff != "" { + _ = console.Printf("\n#### `%s`\n", row.cells[1]) + _ = console.Write(row.diff) } - _ = console.Write(rendered) } } - return nil +} + +// kindNoun names one resource of a kind in plain text. +func kindNoun(kind syncdomain.Kind) string { + if kind == syncdomain.KindVariation { + return "prompt" + } + return string(kind) +} + +// kindHeading is the heading above the rows of one kind, for example "Prompts". +func kindHeading(kind syncdomain.Kind) string { + noun := kindNoun(kind) + return strings.ToUpper(noun[:1]) + noun[1:] + "s" +} + +// countOf formats a count with its noun, for example "1 change" or "2 changes". +func countOf(count int, noun string) string { + if count != 1 { + noun += "s" + } + return fmt.Sprintf("%d %s", count, noun) } // staleSuggestion tells the user what to do when another working copy synced @@ -211,32 +547,6 @@ func reviewHeading(value string, width int) string { return lipgloss.NewStyle().Foreground(lipgloss.Color("67")).Bold(true).Render(value) } -// actionDescription describes an action for the user. -func actionDescription(action Action) string { - switch action { - case ActionInSync: - return "No change (in sync)" - case ActionCreateServer: - return "Create the variation in LaunchDarkly" - case ActionUpdateServer: - return "Update LaunchDarkly from the local file" - case ActionUpdateLocal: - return "Update the local file from LaunchDarkly" - case ActionDeleteLocal: - return "Delete the local file" - case ActionUpdateManifest: - return "Record the matching state in the manifest" - case ActionRemoveManifest: - return "Remove the deleted resource from the manifest" - case ActionConflict: - return "Resolve the conflict before syncing" - case ActionError: - return "Fix the resource before syncing" - default: - return string(action) - } -} - // variationDiffPresentation is the labels and the direction of a diff. type variationDiffPresentation struct { beforeLabel string diff --git a/internal/sync/prompt/output_test.go b/internal/sync/prompt/output_test.go index be3c7e6aa..66dc1df42 100644 --- a/internal/sync/prompt/output_test.go +++ b/internal/sync/prompt/output_test.go @@ -2,9 +2,14 @@ package prompt import ( "bytes" + "encoding/json" + "io" + "os" "strings" "testing" + "github.com/charmbracelet/lipgloss" + "github.com/charmbracelet/x/ansi" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -16,16 +21,296 @@ func TestWritePlanAndOutcomeJSON(t *testing.T) { plan := Plan{Resources: []PlannedResource{{ID: id, Action: ActionUpdateServer}}} var output bytes.Buffer - require.NoError(t, writePlanOutput(&output, "json", plan)) + require.NoError(t, writePlanOutput(&output, "json", plan, false)) assert.JSONEq(t, `{"resources":[{"resourceKind":"variation","projectKey":"production","lookupKey":"support/default","action":"update_server"}]}`, output.String()) output.Reset() - require.NoError(t, writeOutcomeOutput(&output, "json", []ResourceOutcome{{ + require.NoError(t, writeOutcomeOutput(&output, "json", plan, []ResourceOutcome{{ ID: id, Action: ActionUpdateServer, Status: OutcomeSucceeded, }})) assert.JSONEq(t, `{"resources":[{"resourceKind":"variation","projectKey":"production","lookupKey":"support/default","action":"update_server","status":"succeeded"}]}`, output.String()) } +// The JSON output is the stable format for scripts. The plain-text layout can +// change, but this output must not change by accident. +func TestPlanAndOutcomeJSONIsStable(t *testing.T) { + server, local := testVariation("Support"), testVariation("Support") + local.Instructions = "Help briefly" + pushed, stale, broken := testResourceID(), testResourceID(), testResourceID() + stale.LookupKey, broken.LookupKey = "support/stale", "support/broken" + plan := Plan{Resources: []PlannedResource{ + {ID: pushed, Action: ActionUpdateServer, Diff: variationDiff(&server, &local)}, + {ID: stale, Action: ActionUpdateLocal, SyncedElsewhere: true}, + {ID: broken, Action: ActionError, Error: "local mode \"agent\" does not match config mode \"completion\""}, + }} + + var output bytes.Buffer + require.NoError(t, writePlanOutput(&output, "json", plan, true)) + assert.Equal(t, `{ + "resources": [ + { + "resourceKind": "variation", + "projectKey": "production", + "lookupKey": "support/default", + "action": "update_server", + "diff": { + "variation": { + "before": { + "mode": "agent", + "key": "default", + "name": "Support", + "instructions": "Help" + }, + "after": { + "mode": "agent", + "key": "default", + "name": "Support", + "instructions": "Help briefly" + } + } + } + }, + { + "resourceKind": "variation", + "projectKey": "production", + "lookupKey": "support/stale", + "action": "update_local", + "syncedElsewhere": true, + "suggestion": "If the other working copy pushed its change to Git, run git pull before you sync. Then your sync.lock does not get a merge conflict." + }, + { + "resourceKind": "variation", + "projectKey": "production", + "lookupKey": "support/broken", + "action": "error", + "error": "local mode \"agent\" does not match config mode \"completion\"" + } + ] +} +`, output.String()) + + output.Reset() + require.NoError(t, writeOutcomeOutput(&output, "json", plan, []ResourceOutcome{ + {ID: pushed, Action: ActionUpdateServer, Status: OutcomeSucceeded}, + {ID: stale, Action: ActionUpdateLocal, Status: OutcomeFailed, Error: "write failed"}, + })) + assert.Equal(t, `{ + "resources": [ + { + "resourceKind": "variation", + "projectKey": "production", + "lookupKey": "support/default", + "action": "update_server", + "status": "succeeded" + }, + { + "resourceKind": "variation", + "projectKey": "production", + "lookupKey": "support/stale", + "action": "update_local", + "status": "failed", + "error": "write failed" + } + ] +} +`, output.String()) +} + +func TestWritePlanReviewShowsSummaryAndOneRowPerChange(t *testing.T) { + var output bytes.Buffer + require.NoError(t, writePlanReview(&output, "plaintext", reviewTestPlan(), 0, false)) + + assert.Equal(t, `3 changes: 1 tool record, 1 prompt push, 1 prompt pull. 1 conflict. 1 in sync. + +Tools + record production/lookup same as LaunchDarkly +Prompts + push production/support/default instructions changed +1 -1 + pull production/support/second instructions changed in LaunchDarkly +1 -1 + conflict production/support/third changed on both sides +1 -1 + + Variation content (changed) + + --- LaunchDarkly now + +++ Local file now + @@ -4,3 +4,3 @@ + "name": "Support", + - "instructions": "Help" + + "instructions": "Help briefly" + } +`, output.String()) +} + +func TestWritePlanReviewDiffMatchesRowSize(t *testing.T) { + var output bytes.Buffer + require.NoError(t, writePlanReview(&output, "plaintext", reviewTestPlan(), 0, true)) + + rendered := output.String() + assert.Equal(t, 3, strings.Count(rendered, "Variation content (changed)")) + var added, removed int + for _, line := range strings.Split(rendered, "\n") { + line = strings.TrimSpace(line) + switch { + case isAddedDiffLine(line): + added++ + case isRemovedDiffLine(line): + removed++ + } + } + assert.Equal(t, 3, added) + assert.Equal(t, 3, removed) + assert.Equal(t, 3, strings.Count(rendered, "+1 -1")) +} + +func TestWritePlanReviewMarkdownWritesOneTablePerKind(t *testing.T) { + var output bytes.Buffer + require.NoError(t, writePlanReview(&output, "markdown", reviewTestPlan(), 0, false)) + + rendered := output.String() + assert.Contains(t, rendered, "### Tools\n\n| Action | Resource | Reason | Size |\n| --- | --- | --- | --- |\n"+ + "| record | production/lookup | same as LaunchDarkly | |\n") + assert.Contains(t, rendered, "### Prompts\n\n| Action | Resource | Reason | Size |\n| --- | --- | --- | --- |\n"+ + "| push | production/support/default | instructions changed | +1 -1 |\n") + assert.Contains(t, rendered, "\n#### `production/support/third`\n") + assert.NotContains(t, rendered, "#### `production/support/default`") +} + +func TestDiffSizeTextColorsEachPart(t *testing.T) { + useColorTerminal(t, "") + + assert.Equal(t, "+3 -1", diffSizeText(3, 1, false)) + assert.Equal(t, "\x1b[92m+3\x1b[0m \x1b[91m-1\x1b[0m", diffSizeText(3, 1, true)) + assert.Equal(t, "\x1b[92m+2\x1b[0m", diffSizeText(2, 0, true)) + assert.Equal(t, "\x1b[91m-4\x1b[0m", diffSizeText(0, 4, true)) + assert.Empty(t, diffSizeText(0, 0, true)) +} + +func TestWritePlanReviewColorsSizesOnATerminal(t *testing.T) { + var plain bytes.Buffer + require.NoError(t, writePlanReview(&plain, "plaintext", reviewTestPlan(), 0, false)) + useColorTerminal(t, "") + + var output bytes.Buffer + require.NoError(t, writePlanReview(&output, "plaintext", reviewTestPlan(), 80, false)) + + rendered := output.String() + assert.Contains(t, rendered, " push production/support/default instructions changed "+ + "\x1b[92m+1\x1b[0m \x1b[91m-1\x1b[0m\n") + assert.Equal(t, 3, strings.Count(rendered, "\x1b[92m+1\x1b[0m \x1b[91m-1\x1b[0m")) + assert.Equal(t, plain.String(), ansi.Strip(rendered), "the colors must not change the column widths") +} + +func TestWritePlanSizesHaveNoColorOffATerminal(t *testing.T) { + tests := map[string]struct { + noColor string + write func(io.Writer) error + }{ + "piped plain text": {write: func(out io.Writer) error { + return writePlanOutput(out, "plaintext", reviewTestPlan(), false) + }}, + "Markdown on a terminal": {write: func(out io.Writer) error { + return writePlanReview(out, "markdown", reviewTestPlan(), 80, false) + }}, + "JSON": {write: func(out io.Writer) error { + return writePlanOutput(out, "json", reviewTestPlan(), true) + }}, + "NO_COLOR on a terminal": {noColor: "1", write: func(out io.Writer) error { + return writePlanReview(out, "plaintext", reviewTestPlan(), 80, false) + }}, + } + for name, test := range tests { + t.Run(name, func(t *testing.T) { + useColorTerminal(t, test.noColor) + + var output bytes.Buffer + require.NoError(t, test.write(&output)) + assert.NotContains(t, output.String(), "\x1b[") + }) + } +} + +func TestWritePlanReviewSummarizesPlansWithoutChanges(t *testing.T) { + var output bytes.Buffer + require.NoError(t, writePlanReview(&output, "plaintext", Plan{}, 0, false)) + assert.Equal(t, "No prompt variations are synced in this repository. To start, run ldcli sync prompts add.\n", output.String()) + + output.Reset() + inSync := Plan{Resources: []PlannedResource{ + {ID: testResourceID(), Action: ActionInSync}, + {ID: testResourceID(), Action: ActionInSync}, + }} + require.NoError(t, writePlanReview(&output, "plaintext", inSync, 0, false)) + assert.Equal(t, "No changes. 2 in sync.\n", output.String()) +} + +func TestPlanReasonNamesTheChange(t *testing.T) { + variation := testVariation("Support") + tests := map[string]struct { + resource PlannedResource + want string + }{ + "create": {PlannedResource{Action: ActionCreateServer}, "new"}, + "restore": {PlannedResource{Action: ActionUpdateLocal}, "local file missing"}, + "delete": {PlannedResource{Action: ActionDeleteLocal}, "deleted in LaunchDarkly"}, + "forget": {PlannedResource{Action: ActionRemoveManifest}, "gone on both sides"}, + "error": { + PlannedResource{Action: ActionError, Error: "variation does not exist in LaunchDarkly; set upsert: true to create it"}, + "variation does not exist in LaunchDarkly", + }, + "tools": { + PlannedResource{Action: ActionUpdateLocal, Local: &variation, Diff: variationDiffFields{"tools": {}}}, + "tools changed in LaunchDarkly", + }, + "repin": { + PlannedResource{ + Action: ActionUpdateServer, ServerHasStaleAttachmentPins: true, + Diff: variationDiffFields{"attachment versions": { + After: json.RawMessage(`{"skills":{"summarize":2},"tools":{"search":4}}`), + }}, + }, + "search to v4, summarize to v2", + }, + } + + for name, test := range tests { + t.Run(name, func(t *testing.T) { + assert.Equal(t, test.want, planReason(test.resource)) + }) + } +} + +func TestPlanSummaryCountsRepinsSeparately(t *testing.T) { + plan := Plan{Resources: []PlannedResource{ + {ID: testResourceID(), Action: ActionUpdateServer}, + {ID: testResourceID(), Action: ActionUpdateServer, ServerHasStaleAttachmentPins: true}, + {ID: testResourceID(), Action: ActionError}, + }} + + assert.Equal(t, "2 changes: 1 prompt push, 1 prompt repin. 1 error.", planSummary(plan)) +} + +func TestWriteOutcomeOutputMirrorsThePlan(t *testing.T) { + plan := reviewTestPlan() + outcomes := []ResourceOutcome{ + {ID: plan.Resources[0].ID, Action: ActionUpdateManifest, Status: OutcomeSucceeded}, + {ID: plan.Resources[1].ID, Action: ActionUpdateServer, Status: OutcomeSucceeded}, + {ID: plan.Resources[2].ID, Action: ActionUpdateLocal, Status: OutcomeFailed, Error: "write failed"}, + {ID: plan.Resources[4].ID, Action: ActionInSync, Status: OutcomeSucceeded}, + } + + var output bytes.Buffer + require.NoError(t, writeOutcomeOutput(&output, "plaintext", plan, outcomes)) + + assert.Equal(t, `Tools + recorded production/lookup same as LaunchDarkly +Prompts + pushed production/support/default instructions changed + +Failed: variation production/support/second: write failed +2 applied, 1 failed. 1 in sync. +`, output.String()) +} + func TestWritePlanDescribesMissingLaunchDarklyVariation(t *testing.T) { variation := syncdomain.Variation{Mode: syncdomain.VariationModeCompletion, Key: "default", Name: "Default"} plan := Plan{Resources: []PlannedResource{{ @@ -35,8 +320,9 @@ func TestWritePlanDescribesMissingLaunchDarklyVariation(t *testing.T) { }}} var output bytes.Buffer - require.NoError(t, writePlanOutput(&output, "plaintext", plan)) + require.NoError(t, writePlanOutput(&output, "plaintext", plan, true)) + assert.Contains(t, output.String(), " create production/support/default new +5\n") assert.Contains(t, output.String(), "(does not exist in LaunchDarkly)") assert.NotContains(t, output.String(), `"variation": {`) assert.NotContains(t, output.String(), "-null") @@ -45,12 +331,13 @@ func TestWritePlanDescribesMissingLaunchDarklyVariation(t *testing.T) { func TestWritePlanNotesResourceSyncedByAnotherWorkingCopy(t *testing.T) { plan := Plan{Resources: []PlannedResource{{ID: testResourceID(), Action: ActionUpdateLocal, SyncedElsewhere: true}}} - var text, data bytes.Buffer - require.NoError(t, writePlanOutput(&text, "plaintext", plan)) - require.NoError(t, writePlanOutput(&data, "json", plan)) + var notes, data bytes.Buffer + writePlanNotes(¬es, plan) + require.NoError(t, writePlanOutput(&data, "json", plan, false)) - assert.Contains(t, text.String(), "Note: Another working copy synced this variation after your sync.lock.") - assert.Contains(t, text.String(), "Suggestion: If the other working copy pushed its change to Git, run git pull before you sync.") + assert.Equal(t, "Note: Another working copy synced variation production/support/default after your sync.lock. "+ + "If the other working copy pushed its change to Git, run git pull before you sync. "+ + "Then your sync.lock does not get a merge conflict.\n", notes.String()) assert.Contains(t, data.String(), `"syncedElsewhere": true`) assert.Contains(t, data.String(), `"suggestion": "If the other working copy pushed its change to Git`) } @@ -79,15 +366,12 @@ func TestWritePlanRendersHumanFriendlyAttachmentDiff(t *testing.T) { }}} var output bytes.Buffer - require.NoError(t, writePlanReview(&output, "plaintext", plan, 0)) + require.NoError(t, writePlanReview(&output, "plaintext", plan, 0, true)) rendered := output.String() - assert.Contains(t, rendered, " Config: support") - assert.Contains(t, rendered, " Variation: default") - assert.Contains(t, rendered, " Action: Update LaunchDarkly from the local file") + assert.Contains(t, rendered, " push production/support/default tools changed +4\n") assert.Contains(t, rendered, ` Tool "my-first-tool" (added)`) assert.Contains(t, rendered, "(not attached)") - assert.Contains(t, rendered, "my-first-tool") assert.Contains(t, rendered, "Description: This is a test") assert.Contains(t, rendered, "additionalProperties: false") assert.NotContains(t, rendered, "@@") @@ -130,7 +414,7 @@ func TestWritePlanRendersEachSkillInItsOwnDiff(t *testing.T) { }}} var output bytes.Buffer - require.NoError(t, writePlanReview(&output, "plaintext", plan, 140)) + require.NoError(t, writePlanReview(&output, "plaintext", plan, 140, true)) rendered := output.String() assert.Equal(t, 1, strings.Count(rendered, `Skill "first"`)) @@ -162,39 +446,13 @@ func TestWritePlanMarkdownProtectsSkillCodeFences(t *testing.T) { }}} var output bytes.Buffer - require.NoError(t, writePlanReview(&output, "markdown", plan, 0)) + require.NoError(t, writePlanReview(&output, "markdown", plan, 0, true)) rendered := output.String() assert.Contains(t, rendered, "````diff\n") assert.Equal(t, 2, strings.Count(rendered, "````")) } -func TestWritePlanGroupsVariationsUnderTheirConfig(t *testing.T) { - plan := Plan{Resources: []PlannedResource{ - { - ID: ResourceID{ - Kind: syncdomain.KindVariation, ProjectKey: "production", LookupKey: "support/first", - }, - Action: ActionInSync, - }, - { - ID: ResourceID{ - Kind: syncdomain.KindVariation, ProjectKey: "production", LookupKey: "support/second", - }, - Action: ActionInSync, - }, - }} - - var output bytes.Buffer - require.NoError(t, writePlanReview(&output, "plaintext", plan, 0)) - - rendered := output.String() - assert.Equal(t, 1, strings.Count(rendered, " Config: support")) - assert.Contains(t, rendered, " Variation: first") - assert.Contains(t, rendered, " Variation: second") - assert.Contains(t, rendered, " Action: No change (in sync)") -} - func TestUnifiedDiffKeepsOneUnchangedLineAroundChanges(t *testing.T) { lines, err := unifiedDiffLines( "first\nbefore\nold\nafter\nlast", @@ -210,3 +468,31 @@ func TestUnifiedDiffKeepsOneUnchangedLineAroundChanges(t *testing.T) { assert.NotContains(t, rendered, " first") assert.NotContains(t, rendered, " last") } + +// useColorTerminal makes lipgloss render as it does for a color terminal with +// the given NO_COLOR value, until the test ends. +func useColorTerminal(t *testing.T, noColor string) { + t.Helper() + t.Setenv("NO_COLOR", noColor) + t.Setenv("CLICOLOR_FORCE", "1") + previous := lipgloss.DefaultRenderer() + lipgloss.SetDefaultRenderer(lipgloss.NewRenderer(os.Stdout)) + t.Cleanup(func() { lipgloss.SetDefaultRenderer(previous) }) +} + +// reviewTestPlan has one row of each common kind of change, in plan order. +func reviewTestPlan() Plan { + server, local := testVariation("Support"), testVariation("Support") + local.Instructions = "Help briefly" + diff := variationDiff(&server, &local) + variationID := func(lookupKey string) ResourceID { + return ResourceID{Kind: syncdomain.KindVariation, ProjectKey: "production", LookupKey: lookupKey} + } + return Plan{Resources: []PlannedResource{ + {ID: ResourceID{Kind: syncdomain.KindTool, ProjectKey: "production", LookupKey: "lookup"}, Action: ActionUpdateManifest}, + {ID: variationID("support/default"), Action: ActionUpdateServer, Local: &local, Server: &server, Diff: diff}, + {ID: variationID("support/second"), Action: ActionUpdateLocal, Local: &local, Server: &server, Diff: diff}, + {ID: variationID("support/third"), Action: ActionConflict, Local: &local, Server: &server, Diff: diff}, + {ID: variationID("support/fourth"), Action: ActionInSync}, + }} +} diff --git a/internal/sync/prompt/plan.go b/internal/sync/prompt/plan.go index 5df33a296..9e60f200b 100644 --- a/internal/sync/prompt/plan.go +++ b/internal/sync/prompt/plan.go @@ -1,6 +1,7 @@ package prompt import ( + "errors" "fmt" "slices" @@ -257,15 +258,18 @@ func (plan Plan) HasChanges() bool { }) } -// BlockingError returns an error for the first conflict or invalid resource. +// BlockingError returns one error for each conflict or invalid resource, in +// plan order. A plan row shows only the first clause of an error, so this +// error carries the full message of every row. func (plan Plan) BlockingError() error { + var blocking []error for _, resource := range plan.Resources { switch resource.Action { case ActionConflict: - return fmt.Errorf("cannot sync conflicted resource %s", resource.ID) + blocking = append(blocking, fmt.Errorf("cannot sync conflicted resource %s", resource.ID.Label())) case ActionError: - return fmt.Errorf("cannot sync %s: %s", resource.ID, resource.Error) + blocking = append(blocking, fmt.Errorf("cannot sync %s: %s", resource.ID.Label(), resource.Error)) } } - return nil + return errors.Join(blocking...) } diff --git a/internal/sync/prompt/plan_test.go b/internal/sync/prompt/plan_test.go index 2d31c9cb8..42a25f736 100644 --- a/internal/sync/prompt/plan_test.go +++ b/internal/sync/prompt/plan_test.go @@ -133,6 +133,22 @@ func TestBuildPlanRejectsParentConfigModeMismatch(t *testing.T) { require.Contains(t, plan.Resources[0].Error, "does not match config mode") } +func TestPlanBlockingErrorHasTheFullErrorOfEveryRow(t *testing.T) { + conflicted, missing, broken := testResourceID(), testResourceID(), testResourceID() + missing.LookupKey, broken.LookupKey = "support/missing", "support/broken" + plan := Plan{Resources: []PlannedResource{ + {ID: conflicted, Action: ActionConflict}, + {ID: missing, Action: ActionError, Error: "variation does not exist in LaunchDarkly; set upsert: true to create it"}, + {ID: testResourceID(), Action: ActionInSync}, + {ID: broken, Action: ActionError, Error: "invalid server variation: bad mode"}, + }} + + require.EqualError(t, plan.BlockingError(), "cannot sync conflicted resource variation production/support/default\n"+ + "cannot sync variation production/support/missing: variation does not exist in LaunchDarkly; set upsert: true to create it\n"+ + "cannot sync variation production/support/broken: invalid server variation: bad mode") + require.NoError(t, Plan{Resources: []PlannedResource{{ID: testResourceID(), Action: ActionUpdateServer}}}.BlockingError()) +} + func TestPlanHasDestructiveActions(t *testing.T) { tests := map[string]struct { action Action diff --git a/internal/sync/prompt/render_diff.go b/internal/sync/prompt/render_diff.go index a702bc7e0..45690f21c 100644 --- a/internal/sync/prompt/render_diff.go +++ b/internal/sync/prompt/render_diff.go @@ -31,54 +31,107 @@ const ( diffBodyIndent = 8 ) -// renderVariationDiff formats structured field changes as terminal or Markdown -// unified diffs, choosing side-by-side output when the terminal is wide enough. -func renderVariationDiff(fields variationDiffFields, outputKind string, width int, presentation variationDiffPresentation) (string, error) { - keys := make([]string, 0, len(fields)) - for key := range fields { - keys = append(keys, key) - } - slices.Sort(keys) +// A placeholder stands in for the side of a diff that does not exist. It is +// not content, so a diff size does not count it. +const ( + missingInLaunchDarkly = "(does not exist in LaunchDarkly)" + missingLocally = "(does not exist locally)" + notAttached = "(not attached)" +) - var rendered strings.Builder - for _, key := range keys { +// diffBlock is the unified diff of one titled section, for example +// `Tool "search" (changed)`. +type diffBlock struct { + title string + lines []string +} + +// diffBlocks builds the unified diff of each changed part, in the order that +// the review shows them. +func diffBlocks(fields variationDiffFields, presentation variationDiffPresentation) ([]diffBlock, error) { + var blocks []diffBlock + for _, key := range slices.Sorted(maps.Keys(fields)) { diff := fields[key] if presentation.reverse { diff.Before, diff.After = diff.After, diff.Before } sections, err := diffSections(key, diff, presentation) if err != nil { - return "", err + return nil, err } for _, section := range sections { diffLines, err := unifiedDiffLines(section.before, section.after, presentation.beforeLabel, presentation.afterLabel) if err != nil { - return "", err + return nil, err } if key == "tools" || key == "skills" { diffLines = slices.DeleteFunc(diffLines, func(line string) bool { return strings.HasPrefix(line, "@@") }) } - if outputKind == "markdown" { - _, _ = fmt.Fprintf(&rendered, "\n#### %s (%s)\n\n", section.title, section.change) - content := strings.Join(diffLines, "\n") - fence := markdownCodeFence(content) - _, _ = fmt.Fprintf(&rendered, "%sdiff\n%s\n%s\n", fence, content, fence) - continue - } - sectionTitle := fmt.Sprintf("%s (%s)", section.title, section.change) - if width > 0 { - sectionTitle = lipgloss.NewStyle().Foreground(lipgloss.Color("67")).Bold(true).Render(sectionTitle) - } - _, _ = fmt.Fprintf(&rendered, "\n%s%s\n\n", strings.Repeat(" ", diffSectionIndent), sectionTitle) - if width >= 100 { - rendered.WriteString(renderSideBySideUnifiedDiff(diffLines, width)) - } else { - rendered.WriteString(renderUnifiedDiff(diffLines, width > 0)) + blocks = append(blocks, diffBlock{title: fmt.Sprintf("%s (%s)", section.title, section.change), lines: diffLines}) + } + } + return blocks, nil +} + +// diffSize counts the added and the removed lines of content that +// renderVariationDiff shows for the same diff. +func diffSize(fields variationDiffFields, presentation variationDiffPresentation) (added, removed int, err error) { + blocks, err := diffBlocks(fields, presentation) + for _, block := range blocks { + for _, line := range block.lines { + switch { + case isPlaceholderDiffLine(line): + case isAddedDiffLine(line): + added++ + case isRemovedDiffLine(line): + removed++ } } } + return added, removed, err +} + +// isPlaceholderDiffLine reports whether a changed line holds a placeholder +// instead of content. +func isPlaceholderDiffLine(line string) bool { + switch strings.TrimLeft(line, "+-") { + case missingInLaunchDarkly, missingLocally, notAttached: + return true + default: + return false + } +} + +// renderVariationDiff formats structured field changes as terminal or Markdown +// unified diffs, choosing side-by-side output when the terminal is wide enough. +func renderVariationDiff(fields variationDiffFields, outputKind string, width int, presentation variationDiffPresentation) (string, error) { + blocks, err := diffBlocks(fields, presentation) + if err != nil { + return "", err + } + + var rendered strings.Builder + for _, block := range blocks { + if outputKind == "markdown" { + _, _ = fmt.Fprintf(&rendered, "\n#### %s\n\n", block.title) + content := strings.Join(block.lines, "\n") + fence := markdownCodeFence(content) + _, _ = fmt.Fprintf(&rendered, "%sdiff\n%s\n%s\n", fence, content, fence) + continue + } + title := block.title + if width > 0 { + title = lipgloss.NewStyle().Foreground(lipgloss.Color("67")).Bold(true).Render(title) + } + _, _ = fmt.Fprintf(&rendered, "\n%s%s\n\n", strings.Repeat(" ", diffSectionIndent), title) + if width >= 100 { + rendered.WriteString(renderSideBySideUnifiedDiff(block.lines, width)) + } else { + rendered.WriteString(renderUnifiedDiff(block.lines, width > 0)) + } + } return rendered.String(), nil } @@ -162,7 +215,7 @@ func attachmentDiffSections[T any]( continue } - beforeValue, afterValue := "(not attached)", "(not attached)" + beforeValue, afterValue := notAttached, notAttached if beforeExists { beforeValue = format(beforeItem) } @@ -476,9 +529,9 @@ func changedParts(before, after string) (prefix, removed, added, suffix string) func formatDiffValue(value json.RawMessage, label string) (string, error) { if len(value) == 0 || bytes.Equal(value, []byte("null")) { if strings.HasPrefix(label, "LaunchDarkly") { - return "(does not exist in LaunchDarkly)", nil + return missingInLaunchDarkly, nil } - return "(does not exist locally)", nil + return missingLocally, nil } var formatted bytes.Buffer if err := json.Indent(&formatted, value, "", " "); err != nil { diff --git a/internal/sync/prompt/review.go b/internal/sync/prompt/review.go index bee786e1e..9e4771415 100644 --- a/internal/sync/prompt/review.go +++ b/internal/sync/prompt/review.go @@ -1,6 +1,7 @@ package prompt import ( + "errors" "slices" "strings" @@ -12,29 +13,35 @@ import ( const applyQuestion = "\nSync these changes? [y/N] " -// reviewAndConfirmPlan shows the plan and reports whether to apply it. +// reviewAndConfirmPlan reports whether to apply the plan. It shows the plan +// only when the plan has nothing to apply, when a row blocks the sync, or +// before it asks the user. // // A plan that changes only the manifest applies without a question. With // --yes, every plan applies. In watch mode, a plan without a deletion or an // archive applies. Every other plan needs the user to agree. func reviewAndConfirmPlan(options Options, plan Plan, interactive bool) (bool, error) { - if err := writePlanReview(options.ErrorOutput, "plaintext", plan, terminalWidth(options.ErrorOutput)); err != nil { - return false, err + writePlanNotes(options.ErrorOutput, plan) + review := func() error { + return writePlanReview(options.ErrorOutput, outputPlaintext, plan, terminalWidth(options.ErrorOutput), options.Diff) } if err := plan.BlockingError(); err != nil { - return false, err + return false, errors.Join(review(), err) } if !plan.HasChanges() { - if options.OutputKind != "" && options.OutputKind != "plaintext" { - return false, writePlanOutput(options.Output, options.OutputKind, plan) + if options.OutputKind != "" && options.OutputKind != outputPlaintext { + return false, writePlanOutput(options.Output, options.OutputKind, plan, options.Diff) } - return false, nil + return false, review() } autoApply := options.Yes || (options.watching() && !plan.HasDestructiveActions()) if autoApply || !plan.RequiresConfirmation() { return true, nil } + if err := review(); err != nil { + return false, err + } confirmed, err := syncinteractive.Confirm(options.Context, options.Input, options.ErrorOutput, interactive, applyQuestion) if err != nil { return false, err diff --git a/internal/sync/prompt/review_test.go b/internal/sync/prompt/review_test.go index a6a726fa7..ff88b0e08 100644 --- a/internal/sync/prompt/review_test.go +++ b/internal/sync/prompt/review_test.go @@ -216,6 +216,50 @@ func TestReviewAndConfirmPlanRejectsConfirmationWhenWatchContextIsAlreadyCancele require.ErrorIs(t, err, context.Canceled) } +func TestReviewAndConfirmPlanShowsPlanOnlyWhenNeeded(t *testing.T) { + tests := []struct { + name string + action Action + yes bool + input string + wantContinue bool + wantError string + wantReview string + }{ + {name: "yes applies without the plan", action: ActionUpdateServer, yes: true, wantContinue: true}, + {name: "a manifest-only plan applies without the plan", action: ActionUpdateManifest, wantContinue: true}, + { + name: "a question shows the plan first", action: ActionUpdateServer, input: "yes\n", wantContinue: true, + wantReview: "1 change: 1 prompt push.\n\nPrompts\n push production/support/default changed\n\nSync these changes? [y/N] ", + }, + { + name: "a blocking row shows the plan", action: ActionError, yes: true, + wantError: "cannot sync variation production/support/default: broken", + wantReview: "1 error.\n\nPrompts\n error production/support/default broken\n", + }, + {name: "a plan without changes shows its summary", action: ActionInSync, wantReview: "No changes. 1 in sync.\n"}, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + var output bytes.Buffer + plan := Plan{Resources: []PlannedResource{{ID: testResourceID(), Action: test.action, Error: "broken"}}} + + continued, err := reviewAndConfirmPlan(Options{ + Yes: test.yes, Input: strings.NewReader(test.input), ErrorOutput: &output, + }, plan, true) + + assert.Equal(t, test.wantContinue, continued) + if test.wantError != "" { + require.EqualError(t, err, test.wantError) + } else { + require.NoError(t, err) + } + assert.Equal(t, test.wantReview, output.String()) + }) + } +} + func TestReviewAndConfirmPlanWatchPolicy(t *testing.T) { tests := []struct { name string diff --git a/internal/sync/prompt/runner.go b/internal/sync/prompt/runner.go index 07cc53cab..80e8a1d5d 100644 --- a/internal/sync/prompt/runner.go +++ b/internal/sync/prompt/runner.go @@ -232,6 +232,7 @@ func (runner Runner) bootstrapOptions( DryRun: dryRun, Selections: selections, NoInput: options.NoInput, + Diff: options.Diff, } } diff --git a/internal/sync/prompt/sync.go b/internal/sync/prompt/sync.go index 1a31cb479..70a213af2 100644 --- a/internal/sync/prompt/sync.go +++ b/internal/sync/prompt/sync.go @@ -36,7 +36,8 @@ func (runner Runner) runWorkspaceSync(options Options, workspace syncWorkspace, return err } if options.dryRun() { - if err := writePlanOutput(options.Output, options.OutputKind, reviewed.plan); err != nil { + writePlanNotes(options.ErrorOutput, reviewed.plan) + if err := writePlanOutput(options.Output, options.OutputKind, reviewed.plan, options.Diff); err != nil { return err } return reviewed.plan.BlockingError() @@ -111,7 +112,7 @@ func (runner Runner) applyPlan( if err := workspace.local.RemoveEmptyDirectories(); err != nil { failures = append(failures, err) } - if err := writeOutcomeOutput(options.Output, options.OutputKind, outcomes); err != nil { + if err := writeOutcomeOutput(options.Output, options.OutputKind, plan, outcomes); err != nil { failures = append(failures, err) } if err := errors.Join(failures...); err != nil { diff --git a/internal/sync/resource.go b/internal/sync/resource.go index 96bd8dba4..bcd7a204e 100644 --- a/internal/sync/resource.go +++ b/internal/sync/resource.go @@ -55,6 +55,11 @@ func (id ResourceID) String() string { return id.ProjectKey + "/" + id.LookupKey } +// Label returns the kind and the identity, for example "tool demo/search". +func (id ResourceID) Label() string { + return string(id.Kind) + " " + id.String() +} + // VariationKeys splits a variation lookup key into its config key and its // variation key. func (id ResourceID) VariationKeys() (configKey, variationKey string, err error) {