diff --git a/cmd/sync/prompt.go b/cmd/sync/prompt.go index 70210b20..beb0806f 100644 --- a/cmd/sync/prompt.go +++ b/cmd/sync/prompt.go @@ -217,11 +217,14 @@ func newDetachCmd(client resources.Client) *cobra.Command { cmd := &cobra.Command{ Use: "detach [project-key/config-key/variation-key...]", Short: "Stop syncing local prompt variations", + Long: "Stop syncing local prompt variations. Detach deletes their local files and their sync.lock entries. " + + "LaunchDarkly keeps them, unless you pass --archive. Detach lists the variations and asks before it deletes files. " + + "Without a terminal, pass --yes. Tools and skills that the variation used keep syncing on their own.", Example: ` # Choose variations interactively ldcli sync prompts detach - # Stop syncing one variation - ldcli sync prompts detach --project production --config-key support --key default + # Stop syncing one variation without prompting + ldcli sync prompts detach --project production --config-key support --key default --yes --no-input # Stop syncing one variation and archive it in LaunchDarkly ldcli sync prompts detach --project production --config-key support --key default --archive --yes --no-input`, @@ -232,12 +235,12 @@ func newDetachCmd(client resources.Client) *cobra.Command { return nil, err } archive, _ := cmd.Flags().GetBool(archiveFlag) - return syncprompt.DetachAction{Variations: variations, Filter: targetFilter(cmd), Archive: archive}, nil + return syncprompt.DetachAction{Resources: variations, Filter: targetFilter(cmd), Archive: archive}, nil }), } addTargetFlags(cmd, questionFlagHelp) cmd.Flags().Bool(archiveFlag, false, "Also archive the variations in LaunchDarkly") - cmd.Flags().Bool(yesFlag, false, "Archive without confirmation") + cmd.Flags().Bool(yesFlag, false, "Detach, and archive, without confirmation") cmd.Flags().Bool(noInputFlag, false, "Fail instead of prompting for missing input") return cmd } diff --git a/internal/sync/detach/detach.go b/internal/sync/detach/detach.go index bd7b9262..8ee4aa2c 100644 --- a/internal/sync/detach/detach.go +++ b/internal/sync/detach/detach.go @@ -1,6 +1,6 @@ -// Package detach stops syncing selected variations. It removes their local -// files and their baseline. LaunchDarkly keeps the variations, unless the -// user asks to archive them. +// Package detach stops syncing selected resources of any kind. It removes +// their local files and their baseline. LaunchDarkly keeps the resources, +// unless the user asks to archive the variations. package detach import ( @@ -18,7 +18,7 @@ import ( syncmanifest "github.com/launchdarkly/ldcli/internal/sync/manifest" ) -const archiveQuestion = "\nArchive these variations in LaunchDarkly? [y/N] " +const detachQuestion = "\nDetach these resources? [y/N] " // Archiver reads and archives a variation in LaunchDarkly. type Archiver interface { @@ -35,17 +35,20 @@ type Options struct { ProjectKeys []string Input io.Reader Output io.Writer - // Selections are the variations to detach. If it is empty, Run asks the + // Kind is the kind of resource that the user chooses from when + // Selections is empty. + Kind syncdomain.Kind + // Selections are the resources to detach. If it is empty, Run asks the // user to choose from the synced resources that Filter keeps. Selections []syncdomain.ResourceID Filter syncdomain.Filter NoInput bool - // Archive also archives each selected variation in LaunchDarkly. Without - // Yes, the user must confirm the archive. + // Archive also archives each selected variation in LaunchDarkly. Archive bool Archiver Archiver - Yes bool - // Context stops the archive confirmation and the archive when it ends. + // Yes detaches, and archives, without a confirmation. + Yes bool + // Context stops the confirmation and the archive when it ends. Context context.Context // isTerminal reports whether the streams are a terminal. Tests replace it. @@ -60,7 +63,7 @@ func (options Options) context() context.Context { return options.Context } -// Run detaches the selected variations. +// Run detaches the selected resources after the user agrees. func Run(options Options) error { projectKeys := slices.Clone(options.ProjectKeys) for _, selection := range options.Selections { @@ -82,7 +85,9 @@ func Run(options Options) error { return err } } else { - synced = slices.DeleteFunc(synced, func(id syncdomain.ResourceID) bool { return !options.Filter.Includes(id) }) + synced = slices.DeleteFunc(synced, func(id syncdomain.ResourceID) bool { + return id.Kind != options.Kind || !options.Filter.Includes(id) + }) if len(synced) == 0 { _ = console.Line("No resources are currently synced.") return nil @@ -92,11 +97,22 @@ func Run(options Options) error { return err } } + + // A workspace with an invalid variation file does not compile. A detach + // of variations still works then, so the error only stops a check that + // needs the variations. + variations, compileErr := synclocal.CompileWorkspace(options.RepositoryRoot) + if errors.Is(compileErr, synclocal.ErrNoDirectory) { + compileErr = nil + } + if err := checkUnused(selected, variations, compileErr); err != nil { + return err + } + confirmed, err := confirmDetach(options, baseline.Lock, variations, selected) + if err != nil || !confirmed { + return err + } if options.Archive { - confirmed, err := confirmArchive(options, selected) - if err != nil || !confirmed { - return err - } // Archive first. If a later step fails, the same command can run again, // because it skips a variation that is already archived. if err := archiveVariations(options.context(), options.Archiver, selected); err != nil { @@ -113,12 +129,12 @@ func Run(options Options) error { _ = console.Line("Detached resources:") } for _, resource := range selected { - _ = console.Printf("- %s %s\n", resource.Kind, resource) + _ = console.Printf("- %s\n", resource.Label()) } return nil } -// promptForResources asks the user to choose one or more synced variations. +// promptForResources asks the user to choose one or more synced resources. // The bool result is true when the user cancels. func promptForResources(options Options, synced []syncdomain.ResourceID) ([]syncdomain.ResourceID, bool, error) { arguments := "--project and --key" @@ -141,28 +157,106 @@ func promptForResources(options Options, synced []syncdomain.ResourceID) ([]sync return syncinteractive.MultiSelect(options.Input, options.Output, "Select resources to detach", description, choices) } -// confirmArchive asks the user to agree to the archive, unless Yes is set. -func confirmArchive(options Options, selected []syncdomain.ResourceID) (bool, error) { +// checkUnused refuses to detach a tool or a skill that a local variation +// still uses, unless the same detach removes that variation. +func checkUnused(selected []syncdomain.ResourceID, variations []syncdomain.SyncedResource, compileErr error) error { + for _, id := range selected { + if id.Kind == syncdomain.KindVariation { + continue + } + if compileErr != nil { + return fmt.Errorf("find the variations that use %s: %w", id.Label(), compileErr) + } + for _, variation := range variations { + _, uses := variation.Variation.Attachment(syncdomain.AttachmentKind(id.Kind), id.LookupKey) + if uses && variation.ProjectKey == id.ProjectKey && !slices.Contains(selected, variation.ID()) { + return fmt.Errorf("%s is used by variation %s. Remove it from that variation first.", id.Label(), variation.ID()) + } + } + } + return nil +} + +// confirmDetach lists the selected resources and asks once, unless Yes is +// set. Without a terminal, it returns the error that suggests --yes. +func confirmDetach( + options Options, + lock syncmanifest.Manifest, + variations []syncdomain.SyncedResource, + selected []syncdomain.ResourceID, +) (bool, error) { if options.Yes { return true, nil } - console := syncconsole.New(options.Output) - _ = console.Line("Variations to archive in LaunchDarkly:") - for _, resource := range selected { - _ = console.Printf("- %s\n", resource) - } isTerminal := options.isTerminal if isTerminal == nil { isTerminal = syncinteractive.StreamsAreTerminal } interactive := !options.NoInput && isTerminal(options.Input, options.Output) - confirmed, err := syncinteractive.Confirm(options.context(), options.Input, options.Output, interactive, archiveQuestion) + if interactive { + console := syncconsole.New(options.Output) + if options.Archive { + _ = console.Line("Resources to detach and archive in LaunchDarkly:") + } else { + _ = console.Line("Resources to detach:") + } + changed := localChanges(options.RepositoryRoot, lock, variations, selected) + for _, id := range selected { + if changed[id] { + _ = console.Printf(" %s (local changes will be lost)\n", id.Label()) + } else { + _ = console.Printf(" %s\n", id.Label()) + } + } + } + + confirmed, err := syncinteractive.Confirm(options.context(), options.Input, options.Output, interactive, detachQuestion) if err == nil && !confirmed { - _ = console.Line("Detach canceled.") + _ = syncconsole.New(options.Output).Line("Detach canceled.") } return confirmed, err } +// localChanges reports each selected resource whose local file differs from +// its sync.lock entry, because the detach deletes that file. A file that sync +// cannot read counts as changed. A resource without a local file loses +// nothing. +func localChanges( + repositoryRoot string, + lock syncmanifest.Manifest, + variations []syncdomain.SyncedResource, + selected []syncdomain.ResourceID, +) map[syncdomain.ResourceID]bool { + local := make(map[syncdomain.ResourceID]string) + for _, variation := range variations { + if fingerprint, err := syncdomain.FingerprintVariation(variation.ProjectKey, variation.LookupKey, variation.Variation); err == nil { + local[variation.ID()] = fingerprint + } + } + isSelected := func(id syncdomain.ResourceID) bool { return slices.Contains(selected, id) } + attachments, _ := synclocal.CompileAttachments(repositoryRoot, isSelected) + for _, attachment := range attachments { + if fingerprint, err := syncdomain.FingerprintAttachment(attachment.ProjectKey, attachment.Attachment); err == nil { + local[attachment.ID()] = fingerprint + } + } + baseline := make(map[syncdomain.ResourceID]string, len(lock.Resources)) + for _, resource := range lock.Resources { + baseline[resource.ID()] = resource.Fingerprint + } + + files, _ := synclocal.SourceFiles(repositoryRoot) + changed := make(map[syncdomain.ResourceID]bool) + for _, file := range files { + id, ok := synclocal.ParseManagedPath(file) + if ok && isSelected(id) { + fingerprint, readable := local[id] + changed[id] = !readable || fingerprint != baseline[id] + } + } + return changed +} + // archiveVariations archives each variation that LaunchDarkly still has. It // reads each variation first, because the API rejects the archive of an // archived variation. A variation that is already archived or gone does not @@ -170,6 +264,10 @@ func confirmArchive(options Options, selected []syncdomain.ResourceID) (bool, er // before the next archive when ctx ends. func archiveVariations(ctx context.Context, archiver Archiver, selected []syncdomain.ResourceID) error { for _, resource := range selected { + // LaunchDarkly keeps a detached tool or skill. + if resource.Kind != syncdomain.KindVariation { + continue + } if err := ctx.Err(); err != nil { return err } @@ -196,17 +294,17 @@ func validateSelections(synced, selected []syncdomain.ResourceID) error { seen := make(map[syncdomain.ResourceID]struct{}, len(selected)) for _, selection := range selected { if _, duplicate := seen[selection]; duplicate { - return fmt.Errorf("variation %s was selected more than once", selection) + return fmt.Errorf("%s was selected more than once", selection.Label()) } seen[selection] = struct{}{} if !slices.Contains(synced, selection) { - return fmt.Errorf("variation %s is not synced", selection) + return fmt.Errorf("%s is not synced", selection.Label()) } } return nil } -// loadResources returns each variation that the lock tracks or that has a +// loadResources returns each resource that the lock tracks or that has a // local file, in identity order. func loadResources( repositoryRoot string, @@ -224,12 +322,10 @@ func loadResources( var synced []syncdomain.ResourceID for _, resource := range baseline.Lock.Resources { - if resource.ResourceKind == syncdomain.KindVariation { - synced = append(synced, resource.ID()) - } + synced = append(synced, resource.ID()) } for _, file := range files { - if id, ok := synclocal.ParseManagedPath(file); ok && id.Kind == syncdomain.KindVariation { + if id, ok := synclocal.ParseManagedPath(file); ok { synced = append(synced, id) } } @@ -237,24 +333,15 @@ func loadResources( return slices.Compact(synced), baseline, nil } -// detachResources removes the selected variations from the baseline, and then -// deletes their local files. If the delete fails, it restores the baseline. +// detachResources removes the selected resources from the baseline, and then +// deletes their local files in one transaction. Every other entry stays, also +// the tools and skills that only the detached variations used, because they +// sync on their own. If the delete fails, it keeps every file and restores the +// baseline. func detachResources(options Options, original syncmanifest.Baseline, selected []syncdomain.ResourceID) error { - isSelected := func(id syncdomain.ResourceID) bool { return slices.Contains(selected, id) } - - next := syncmanifest.New() - for _, resource := range original.Lock.Resources { - if !isSelected(resource.ID()) { - next.Resources = append(next.Resources, resource) - } - } - // Stop tracking each tool and skill that only the detached variations use. - // If the workspace does not compile, keep every attachment baseline. - if variations, err := synclocal.CompileWorkspace(options.RepositoryRoot); err == nil { - remaining := slices.DeleteFunc(variations, func(variation syncdomain.SyncedResource) bool { - return isSelected(variation.ID()) - }) - next.RemoveUnusedAttachments(remaining) + next := original.Lock.Clone() + for _, id := range selected { + next.Remove(id) } saved, err := options.Baselines.Save(original, next) if err != nil { @@ -265,24 +352,40 @@ func detachResources(options Options, original syncmanifest.Baseline, selected [ return errors.Join(cause, restoreErr) } - var deletions []synclocal.VariationDeletion - for _, resource := range selected { - configKey, variationKey, err := resource.VariationKeys() + deletions, err := localAttachments(options.RepositoryRoot, selected) + if err != nil { + return restoreBaseline(err) + } + for _, id := range selected { + configKey, variationKey, err := id.VariationKeys() if err != nil { continue } - exists, err := options.Store.VariationExists(resource.ProjectKey, configKey, variationKey) + exists, err := options.Store.VariationExists(id.ProjectKey, configKey, variationKey) if err != nil { return restoreBaseline(err) } if exists { - deletions = append(deletions, synclocal.VariationDeletion{ - ProjectKey: resource.ProjectKey, ConfigKey: configKey, VariationKey: variationKey, - }) + deletions = append(deletions, id) } } - if _, err := options.Store.DeleteVariations(deletions); err != nil { + if _, err := options.Store.DeleteResources(deletions); err != nil { return restoreBaseline(err) } return nil } + +// localAttachments returns each selected tool and skill that has a local file. +func localAttachments(repositoryRoot string, selected []syncdomain.ResourceID) ([]syncdomain.ResourceID, error) { + files, err := synclocal.SourceFiles(repositoryRoot) + if err != nil { + return nil, err + } + var attachments []syncdomain.ResourceID + for _, file := range files { + if id, ok := synclocal.ParseManagedPath(file); ok && id.Kind != syncdomain.KindVariation && slices.Contains(selected, id) { + attachments = append(attachments, id) + } + } + return attachments, nil +} diff --git a/internal/sync/detach/detach_test.go b/internal/sync/detach/detach_test.go index c996406f..d7865a1e 100644 --- a/internal/sync/detach/detach_test.go +++ b/internal/sync/detach/detach_test.go @@ -47,12 +47,14 @@ func TestLoadResourcesUnionsLocalAndManifestResources(t *testing.T) { require.NoError(t, err) assert.Equal(t, []syncdomain.ResourceID{ + {Kind: syncdomain.KindTool, ProjectKey: "project", LookupKey: "search"}, {Kind: syncdomain.KindVariation, ProjectKey: "project", LookupKey: "config/local"}, {Kind: syncdomain.KindVariation, ProjectKey: "project", LookupKey: "config/manifest-only"}, }, resources) } -func TestDetachResourcesPrunesUnreferencedAttachmentManifestEntries(t *testing.T) { +// A tool keeps syncing on its own after the variation that used it is detached. +func TestDetachResourcesKeepsAttachmentManifestEntries(t *testing.T) { root := t.TempDir() store := synclocal.NewStore(root) description := "Search documentation" @@ -89,9 +91,10 @@ func TestDetachResourcesPrunesUnreferencedAttachmentManifestEntries(t *testing.T require.NoError(t, err) baseline, err := manifestStore.Load([]string{"project"}) - manifest := baseline.Lock require.NoError(t, err) - assert.Empty(t, manifest.Resources) + assert.Equal(t, []syncmanifest.Resource{original.Resources[1]}, baseline.Lock.Resources) + _, err = os.Stat(filepath.Join(root, syncdomain.RootDir, "project", "tools", "search.json")) + require.NoError(t, err, "the tool file stays") } func TestDetachResourcesRemovesWrapperAndManifestButKeepsReferencedFile(t *testing.T) { @@ -118,7 +121,11 @@ func TestDetachResourcesRemovesWrapperAndManifestButKeepsReferencedFile(t *testi require.NoError(t, manifestStore.Write(original)) resource := syncdomain.ResourceID{Kind: syncdomain.KindVariation, ProjectKey: "project", LookupKey: "config/prompt"} - err = detachResources(Options{Store: store, Baselines: manifestStore}, syncmanifest.Baseline{Lock: original}, []syncdomain.ResourceID{resource}) + err = detachResources( + Options{RepositoryRoot: root, Store: store, Baselines: manifestStore}, + syncmanifest.Baseline{Lock: original}, + []syncdomain.ResourceID{resource}, + ) require.NoError(t, err) exists, err := store.VariationExists("project", "config", "prompt") @@ -145,7 +152,11 @@ func TestDetachResourcesRemovesManifestEntryWhenWrapperWasAlreadyDeleted(t *test require.NoError(t, manifestStore.Write(original)) resource := syncdomain.ResourceID{Kind: syncdomain.KindVariation, ProjectKey: "project", LookupKey: "config/deleted"} - err := detachResources(Options{Store: store, Baselines: manifestStore}, syncmanifest.Baseline{Lock: original}, []syncdomain.ResourceID{resource}) + err := detachResources( + Options{RepositoryRoot: root, Store: store, Baselines: manifestStore}, + syncmanifest.Baseline{Lock: original}, + []syncdomain.ResourceID{resource}, + ) require.NoError(t, err) baseline, err := manifestStore.Load([]string{"project"}) @@ -165,7 +176,7 @@ func TestDetachResourcesDeletesUnreadableWrapper(t *testing.T) { resource := syncdomain.ResourceID{Kind: syncdomain.KindVariation, ProjectKey: "project", LookupKey: "config/broken"} err := detachResources( - Options{Store: store, Baselines: manifestStore}, + Options{RepositoryRoot: root, Store: store, Baselines: manifestStore}, syncmanifest.Baseline{Lock: syncmanifest.New()}, []syncdomain.ResourceID{resource}, ) @@ -193,6 +204,7 @@ func TestRunRequiresTerminalWhenResourcesExist(t *testing.T) { Baselines: newMemoryManifestStore(), Input: bytes.NewBuffer(nil), Output: bytes.NewBuffer(nil), + Kind: syncdomain.KindVariation, }) require.ErrorContains(t, err, "requires a terminal") @@ -218,6 +230,7 @@ func TestRunUsesExplicitSelectionsWithoutTerminal(t *testing.T) { Output: bytes.NewBuffer(nil), Selections: []syncdomain.ResourceID{selection}, NoInput: true, + Yes: true, }) require.NoError(t, err) @@ -236,7 +249,7 @@ func TestRunListsOnlyTheResourcesThatTheFilterKeeps(t *testing.T) { require.NoError(t, err) options := Options{ RepositoryRoot: root, Store: store, Baselines: newMemoryManifestStore(), - Input: bytes.NewBuffer(nil), NoInput: true, + Input: bytes.NewBuffer(nil), NoInput: true, Kind: syncdomain.KindVariation, } var otherProject bytes.Buffer @@ -261,6 +274,7 @@ func TestRunReportsWhenNoResourcesAreSynced(t *testing.T) { Baselines: newMemoryManifestStore(), Input: bytes.NewBuffer(nil), Output: &output, + Kind: syncdomain.KindVariation, }) require.NoError(t, err) @@ -295,10 +309,11 @@ func TestRunArchive(t *testing.T) { wantArchived []string wantDetached bool }{ - "detach without archive keeps the variation in LaunchDarkly": {wantDetached: true}, + "detach without archive keeps the variation in LaunchDarkly": {yes: true, wantDetached: true}, "archive with yes": {archive: true, yes: true, wantArchived: []string{"project/config/prompt"}, wantDetached: true}, "variation already archived": {archive: true, yes: true, archiver: recordingArchiver{preArchived: true}, wantDetached: true}, "config gone in LaunchDarkly": {archive: true, yes: true, archiver: recordingArchiver{readErr: notFound}, wantDetached: true}, + "detach needs a confirmation": {wantError: "rerun with --yes"}, "archive needs a confirmation": {archive: true, wantError: "rerun with --yes"}, "archive failure stops detach": { archive: true, yes: true, archiver: recordingArchiver{err: errors.New("forbidden")}, @@ -372,6 +387,7 @@ func TestRunArchiveCanRunAgainAfterTheDeleteFails(t *testing.T) { workspace.requireTracked(t, "prompt", false) } +// One question covers the detach and the archive. func TestRunArchiveAsksForConfirmation(t *testing.T) { tests := map[string]struct { answer string @@ -395,7 +411,9 @@ func TestRunArchiveAsksForConfirmation(t *testing.T) { require.NoError(t, Run(options)) - assert.Contains(t, output.String(), "Archive these variations in LaunchDarkly? [y/N]") + assert.Contains(t, output.String(), "Resources to detach and archive in LaunchDarkly:\n variation project/config/prompt") + assert.Equal(t, 1, strings.Count(output.String(), "[y/N]")) + assert.Contains(t, output.String(), "Detach these resources? [y/N]") assert.Contains(t, output.String(), test.wantOutput) assert.Equal(t, test.wantArchived, archiver.archived) workspace.requireTracked(t, "prompt", test.wantArchived == nil) @@ -403,6 +421,123 @@ func TestRunArchiveAsksForConfirmation(t *testing.T) { } } +func TestRunDetachesToolsAndSkills(t *testing.T) { + workspace := newArchiveWorkspace(t, "prompt") + tool, skill := testTool(), testSkill() + workspace.addAttachments(t, tool, skill) + archiver := &recordingArchiver{} + options := workspace.options(archiver) + options.Selections = []syncdomain.ResourceID{tool.ID("project"), skill.ID("project")} + options.Yes = true + + require.NoError(t, Run(options)) + + assert.Empty(t, archiver.archived, "LaunchDarkly keeps the tool and the skill") + assert.Equal(t, "Detached resources:\n- tool project/search\n- skill project/summarize\n", workspace.output(options)) + local, err := synclocal.CompileAttachments(workspace.root, func(syncdomain.ResourceID) bool { return true }) + require.NoError(t, err) + assert.Empty(t, local) + assert.Equal(t, []syncdomain.ResourceID{syncdomain.VariationID("project", "config", "prompt")}, lockIDs(workspace.baselines.manifest)) +} + +// --archive archives only variations. LaunchDarkly keeps a detached tool or +// skill, so a tool in the same selection must not stop the archive or the +// detach. +func TestRunArchiveWithAToolArchivesOnlyTheVariation(t *testing.T) { + workspace := newArchiveWorkspace(t, "prompt") + tool := testTool() + workspace.addAttachments(t, tool) + archiver := &recordingArchiver{} + options := workspace.options(archiver) + options.Selections = append(options.Selections, tool.ID("project")) + options.Archive, options.Yes = true, true + + require.NoError(t, Run(options)) + + assert.Equal(t, []string{"project/config/prompt"}, archiver.archived) + workspace.requireTracked(t, "prompt", false) + assert.Empty(t, workspace.baselines.manifest.Resources) + local, err := synclocal.CompileAttachments(workspace.root, func(syncdomain.ResourceID) bool { return true }) + require.NoError(t, err) + assert.Empty(t, local) +} + +// Detach deletes the variation, tool, and skill files in one transaction. When +// one file cannot be deleted, every file stays, and so does the baseline. +func TestRunKeepsEveryFileWhenOneDeleteFails(t *testing.T) { + workspace := newArchiveWorkspace(t, "prompt") + tool := testTool() + workspace.addAttachments(t, tool) + // A symbolic link in place of the tool file makes its delete fail. + toolFile := filepath.Join(workspace.root, syncdomain.RootDir, "project", "tools", "search.json") + target := filepath.Join(t.TempDir(), "search.json") + require.NoError(t, os.Rename(toolFile, target)) + require.NoError(t, os.Symlink(target, toolFile)) + options := workspace.options(&recordingArchiver{}) + options.Selections = append(options.Selections, tool.ID("project")) + options.Yes = true + + require.Error(t, Run(options)) + + workspace.requireTracked(t, "prompt", true) + _, err := os.Lstat(toolFile) + require.NoError(t, err) +} + +func TestRunRefusesAToolThatAVariationStillUses(t *testing.T) { + workspace := newArchiveWorkspace(t) + tool := testTool() + variation := testVariation("prompt") + variation.Tools = []syncdomain.AttachmentRef{{Key: "search"}} + variation.Attachments = []syncdomain.Attachment{tool} + _, err := workspace.store.Add([]synclocal.VariationFile{{ProjectKey: "project", ConfigKey: "config", Variation: variation}}) + require.NoError(t, err) + prompt := syncdomain.VariationID("project", "config", "prompt") + workspace.baselines.manifest.SetFingerprint(prompt, testFingerprint()) + require.NoError(t, workspace.baselines.manifest.SetAttachments("project", variation.Attachments)) + options := workspace.options(&recordingArchiver{}) + options.Yes = true + + options.Selections = []syncdomain.ResourceID{tool.ID("project")} + require.EqualError(t, Run(options), + "tool project/search is used by variation project/config/prompt. Remove it from that variation first.") + assert.Len(t, workspace.baselines.manifest.Resources, 2, "nothing changes") + + options.Selections = []syncdomain.ResourceID{prompt, tool.ID("project")} + require.NoError(t, Run(options), "a detach of the variation too frees the tool") + assert.Empty(t, workspace.baselines.manifest.Resources) +} + +func TestRunMarksResourcesWithLocalChanges(t *testing.T) { + workspace := newArchiveWorkspace(t, "edited", "synced") + tool := testTool() + workspace.addAttachments(t, tool) + variations, err := synclocal.CompileWorkspace(workspace.root) + require.NoError(t, err) + synced := syncdomain.VariationID("project", "config", "synced") + fingerprint, err := syncdomain.FingerprintVariation("project", synced.LookupKey, variations[1].Variation) + require.NoError(t, err) + workspace.baselines.manifest.SetFingerprint(synced, fingerprint) + lockOnly := syncdomain.VariationID("project", "config", "lock-only") + workspace.baselines.manifest.SetFingerprint(lockOnly, testFingerprint()) + + var output bytes.Buffer + options := workspace.options(&recordingArchiver{}) + options.Selections = append(options.Selections, tool.ID("project"), lockOnly) + options.NoInput, options.Input, options.Output = false, strings.NewReader("n\n"), &output + options.isTerminal = func(io.Reader, io.Writer) bool { return true } + + require.NoError(t, Run(options)) + + assert.Equal(t, "Resources to detach:\n"+ + " variation project/config/edited (local changes will be lost)\n"+ + " variation project/config/synced\n"+ + " tool project/search\n"+ + " variation project/config/lock-only\n"+ + "\nDetach these resources? [y/N] Detach canceled.\n", output.String()) + assert.Len(t, workspace.baselines.manifest.Resources, 4, "a no changes nothing") +} + // archiveWorkspace is a workspace whose variations have local files and // baseline entries. type archiveWorkspace struct { @@ -443,6 +578,47 @@ func (workspace archiveWorkspace) options(archiver Archiver) Options { } } +// addAttachments writes standalone tool and skill files with baseline entries +// that match them. +func (workspace archiveWorkspace) addAttachments(t *testing.T, attachments ...syncdomain.Attachment) { + t.Helper() + synced := make([]syncdomain.SyncedAttachment, 0, len(attachments)) + for _, attachment := range attachments { + synced = append(synced, syncdomain.SyncedAttachment{ProjectKey: "project", Attachment: attachment}) + } + _, err := workspace.store.ReplaceAttachments(synced) + require.NoError(t, err) + require.NoError(t, workspace.baselines.manifest.SetAttachments("project", attachments)) +} + +// output returns what Run wrote to the output of options. +func (archiveWorkspace) output(options Options) string { + return options.Output.(*bytes.Buffer).String() +} + +func lockIDs(manifest syncmanifest.Manifest) []syncdomain.ResourceID { + ids := make([]syncdomain.ResourceID, 0, len(manifest.Resources)) + for _, resource := range manifest.Resources { + ids = append(ids, resource.ID()) + } + return ids +} + +func testTool() syncdomain.Attachment { + description := "Search documentation" + return syncdomain.Attachment{ + Kind: syncdomain.AttachmentTool, + Tool: &syncdomain.Tool{Key: "search", Description: &description, Schema: map[string]any{"type": "object"}}, + } +} + +func testSkill() syncdomain.Attachment { + return syncdomain.Attachment{ + Kind: syncdomain.AttachmentSkill, + Skill: &syncdomain.Skill{Key: "summarize", Description: "Summarize", Markdown: "# Summarize\n"}, + } +} + // requireTracked checks both the local file and the baseline entry. func (workspace archiveWorkspace) requireTracked(t *testing.T, key string, tracked bool) { t.Helper() diff --git a/internal/sync/local/attachment.go b/internal/sync/local/attachment.go index 89235950..ea3b57f5 100644 --- a/internal/sync/local/attachment.go +++ b/internal/sync/local/attachment.go @@ -2,14 +2,12 @@ package local import ( "bytes" - "cmp" "encoding/json" "errors" "fmt" "io" "io/fs" "maps" - "os" "path" "path/filepath" "slices" @@ -37,14 +35,6 @@ type skillFrontMatter struct { Upsert bool `yaml:"upsert,omitempty"` } -// OrphanedAttachment is a tool or skill file that no variation uses. -type OrphanedAttachment struct { - ProjectKey string - Kind syncdomain.AttachmentKind - Key string - Path string -} - // CompileAttachments reads each tool and skill file that include accepts. It // does not read the other files, so an invalid file outside include does not // stop the read. Like CompileWorkspace, it rejects a symbolic link in a @@ -292,70 +282,3 @@ func preserveLocalFields(filePath string, original, replacement []byte) ([]byte, updated.Name, updated.Upsert = local.Name, updated.Upsert || local.Upsert return renderSkillFile(updated, string(markdown)) } - -// OrphanedAttachments returns each tool and skill file that no local -// variation uses. -func (store Store) OrphanedAttachments() ([]OrphanedAttachment, error) { - variations, err := CompileWorkspace(store.repositoryRoot) - if err != nil && !errors.Is(err, ErrNoDirectory) { - return nil, err - } - used := make(map[syncdomain.ResourceID]struct{}) - for _, variation := range variations { - for _, attachment := range variation.Variation.Attachments { - used[attachment.ID(variation.ProjectKey)] = struct{}{} - } - } - - var orphaned []OrphanedAttachment - err = filepath.WalkDir(store.root, func(filePath string, entry fs.DirEntry, err error) error { - if err != nil || entry.IsDir() { - return err - } - relativePath, err := filepath.Rel(store.repositoryRoot, filePath) - if err != nil { - return err - } - id, ok := ParseManagedPath(filepath.ToSlash(relativePath)) - if !ok || id.Kind == syncdomain.KindVariation { - return nil - } - if _, isUsed := used[id]; !isUsed { - orphaned = append(orphaned, OrphanedAttachment{ - ProjectKey: id.ProjectKey, - Kind: syncdomain.AttachmentKind(id.Kind), - Key: id.LookupKey, - Path: strings.TrimPrefix(filepath.ToSlash(relativePath), syncdomain.RootDir+"/"), - }) - } - return nil - }) - if errors.Is(err, os.ErrNotExist) { - return nil, nil - } - if err != nil { - return nil, fmt.Errorf("find unreferenced attachments: %w", err) - } - - slices.SortFunc(orphaned, func(left, right OrphanedAttachment) int { - return cmp.Or( - strings.Compare(left.ProjectKey, right.ProjectKey), - strings.Compare(string(left.Kind), string(right.Kind)), - strings.Compare(left.Key, right.Key), - ) - }) - return orphaned, nil -} - -// DeleteAttachments deletes the files of tools and skills in one transaction. -func (store Store) DeleteAttachments(ids []syncdomain.ResourceID) ([]string, error) { - paths := make([]string, 0, len(ids)) - for _, id := range ids { - relativePath, err := attachmentPath(id.ProjectKey, syncdomain.AttachmentKind(id.Kind), id.LookupKey) - if err != nil { - return nil, err - } - paths = append(paths, relativePath) - } - return store.deleteFiles(paths) -} diff --git a/internal/sync/local/attachment_test.go b/internal/sync/local/attachment_test.go index ce973489..2d23151a 100644 --- a/internal/sync/local/attachment_test.go +++ b/internal/sync/local/attachment_test.go @@ -241,68 +241,29 @@ func TestReplaceVariationsLeavesAttachmentUnchangedWhenPreflightFails(t *testing assert.Equal(t, oldDescription, *attachment.Tool.Description) } -func TestOrphanedAttachmentsRequireEveryLocalReferenceToBeRemoved(t *testing.T) { +func TestDeleteResourcesDeletesTheFilesOfEveryKindByID(t *testing.T) { root := t.TempDir() store := NewStore(root) - tool := syncdomain.Tool{Key: "search", Schema: map[string]any{"type": "object"}} - first, second := localVariation("first"), localVariation("second") - for _, variation := range []*VariationFile{&first, &second} { - variation.Variation.Tools = []syncdomain.AttachmentRef{{Key: tool.Key}} - variation.Variation.Attachments = []syncdomain.Attachment{{ - Kind: syncdomain.AttachmentTool, Tool: &tool, - }} - } - _, err := store.Add([]VariationFile{first, second}) - require.NoError(t, err) - - first.Variation.Tools = nil - first.Variation.Attachments = nil - _, err = store.ReplaceVariations([]VariationReplacement{{ - ProjectKey: first.ProjectKey, ConfigKey: first.ConfigKey, Variation: first.Variation, - }}) - require.NoError(t, err) - orphaned, err := store.OrphanedAttachments() - require.NoError(t, err) - assert.Empty(t, orphaned) - - second.Variation.Tools = nil - second.Variation.Attachments = nil - _, err = store.ReplaceVariations([]VariationReplacement{{ - ProjectKey: second.ProjectKey, ConfigKey: second.ConfigKey, Variation: second.Variation, - }}) - require.NoError(t, err) - orphaned, err = store.OrphanedAttachments() - require.NoError(t, err) - require.Equal(t, []OrphanedAttachment{{ - ProjectKey: "project", Kind: syncdomain.AttachmentTool, Key: "search", Path: "project/tools/search.json", - }}, orphaned) - - deleted, err := store.DeleteAttachments([]syncdomain.ResourceID{{Kind: syncdomain.KindTool, ProjectKey: "project", LookupKey: "search"}}) - require.NoError(t, err) - assert.Equal(t, []string{"project/tools/search.json"}, deleted) - _, statErr := os.Stat(filepath.Join(root, ".launchdarkly", "project", "tools", "search.json")) - require.ErrorIs(t, statErr, os.ErrNotExist) -} - -func TestOrphanedAttachmentsFindsFlatSkillFile(t *testing.T) { - root := t.TempDir() + toolPath := filepath.Join(root, ".launchdarkly", "project", "tools", "search.json") skillPath := filepath.Join(root, ".launchdarkly", "project", "skills", "support.md") - require.NoError(t, os.MkdirAll(filepath.Dir(skillPath), 0o755)) - require.NoError(t, os.WriteFile(skillPath, []byte("# Support\n"), 0o644)) - - store := NewStore(root) - orphaned, err := store.OrphanedAttachments() + variationPath := filepath.Join(root, ".launchdarkly", "project", "configs", "config", "default.prompt.md") + for _, file := range []string{toolPath, skillPath, variationPath} { + require.NoError(t, os.MkdirAll(filepath.Dir(file), 0o755)) + require.NoError(t, os.WriteFile(file, []byte("content\n"), 0o644)) + } - require.NoError(t, err) - require.Equal(t, []OrphanedAttachment{{ - ProjectKey: "project", Kind: syncdomain.AttachmentSkill, Key: "support", Path: "project/skills/support.md", - }}, orphaned) + deleted, err := store.DeleteResources([]syncdomain.ResourceID{ + {Kind: syncdomain.KindTool, ProjectKey: "project", LookupKey: "search"}, + {Kind: syncdomain.KindSkill, ProjectKey: "project", LookupKey: "support"}, + syncdomain.VariationID("project", "config", "default"), + }) - deleted, err := store.DeleteAttachments([]syncdomain.ResourceID{{Kind: syncdomain.KindSkill, ProjectKey: "project", LookupKey: "support"}}) require.NoError(t, err) - assert.Equal(t, []string{"project/skills/support.md"}, deleted) - _, statErr := os.Stat(skillPath) - require.ErrorIs(t, statErr, os.ErrNotExist) + assert.Equal(t, []string{"project/tools/search.json", "project/skills/support.md", "project/configs/config/default.prompt.md"}, deleted) + for _, file := range []string{toolPath, skillPath, variationPath} { + _, statErr := os.Stat(file) + require.ErrorIs(t, statErr, os.ErrNotExist) + } } func TestCompileAttachmentsReadsStandaloneAndReferencedFiles(t *testing.T) { diff --git a/internal/sync/local/delete.go b/internal/sync/local/delete.go index 18750c2b..bc698515 100644 --- a/internal/sync/local/delete.go +++ b/internal/sync/local/delete.go @@ -5,6 +5,8 @@ import ( "fmt" "os" "path/filepath" + + syncdomain "github.com/launchdarkly/ldcli/internal/sync" ) // stagedDeletion is one file in a delete transaction. The file first moves to @@ -28,6 +30,20 @@ func (store Store) DeleteVariations(deletions []VariationDeletion) ([]string, er return store.deleteFiles(paths) } +// DeleteResources deletes the files of variations, tools, and skills in one +// transaction, so that a failure deletes none of them. It returns their paths. +func (store Store) DeleteResources(ids []syncdomain.ResourceID) ([]string, error) { + paths := make([]string, 0, len(ids)) + for _, id := range ids { + relativePath, err := resourcePath(id) + if err != nil { + return nil, err + } + paths = append(paths, relativePath) + } + return store.deleteFiles(paths) +} + // deleteFiles deletes a batch of managed files in one transaction. It first // moves every file to a backup beside it. If a move fails, it moves the // files back. When every file has moved, it removes the backups. diff --git a/internal/sync/local/layout.go b/internal/sync/local/layout.go index f992d7d3..29506dc6 100644 --- a/internal/sync/local/layout.go +++ b/internal/sync/local/layout.go @@ -91,6 +91,19 @@ func attachmentPath(projectKey string, kind syncdomain.AttachmentKind, key strin } } +// resourcePath returns the path of the file of a variation, a tool, or a +// skill relative to the managed directory. +func resourcePath(id syncdomain.ResourceID) (string, error) { + if id.Kind != syncdomain.KindVariation { + return attachmentPath(id.ProjectKey, syncdomain.AttachmentKind(id.Kind), id.LookupKey) + } + configKey, variationKey, err := id.VariationKeys() + if err != nil { + return "", err + } + return variationPath(id.ProjectKey, configKey, variationKey) +} + // validateKey makes sure that key is a safe path segment. The name is the kind // of key, for example "project", and appears in the error. func validateKey(name, key string) error { diff --git a/internal/sync/manifest/model.go b/internal/sync/manifest/model.go index 4290f896..d3c64345 100644 --- a/internal/sync/manifest/model.go +++ b/internal/sync/manifest/model.go @@ -116,24 +116,6 @@ func (manifest *Manifest) setAttachments(projectKey string, attachments []syncdo return nil } -// RemoveUnusedAttachments stops tracking each tool and skill that none of the -// variations references. -func (manifest *Manifest) RemoveUnusedAttachments(variations []syncdomain.SyncedResource) { - used := make(map[syncdomain.ResourceID]struct{}) - for _, variation := range variations { - for _, attachment := range variation.Variation.Attachments { - used[attachment.ID(variation.ProjectKey)] = struct{}{} - } - } - manifest.Resources = slices.DeleteFunc(manifest.Resources, func(resource Resource) bool { - if resource.ResourceKind == syncdomain.KindVariation { - return false - } - _, ok := used[resource.ID()] - return !ok - }) -} - // WithChanges returns a copy of the manifest with the entry changes from // before to after. An entry that is the same in before and after keeps the // value that the manifest has, which can come from another working copy. diff --git a/internal/sync/manifest/model_test.go b/internal/sync/manifest/model_test.go index 4f92975f..0efbd868 100644 --- a/internal/sync/manifest/model_test.go +++ b/internal/sync/manifest/model_test.go @@ -59,24 +59,3 @@ func TestManifestPreservesExistingAttachmentBaselineWhenAddingConsumer(t *testin require.Len(t, manifest.Resources, 1) assert.Equal(t, "sha256:existing", manifest.Resources[0].Fingerprint) } - -func TestManifestRemovesOnlyUnreferencedAttachments(t *testing.T) { - manifest := New() - manifest.Resources = []Resource{ - {ResourceKind: syncdomain.KindVariation, ProjectKey: "project", LookupKey: "config/default"}, - {ResourceKind: syncdomain.KindTool, ProjectKey: "project", LookupKey: "search"}, - {ResourceKind: syncdomain.KindSkill, ProjectKey: "project", LookupKey: "support"}, - } - search := syncdomain.Attachment{Kind: syncdomain.AttachmentTool, Tool: &syncdomain.Tool{Key: "search"}} - variations := []syncdomain.SyncedResource{{ - ProjectKey: "project", - Variation: syncdomain.Variation{Attachments: []syncdomain.Attachment{search}}, - }} - - manifest.RemoveUnusedAttachments(variations) - - assert.Equal(t, []Resource{ - {ResourceKind: syncdomain.KindVariation, ProjectKey: "project", LookupKey: "config/default"}, - {ResourceKind: syncdomain.KindTool, ProjectKey: "project", LookupKey: "search"}, - }, manifest.Resources) -} diff --git a/internal/sync/prompt/acceptance_test.go b/internal/sync/prompt/acceptance_test.go index ee26b09e..9a4bb516 100644 --- a/internal/sync/prompt/acceptance_test.go +++ b/internal/sync/prompt/acceptance_test.go @@ -611,7 +611,9 @@ func TestPromptPullsLatestToolAndAdvancesVariationPin(t *testing.T) { assertManifestFingerprint(t, root, expected) } -func TestPromptDeletesUnreferencedLocalToolWithYes(t *testing.T) { +// A tool file that no variation uses syncs on its own, so a sync keeps the +// file and both of its rows. +func TestPromptKeepsUnreferencedLocalToolWithYes(t *testing.T) { root := initRepository(t) tool := syncdomain.Tool{Key: "search", Schema: map[string]any{"type": "object"}} baseline := variation("Baseline") @@ -639,15 +641,52 @@ func TestPromptDeletesUnreferencedLocalToolWithYes(t *testing.T) { assert.Empty(t, api.variation.Tools) assert.Contains(t, api.tools, "search") _, err = os.Stat(filepath.Join(root, ".launchdarkly", "production", "tools", "search.json")) - require.ErrorIs(t, err, os.ErrNotExist) - // A sync keeps the row of a tool that no variation uses. Only a detach - // removes it. + require.NoError(t, err, "the tool file stays") searchFingerprint, err := syncdomain.FingerprintAttachment("production", toolAttachment(tool, 2)) require.NoError(t, err) assert.Equal(t, []syncmanifest.Resource{ {ResourceKind: syncdomain.KindTool, ProjectKey: "production", LookupKey: "search", Fingerprint: searchFingerprint}, {ResourceKind: syncdomain.KindVariation, ProjectKey: "production", LookupKey: "support/default", Fingerprint: fingerprint(t, detached)}, }, readLock(t, root).Resources) + assert.Contains(t, manifestsByRoot[root].Items, syncapi.SyncManifestResource{ + ResourceKind: syncdomain.KindTool, ResourceLookupKey: "search", Fingerprint: searchFingerprint, Version: 1, + }) +} + +// A detach of a variation keeps the tools that it used, with their files +// and rows. Without --yes and without a terminal, detach changes nothing. +func TestPromptDetachKeepsToolsOfTheVariation(t *testing.T) { + root := initRepository(t) + tool := syncdomain.Tool{Key: "search", Schema: map[string]any{"type": "object"}} + baseline := variation("Baseline") + baseline.Tools = []syncdomain.AttachmentRef{{Key: "search", Version: 1}} + baseline.Attachments = []syncdomain.Attachment{toolAttachment(tool, 1)} + writeVariation(t, root, baseline, true) + writeManifest(t, root, baseline) + api := &directAPI{variation: pointer(baseline), tools: map[string]versionedTool{"search": {Tool: tool, Version: 1}}} + detach := []string{"detach", "--project", "production", "--config-key", "support", "--key", "default", "--no-input"} + toolFile := filepath.Join(root, syncdomain.RootDir, "production", "tools", "search.json") + + _, _, err := runPrompt(t, root, api, detach...) + require.ErrorContains(t, err, "rerun with --yes") + assert.Len(t, readLock(t, root).Resources, 2, "nothing changes") + + stdout, _, err := runPrompt(t, root, api, append(detach, "--yes")...) + require.NoError(t, err) + assert.Equal(t, "Detached resources:\n- variation production/support/default\n", stdout) + _, err = os.Stat(toolFile) + require.NoError(t, err, "the tool file stays") + assert.Equal(t, []syncdomain.ResourceID{{Kind: syncdomain.KindTool, ProjectKey: "production", LookupKey: "search"}}, + lockResourceIDs(readLock(t, root))) + assert.Len(t, manifestsByRoot[root].Items, 1) +} + +func lockResourceIDs(lock syncmanifest.Manifest) []syncdomain.ResourceID { + ids := make([]syncdomain.ResourceID, 0, len(lock.Resources)) + for _, resource := range lock.Resources { + ids = append(ids, resource.ID()) + } + return ids } func TestPromptUpdateResolvesOmittedModelConfigVersionToLatest(t *testing.T) { diff --git a/internal/sync/prompt/action.go b/internal/sync/prompt/action.go index 8b31dfbb..df9c7cbe 100644 --- a/internal/sync/prompt/action.go +++ b/internal/sync/prompt/action.go @@ -44,13 +44,13 @@ type AttachAction struct { Target *syncdomain.ResourceID } -// DetachAction stops syncing selected local variations. With Archive, it -// also archives them in LaunchDarkly. The variations come from positional -// selectors or from the target flags in Filter. +// DetachAction stops syncing selected local resources. With Archive, it +// also archives the variations in LaunchDarkly. The resources come from +// positional selectors or from the target flags in Filter. type DetachAction struct { - Variations []syncdomain.ResourceID - Filter syncdomain.Filter - Archive bool + Resources []syncdomain.ResourceID + Filter syncdomain.Filter + Archive bool } // LinkAction creates a variation whose prompt is an external file, and then @@ -74,7 +74,7 @@ func (action AddAction) validate() error { } func (action DetachAction) validate() error { - return validateSelections(action.Variations, action.Filter) + return validateSelections(action.Resources, action.Filter) } func (action AttachAction) validate() error { diff --git a/internal/sync/prompt/attach.go b/internal/sync/prompt/attach.go index 54174ccb..443b5572 100644 --- a/internal/sync/prompt/attach.go +++ b/internal/sync/prompt/attach.go @@ -50,11 +50,16 @@ func attachToVariation(store synclocal.Store, client syncapi.Client, options att return err } - // Other local variations can share the attachment file. Keep its local - // content, so that attach never overwrites a local edit. The next sync - // shows a conflict if the content differs from LaunchDarkly. - if local, ok := localAttachment(variations, attachment.ID(projectKey)); ok { - attachment = local + // The attachment can already have a local file, which other variations use + // or which syncs on its own. Keep its local content, so that attach never + // overwrites a local edit. The next sync shows a conflict if the content + // differs from LaunchDarkly. + local, err := localAttachment(options.RepositoryRoot, attachment.ID(projectKey)) + if err != nil { + return err + } + if local != nil { + attachment = *local } variation := target.Variation variation.Attach(attachment) @@ -72,17 +77,14 @@ func attachToVariation(store synclocal.Store, client syncapi.Client, options att return err } -// localAttachment finds the local content of an attachment. -func localAttachment(variations []syncdomain.SyncedResource, id ResourceID) (syncdomain.Attachment, bool) { - for _, variation := range variations { - if variation.ProjectKey != id.ProjectKey { - continue - } - if attachment, ok := variation.Variation.Attachment(syncdomain.AttachmentKind(id.Kind), id.LookupKey); ok { - return attachment, true - } +// localAttachment reads the local file of an attachment. It returns nil when +// the attachment has no local file. +func localAttachment(repositoryRoot string, id ResourceID) (*syncdomain.Attachment, error) { + local, err := synclocal.CompileAttachments(repositoryRoot, func(candidate ResourceID) bool { return candidate == id }) + if err != nil || len(local) == 0 { + return nil, err } - return syncdomain.Attachment{}, false + return &local[0].Attachment, nil } // selectAttachmentProject returns the project of the attachment. A workspace diff --git a/internal/sync/prompt/execute.go b/internal/sync/prompt/execute.go index e4aa82bd..78cca748 100644 --- a/internal/sync/prompt/execute.go +++ b/internal/sync/prompt/execute.go @@ -286,7 +286,7 @@ func applyAttachmentChange( }); index >= 0 { return 0, fmt.Errorf("%s still uses it", variations[index].ID().Label()) } - if _, err := localStore.DeleteAttachments([]syncdomain.ResourceID{resource.ID}); err != nil { + if _, err := localStore.DeleteResources([]syncdomain.ResourceID{resource.ID}); err != nil { return 0, err } return 0, verifyLocalResult(repositoryRoot, resource) diff --git a/internal/sync/prompt/review.go b/internal/sync/prompt/review.go index 3f75abb2..181fdbad 100644 --- a/internal/sync/prompt/review.go +++ b/internal/sync/prompt/review.go @@ -2,13 +2,9 @@ package prompt import ( "errors" - "slices" - "strings" - syncdomain "github.com/launchdarkly/ldcli/internal/sync" syncconsole "github.com/launchdarkly/ldcli/internal/sync/console" syncinteractive "github.com/launchdarkly/ldcli/internal/sync/interactive" - synclocal "github.com/launchdarkly/ldcli/internal/sync/local" ) const applyQuestion = "\nSync these changes? [y/N] " @@ -51,83 +47,3 @@ func reviewAndConfirmPlan(options Options, plan Plan, interactive bool) (bool, e } return confirmed, nil } - -// cleanupOrphanedAttachments shows the tool and skill files that no local -// variation uses, and deletes them after the user agrees. Without --yes and -// without a terminal, it keeps the files. Before it deletes a file, it makes -// sure again that no variation uses the file. A filtered sync keeps every -// file. A variation outside the filter can have a missing file that a full -// sync restores, and then its tools and skills only look unused. A sync that -// plans tools or skills also keeps every file, because each file is a row of -// its own. -func cleanupOrphanedAttachments(options Options, store synclocal.Store, interactive bool) error { - action := options.syncAction() - if !action.Filter.IsZero() || action.Scope.Includes(syncdomain.KindTool) || action.Scope.Includes(syncdomain.KindSkill) { - return nil - } - orphaned, err := store.OrphanedAttachments() - if err != nil || len(orphaned) == 0 { - return err - } - - console := syncconsole.New(options.ErrorOutput) - _ = console.Line("\nUnreferenced local attachment files:") - currentProject := "" - for _, attachment := range orphaned { - if attachment.ProjectKey != currentProject { - currentProject = attachment.ProjectKey - _ = console.Printf(" Project: %s\n", currentProject) - } - kind := string(attachment.Kind) - _ = console.Printf( - " %s %q\n %s/%s\n", strings.ToUpper(kind[:1])+kind[1:], attachment.Key, syncdomain.RootDir, attachment.Path, - ) - } - - if !options.Yes && !interactive { - _ = console.Line("Unreferenced attachment files kept. Rerun with --yes to delete them.") - return nil - } - if !options.Yes { - confirmed, err := syncinteractive.Confirm( - options.Context, options.Input, options.ErrorOutput, interactive, - "\nDelete these unreferenced local files? [y/N] ", - ) - if err != nil { - return err - } - if !confirmed { - _ = console.Line("Unreferenced attachment files kept.") - return nil - } - } - - // A file can become used again while the user reads the question. - current, err := store.OrphanedAttachments() - if err != nil { - return err - } - orphaned = slices.DeleteFunc(orphaned, func(attachment synclocal.OrphanedAttachment) bool { - return !slices.Contains(current, attachment) - }) - if len(orphaned) == 0 { - _ = console.Line("No selected attachment files remain unreferenced.") - return nil - } - - ids := make([]syncdomain.ResourceID, 0, len(orphaned)) - for _, attachment := range orphaned { - ids = append(ids, syncdomain.ResourceID{ - Kind: syncdomain.Kind(attachment.Kind), ProjectKey: attachment.ProjectKey, LookupKey: attachment.Key, - }) - } - deleted, err := store.DeleteAttachments(ids) - if err != nil { - return err - } - _ = console.Line("Deleted unreferenced attachment files:") - for _, file := range deleted { - _ = console.Printf("- %s/%s\n", syncdomain.RootDir, file) - } - return nil -} diff --git a/internal/sync/prompt/review_test.go b/internal/sync/prompt/review_test.go index ff88b0e0..78fbeaf1 100644 --- a/internal/sync/prompt/review_test.go +++ b/internal/sync/prompt/review_test.go @@ -4,8 +4,6 @@ import ( "bytes" "context" "io" - "os" - "path/filepath" "strings" "sync" "testing" @@ -14,9 +12,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - syncdomain "github.com/launchdarkly/ldcli/internal/sync" syncinteractive "github.com/launchdarkly/ldcli/internal/sync/interactive" - synclocal "github.com/launchdarkly/ldcli/internal/sync/local" ) type notifyingWriter struct { @@ -32,17 +28,6 @@ func (writer *notifyingWriter) Write(data []byte) (int, error) { return writer.Writer.Write(data) } -type callbackReader struct { - io.Reader - once sync.Once - callback func() -} - -func (reader *callbackReader) Read(data []byte) (int, error) { - reader.once.Do(reader.callback) - return reader.Reader.Read(data) -} - func TestConfirmApply(t *testing.T) { tests := []struct { name string @@ -72,93 +57,6 @@ func TestConfirmApply(t *testing.T) { } } -func TestCleanupOrphanedAttachmentsRequiresConfirmationUnlessYes(t *testing.T) { - tests := []struct { - name string - input string - yes bool - interactive bool - wantExist bool - message string - }{ - {name: "declined", input: "n\n", interactive: true, wantExist: true, message: "files kept"}, - {name: "yes flag", yes: true, message: "Deleted unreferenced attachment files"}, - {name: "non-terminal", wantExist: true, message: "Rerun with --yes"}, - } - - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - root, store, _ := newOrphanedToolStore(t) - - var output bytes.Buffer - err := cleanupOrphanedAttachments(Options{ - Yes: test.yes, Input: strings.NewReader(test.input), ErrorOutput: &output, - }, store, test.interactive) - - require.NoError(t, err) - _, statErr := os.Stat(filepath.Join(root, ".launchdarkly", "project", "tools", "search.json")) - assert.Equal(t, test.wantExist, statErr == nil) - assert.Contains(t, output.String(), `Tool "search"`) - assert.Contains(t, output.String(), test.message) - if test.yes { - assert.NotContains(t, output.String(), "Delete these unreferenced local files?") - } - }) - } -} - -func TestCleanupOrphanedAttachmentsRechecksReferencesAfterConfirmation(t *testing.T) { - root, store, attachedVariation := newOrphanedToolStore(t) - input := &callbackReader{ - Reader: strings.NewReader("yes\n"), - callback: func() { - _, err := store.ReplaceVariations([]synclocal.VariationReplacement{{ - ProjectKey: "project", - ConfigKey: "config", - Variation: attachedVariation, - }}) - require.NoError(t, err) - }, - } - var output bytes.Buffer - - err := cleanupOrphanedAttachments(Options{ - Input: input, ErrorOutput: &output, - }, store, true) - - require.NoError(t, err) - _, statErr := os.Stat(filepath.Join(root, ".launchdarkly", "project", "tools", "search.json")) - require.NoError(t, statErr) - assert.Contains(t, output.String(), "No selected attachment files remain unreferenced.") -} - -func newOrphanedToolStore(t *testing.T) (string, synclocal.Store, syncdomain.Variation) { - t.Helper() - root := t.TempDir() - store := synclocal.NewStore(root) - tool := syncdomain.Tool{Key: "search", Schema: map[string]any{"type": "object"}} - attached := syncdomain.Variation{ - Mode: syncdomain.VariationModeAgent, Key: "default", Name: "Default", - Tools: []syncdomain.AttachmentRef{{Key: tool.Key}}, - Attachments: []syncdomain.Attachment{{ - Kind: syncdomain.AttachmentTool, Tool: &tool, - }}, - } - _, err := store.Add([]synclocal.VariationFile{{ - ProjectKey: "project", ConfigKey: "config", Variation: attached, - }}) - require.NoError(t, err) - - orphaned := attached - orphaned.Tools = nil - orphaned.Attachments = nil - _, err = store.ReplaceVariations([]synclocal.VariationReplacement{{ - ProjectKey: "project", ConfigKey: "config", Variation: orphaned, - }}) - require.NoError(t, err) - return root, store, attached -} - func TestReviewAndConfirmPlanStopsWhenWatchContextIsCanceled(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) input, inputWriter := io.Pipe() diff --git a/internal/sync/prompt/runner.go b/internal/sync/prompt/runner.go index e6917df1..4dfa69c3 100644 --- a/internal/sync/prompt/runner.go +++ b/internal/sync/prompt/runner.go @@ -155,8 +155,9 @@ func (runner Runner) runDetach(options Options, workspace syncWorkspace, action ProjectKeys: projectKeys(synced, syncdomain.Filter{}), Input: options.Input, Output: options.Output, + Kind: syncdomain.KindVariation, Filter: action.Filter, - Selections: selections(action.Variations, action.Filter), + Selections: selections(action.Resources, action.Filter), NoInput: options.NoInput, Archive: action.Archive, Archiver: runner.api(options), diff --git a/internal/sync/prompt/runner_test.go b/internal/sync/prompt/runner_test.go index ec0a45da..0179931c 100644 --- a/internal/sync/prompt/runner_test.go +++ b/internal/sync/prompt/runner_test.go @@ -161,18 +161,21 @@ func TestRunnerDetachesWithoutCallingTheAPI(t *testing.T) { root := initGitRepository(t) runner := NewRunner(noopResourceClient{}) called := false + selection := syncdomain.VariationID("project", "config", "default") runner.detach = func(options syncdetach.Options) error { called = true assert.NotZero(t, options.Store) assert.NotZero(t, options.Baselines) assert.Equal(t, os.Stdin, options.Input) assert.Equal(t, io.Discard, options.Output) + assert.Equal(t, syncdomain.KindVariation, options.Kind) + assert.Equal(t, []syncdomain.ResourceID{selection}, options.Selections) return nil } err := runner.Run(Options{ WorkingDirectory: root, - Action: DetachAction{}, + Action: DetachAction{Resources: []syncdomain.ResourceID{selection}}, Input: os.Stdin, Output: io.Discard, ErrorOutput: io.Discard, @@ -277,6 +280,35 @@ func TestAttachToVariationPreservesExistingSharedAttachmentEdits(t *testing.T) { } } +// A tool file that no variation uses yet also keeps its local edit. +func TestAttachToVariationPreservesStandaloneAttachmentEdits(t *testing.T) { + root := t.TempDir() + store := synclocal.NewStore(root) + localDescription, serverDescription := "Local edit", "Server content" + tool := syncdomain.Tool{Key: "search", Description: &localDescription, Schema: map[string]any{"type": "object"}} + _, err := store.Add([]synclocal.VariationFile{{ProjectKey: "project", ConfigKey: "config", Variation: testVariation("default")}}) + require.NoError(t, err) + _, err = store.ReplaceAttachments([]syncdomain.SyncedAttachment{{ + ProjectKey: "project", Attachment: syncdomain.Attachment{Kind: syncdomain.AttachmentTool, Tool: &tool}, + }}) + require.NoError(t, err) + transport := &attachmentMutationAPI{ + current: syncdomain.Tool{Key: "search", Description: &serverDescription, Schema: map[string]any{"type": "object"}}, + version: 2, + } + + err = attachToVariation(store, syncapi.NewClient(transport, "token", "https://example.com"), attachOptions{ + RepositoryRoot: root, ProjectKey: "project", VariationID: "config/default", + Kind: syncdomain.AttachmentTool, Key: "search", + }) + + require.NoError(t, err) + resources, err := synclocal.CompileWorkspace(root) + require.NoError(t, err) + require.Len(t, resources[0].Variation.Attachments, 1) + assert.Equal(t, localDescription, *resources[0].Variation.Attachments[0].Tool.Description) +} + func TestEmptyAttachmentSearchReturnsNoResults(t *testing.T) { client := syncapi.NewClient(emptyAttachmentClient{}, "token", "https://example.com") diff --git a/internal/sync/prompt/sync.go b/internal/sync/prompt/sync.go index 34998336..cec9598a 100644 --- a/internal/sync/prompt/sync.go +++ b/internal/sync/prompt/sync.go @@ -61,9 +61,6 @@ func (runner Runner) runWorkspaceSync(options Options, workspace syncWorkspace, return err } if !proceed { - if !resolved.HasChanges() { - return cleanupOrphanedAttachments(options, workspace.local, interactive) - } return nil } @@ -75,7 +72,7 @@ func (runner Runner) runWorkspaceSync(options Options, workspace syncWorkspace, if err := checkUnchanged(options, reviewed, current); err != nil { return err } - return runner.applyPlan(options, workspace, client, current, conflicts.resolutions, interactive) + return runner.applyPlan(options, workspace, client, current, conflicts.resolutions) } // checkUnchanged returns an error if the state changed after the review. @@ -93,15 +90,13 @@ func checkUnchanged(options Options, reviewed, current workspaceState) error { } // applyPlan applies the conflict choices to the current plan, executes it, -// and records the new baseline. It removes unused attachment files only when -// every step succeeds. +// and records the new baseline. func (runner Runner) applyPlan( options Options, workspace syncWorkspace, client syncapi.Client, current workspaceState, resolutions map[syncdomain.ResourceID]ConflictResolution, - interactive bool, ) error { plan := applyConflictResolutions(current.plan, resolutions) outcomes, next, err := executePlan(workspace.root, workspace.local, client, current.baseline.Lock, plan, current.localFiles) @@ -116,8 +111,5 @@ func (runner Runner) applyPlan( if err := writeOutcomeOutput(options.Output, options.OutputKind, plan, outcomes); err != nil { failures = append(failures, err) } - if err := errors.Join(failures...); err != nil { - return err - } - return cleanupOrphanedAttachments(options, workspace.local, interactive) + return errors.Join(failures...) }