From adca116153f1699038c4b9b0378bfae7e6a118f1 Mon Sep 17 00:00:00 2001 From: Clifford Tawiah Date: Thu, 8 Oct 2026 17:32:21 -0400 Subject: [PATCH] feat(sync): keep each working copy's baseline in sync.lock All clones of a repository shared one remote manifest as their sync baseline. A clone that was behind then looked like it changed its files back, so sync wrote the older content to LaunchDarkly or archived new variations. Each working copy now keeps its baseline in .launchdarkly/sync.lock. Engineers commit the file, so the baseline moves with the files on each pull and branch switch. The plan compares local files and LaunchDarkly with the lock. A clone that is behind pulls the newer state, and two clones that edit one variation get a conflict. The remote manifest still supplies the version of each entry. Each save sends the version that sync read, so LaunchDarkly rejects two saves of one entry at the same time. A working copy without sync.lock uses the remote manifest once, and its next sync writes the file. The review shows a note and a suggestion when another working copy synced a different state of a variation. The lock lists every tracked project, so sync no longer asks Git for deleted files. --- internal/sync/bootstrap/bootstrap.go | 14 +- internal/sync/bootstrap/bootstrap_test.go | 54 +++--- internal/sync/detach/detach.go | 48 ++--- internal/sync/detach/detach_test.go | 43 +++-- internal/sync/local/layout.go | 3 + internal/sync/local/lock.go | 50 +++++ internal/sync/local/lock_test.go | 47 +++++ internal/sync/manifest/baseline.go | 97 ++++++++++ internal/sync/manifest/baseline_test.go | 222 ++++++++++++++++++++++ internal/sync/manifest/lock.go | 105 ++++++++++ internal/sync/manifest/model.go | 54 +++++- internal/sync/manifest/store.go | 27 +-- internal/sync/prompt/acceptance_test.go | 42 +++- internal/sync/prompt/conflict_test.go | 21 +- internal/sync/prompt/output.go | 52 +++-- internal/sync/prompt/output_test.go | 20 ++ internal/sync/prompt/plan.go | 4 + internal/sync/prompt/runner.go | 27 ++- internal/sync/prompt/runner_test.go | 2 +- internal/sync/prompt/state.go | 48 ++--- internal/sync/prompt/sync.go | 12 +- internal/sync/prompt/watch_test.go | 13 ++ internal/sync/repository/git.go | 36 ---- internal/sync/repository/git_test.go | 19 -- 24 files changed, 821 insertions(+), 239 deletions(-) create mode 100644 internal/sync/local/lock.go create mode 100644 internal/sync/local/lock_test.go create mode 100644 internal/sync/manifest/baseline.go create mode 100644 internal/sync/manifest/baseline_test.go create mode 100644 internal/sync/manifest/lock.go diff --git a/internal/sync/bootstrap/bootstrap.go b/internal/sync/bootstrap/bootstrap.go index 3a76eb84..3eefd8d7 100644 --- a/internal/sync/bootstrap/bootstrap.go +++ b/internal/sync/bootstrap/bootstrap.go @@ -31,18 +31,12 @@ type AttachmentReader interface { ReadAttachment(projectKey string, kind syncdomain.AttachmentKind, key string) (syncdomain.Attachment, error) } -// ManifestStore reads and writes the sync baseline. -type ManifestStore interface { - Load(projectKeys []string) (syncmanifest.Manifest, error) - Update(previous, next syncmanifest.Manifest) (syncmanifest.Manifest, error) -} - // Options are the dependencies and the input of one add. type Options struct { Catalog Catalog Attachments AttachmentReader Store synclocal.Store - Manifest ManifestStore + Baselines syncmanifest.Baselines Input io.Reader Output io.Writer // Initial is true when the workspace has no .launchdarkly directory. @@ -242,7 +236,7 @@ func finishSelection(options Options, files []synclocal.VariationFile) error { for _, file := range files { projectKeys = append(projectKeys, file.ProjectKey) } - manifest, err := options.Manifest.Load(projectKeys) + baseline, err := options.Baselines.Load(projectKeys) if err != nil { return err } @@ -256,9 +250,9 @@ func finishSelection(options Options, files []synclocal.VariationFile) error { if err != nil { return err } - next, err := recordCreatedVariations(manifest, options.Store, files) + next, err := recordCreatedVariations(baseline.Lock, options.Store, files) if err == nil { - _, err = options.Manifest.Update(manifest, next) + _, err = options.Baselines.Save(baseline, next) } if err != nil { return errors.Join(err, options.Store.RollbackCreation(creation)) diff --git a/internal/sync/bootstrap/bootstrap_test.go b/internal/sync/bootstrap/bootstrap_test.go index 357e15ea..0dd06807 100644 --- a/internal/sync/bootstrap/bootstrap_test.go +++ b/internal/sync/bootstrap/bootstrap_test.go @@ -176,17 +176,18 @@ func TestFinishSelectionWritesInitialManifest(t *testing.T) { secondVariation.Name = "Variation 2" err := finishSelection(Options{ - Store: synclocal.NewStore(root), - Manifest: manifestStore, - Output: &output, - Initial: true, + Store: synclocal.NewStore(root), + Baselines: manifestStore, + Output: &output, + Initial: true, }, []synclocal.VariationFile{ {ProjectKey: "project", ConfigKey: "config", Upsert: true, Variation: variation}, {ProjectKey: "project", ConfigKey: "config", Upsert: true, Variation: secondVariation}, }) require.NoError(t, err) - manifest, err := manifestStore.Load([]string{"project"}) + baseline, err := manifestStore.Load([]string{"project"}) + manifest := baseline.Lock require.NoError(t, err) require.Len(t, manifest.Resources, 3) attachmentFingerprint, err := syncdomain.FingerprintAttachment("project", variation.Attachments[0]) @@ -221,7 +222,7 @@ func TestFinishSelectionFingerprintsExistingAttachmentContent(t *testing.T) { Tools: []syncdomain.AttachmentRef{{Key: "search"}}, Attachments: []syncdomain.Attachment{localAttachment}, } require.NoError(t, finishSelection(Options{ - Store: store, Manifest: manifestStore, Output: io.Discard, Initial: true, + Store: store, Baselines: manifestStore, Output: io.Discard, Initial: true, }, []synclocal.VariationFile{{ ProjectKey: "project", ConfigKey: "config", Variation: first, }})) @@ -241,7 +242,7 @@ func TestFinishSelectionFingerprintsExistingAttachmentContent(t *testing.T) { Tools: []syncdomain.AttachmentRef{{Key: "search"}}, Attachments: []syncdomain.Attachment{serverAttachment}, } require.NoError(t, finishSelection(Options{ - Store: store, Manifest: manifestStore, Output: io.Discard, + Store: store, Baselines: manifestStore, Output: io.Discard, }, []synclocal.VariationFile{{ ProjectKey: "project", ConfigKey: "config", Variation: second, }})) @@ -258,7 +259,8 @@ func TestFinishSelectionFingerprintsExistingAttachmentContent(t *testing.T) { } require.NotEmpty(t, expectedFingerprint) - manifest, err := manifestStore.Load([]string{"project"}) + baseline, err := manifestStore.Load([]string{"project"}) + manifest := baseline.Lock require.NoError(t, err) for _, resource := range manifest.Resources { if resource.ResourceKind == syncdomain.KindVariation && resource.LookupKey == "config/second" { @@ -285,19 +287,20 @@ func TestFinishSelectionAddsMultipleVersionedVariationsToExistingManifest(t *tes ModelConfigKey: "model", ModelConfigVersion: 3, } require.NoError(t, finishSelection(Options{ - Store: store, Manifest: manifestStore, Output: io.Discard, Initial: true, + Store: store, Baselines: manifestStore, Output: io.Discard, Initial: true, }, []synclocal.VariationFile{{ ProjectKey: "project", ConfigKey: "config", Variation: first, }})) require.NoError(t, finishSelection(Options{ - Store: store, Manifest: manifestStore, Output: io.Discard, + Store: store, Baselines: manifestStore, Output: io.Discard, }, []synclocal.VariationFile{ {ProjectKey: "project", ConfigKey: "config", Variation: second}, {ProjectKey: "project", ConfigKey: "config", Variation: third}, })) - manifest, err := manifestStore.Load([]string{"project"}) + baseline, err := manifestStore.Load([]string{"project"}) + manifest := baseline.Lock require.NoError(t, err) require.Len(t, manifest.Resources, 3) assert.Equal(t, "config/first", manifest.Resources[0].LookupKey) @@ -317,10 +320,10 @@ func TestFinishSelectionRollsBackFilesWhenManifestWriteFails(t *testing.T) { } err := finishSelection(Options{ - Store: synclocal.NewStore(root), - Manifest: failingManifestStore{}, - Output: io.Discard, - Initial: true, + Store: synclocal.NewStore(root), + Baselines: failingManifestStore{}, + Output: io.Discard, + Initial: true, }, []synclocal.VariationFile{{ ProjectKey: "project", ConfigKey: "config", Variation: variation, }}) @@ -350,7 +353,7 @@ func TestFinishSelectionRollsBackNewAttachmentWithoutRemovingExistingWorkspace(t } err = finishSelection(Options{ - Store: store, Manifest: failingManifestStore{}, Output: io.Discard, + Store: store, Baselines: failingManifestStore{}, Output: io.Discard, }, []synclocal.VariationFile{{ ProjectKey: "project", ConfigKey: "config", Variation: variation, }}) @@ -429,27 +432,24 @@ func (catalog *fakeCatalog) Config(string, string) (syncapi.Config, error) { type failingManifestStore struct{} -func (failingManifestStore) Load([]string) (syncmanifest.Manifest, error) { - return syncmanifest.New(), nil +func (failingManifestStore) Load([]string) (syncmanifest.Baseline, error) { + return syncmanifest.Baseline{Lock: syncmanifest.New()}, nil } -func (failingManifestStore) Update(syncmanifest.Manifest, syncmanifest.Manifest) (syncmanifest.Manifest, error) { - return syncmanifest.Manifest{}, errors.New("write manifest") +func (failingManifestStore) Save(syncmanifest.Baseline, syncmanifest.Manifest) (syncmanifest.Baseline, error) { + return syncmanifest.Baseline{}, errors.New("write manifest") } type memoryManifestStore struct { manifest syncmanifest.Manifest } -func (store *memoryManifestStore) Load([]string) (syncmanifest.Manifest, error) { - return store.manifest, nil +func (store *memoryManifestStore) Load([]string) (syncmanifest.Baseline, error) { + return syncmanifest.Baseline{Lock: store.manifest}, nil } -func (store *memoryManifestStore) Update( - _ syncmanifest.Manifest, - next syncmanifest.Manifest, -) (syncmanifest.Manifest, error) { +func (store *memoryManifestStore) Save(_ syncmanifest.Baseline, next syncmanifest.Manifest) (syncmanifest.Baseline, error) { next.Sort() store.manifest = next - return next, nil + return syncmanifest.Baseline{Lock: next}, nil } diff --git a/internal/sync/detach/detach.go b/internal/sync/detach/detach.go index 59e8c6a5..c4ed1d82 100644 --- a/internal/sync/detach/detach.go +++ b/internal/sync/detach/detach.go @@ -15,17 +15,11 @@ import ( syncmanifest "github.com/launchdarkly/ldcli/internal/sync/manifest" ) -// ManifestStore reads and writes the sync baseline. -type ManifestStore interface { - Load(projectKeys []string) (syncmanifest.Manifest, error) - Update(previous, next syncmanifest.Manifest) (syncmanifest.Manifest, error) -} - // Options are the dependencies and the input of one detach. type Options struct { RepositoryRoot string Store synclocal.Store - Manifest ManifestStore + Baselines syncmanifest.Baselines // ProjectKeys are the projects that have local files. ProjectKeys []string Input io.Reader @@ -44,7 +38,7 @@ func Run(options Options) error { slices.Sort(projectKeys) projectKeys = slices.Compact(projectKeys) - synced, manifest, err := loadResources(options.RepositoryRoot, options.Manifest, projectKeys) + synced, baseline, err := loadResources(options.RepositoryRoot, options.Baselines, projectKeys) if err != nil { return err } @@ -66,7 +60,7 @@ func Run(options Options) error { return err } } - if err := detachResources(options, manifest, selected); err != nil { + if err := detachResources(options, baseline, selected); err != nil { return err } @@ -110,24 +104,24 @@ func validateSelections(synced, selected []syncdomain.ResourceID) error { return nil } -// loadResources returns each variation that the manifest tracks or that has -// a local file, in identity order. +// loadResources returns each variation that the lock tracks or that has a +// local file, in identity order. func loadResources( repositoryRoot string, - manifestStore ManifestStore, + baselines syncmanifest.Baselines, projectKeys []string, -) ([]syncdomain.ResourceID, syncmanifest.Manifest, error) { - manifest, err := manifestStore.Load(projectKeys) +) ([]syncdomain.ResourceID, syncmanifest.Baseline, error) { + baseline, err := baselines.Load(projectKeys) if err != nil { - return nil, syncmanifest.Manifest{}, err + return nil, syncmanifest.Baseline{}, err } files, err := synclocal.SourceFiles(repositoryRoot) if err != nil { - return nil, syncmanifest.Manifest{}, err + return nil, syncmanifest.Baseline{}, err } var synced []syncdomain.ResourceID - for _, resource := range manifest.Resources { + for _, resource := range baseline.Lock.Resources { if resource.ResourceKind == syncdomain.KindVariation { synced = append(synced, resource.ID()) } @@ -138,16 +132,16 @@ func loadResources( } } slices.SortFunc(synced, syncdomain.CompareResourceIDs) - return slices.Compact(synced), manifest, nil + return slices.Compact(synced), baseline, nil } -// detachResources removes the selected variations from the manifest, and then -// deletes their local files. If the delete fails, it restores the manifest. -func detachResources(options Options, original syncmanifest.Manifest, selected []syncdomain.ResourceID) error { +// detachResources removes the selected variations from the baseline, and then +// deletes their local files. If the delete fails, it 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.Resources { + for _, resource := range original.Lock.Resources { if !isSelected(resource.ID()) { next.Resources = append(next.Resources, resource) } @@ -160,12 +154,12 @@ func detachResources(options Options, original syncmanifest.Manifest, selected [ }) next.RemoveUnusedAttachments(remaining) } - persisted, err := options.Manifest.Update(original, next) + saved, err := options.Baselines.Save(original, next) if err != nil { return err } - restoreManifest := func(cause error) error { - _, restoreErr := options.Manifest.Update(persisted, original) + restoreBaseline := func(cause error) error { + _, restoreErr := options.Baselines.Save(saved, original.Lock) return errors.Join(cause, restoreErr) } @@ -177,7 +171,7 @@ func detachResources(options Options, original syncmanifest.Manifest, selected [ } exists, err := options.Store.VariationExists(resource.ProjectKey, configKey, variationKey) if err != nil { - return restoreManifest(err) + return restoreBaseline(err) } if exists { deletions = append(deletions, synclocal.VariationDeletion{ @@ -186,7 +180,7 @@ func detachResources(options Options, original syncmanifest.Manifest, selected [ } } if _, err := options.Store.DeleteVariations(deletions); err != nil { - return restoreManifest(err) + return restoreBaseline(err) } return nil } diff --git a/internal/sync/detach/detach_test.go b/internal/sync/detach/detach_test.go index 2d739bc0..7e6a27cb 100644 --- a/internal/sync/detach/detach_test.go +++ b/internal/sync/detach/detach_test.go @@ -77,13 +77,14 @@ func TestDetachResourcesPrunesUnreferencedAttachmentManifestEntries(t *testing.T resource := syncdomain.ResourceID{Kind: syncdomain.KindVariation, ProjectKey: "project", LookupKey: "config/prompt"} err = detachResources( - Options{RepositoryRoot: root, Store: store, Manifest: manifestStore}, - original, + Options{RepositoryRoot: root, Store: store, Baselines: manifestStore}, + syncmanifest.Baseline{Lock: original}, []syncdomain.ResourceID{resource}, ) require.NoError(t, err) - manifest, err := manifestStore.Load([]string{"project"}) + baseline, err := manifestStore.Load([]string{"project"}) + manifest := baseline.Lock require.NoError(t, err) assert.Empty(t, manifest.Resources) } @@ -112,7 +113,7 @@ 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, Manifest: manifestStore}, original, []syncdomain.ResourceID{resource}) + err = detachResources(Options{Store: store, Baselines: manifestStore}, syncmanifest.Baseline{Lock: original}, []syncdomain.ResourceID{resource}) require.NoError(t, err) exists, err := store.VariationExists("project", "config", "prompt") @@ -120,7 +121,8 @@ func TestDetachResourcesRemovesWrapperAndManifestButKeepsReferencedFile(t *testi assert.False(t, exists) _, err = os.Stat(referencePath) require.NoError(t, err) - manifest, err := manifestStore.Load([]string{"project"}) + baseline, err := manifestStore.Load([]string{"project"}) + manifest := baseline.Lock require.NoError(t, err) assert.Empty(t, manifest.Resources) } @@ -138,10 +140,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, Manifest: manifestStore}, original, []syncdomain.ResourceID{resource}) + err := detachResources(Options{Store: store, Baselines: manifestStore}, syncmanifest.Baseline{Lock: original}, []syncdomain.ResourceID{resource}) require.NoError(t, err) - manifest, err := manifestStore.Load([]string{"project"}) + baseline, err := manifestStore.Load([]string{"project"}) + manifest := baseline.Lock require.NoError(t, err) assert.Empty(t, manifest.Resources) } @@ -157,13 +160,14 @@ func TestDetachResourcesDeletesUnreadableWrapper(t *testing.T) { resource := syncdomain.ResourceID{Kind: syncdomain.KindVariation, ProjectKey: "project", LookupKey: "config/broken"} err := detachResources( - Options{Store: store, Manifest: manifestStore}, - syncmanifest.New(), + Options{Store: store, Baselines: manifestStore}, + syncmanifest.Baseline{Lock: syncmanifest.New()}, []syncdomain.ResourceID{resource}, ) require.NoError(t, err) - manifest, loadErr := manifestStore.Load([]string{"project"}) + baseline, loadErr := manifestStore.Load([]string{"project"}) + manifest := baseline.Lock require.NoError(t, loadErr) assert.Empty(t, manifest.Resources) _, statErr := os.Stat(wrapper) @@ -181,7 +185,7 @@ func TestRunRequiresTerminalWhenResourcesExist(t *testing.T) { err = Run(Options{ RepositoryRoot: root, Store: store, - Manifest: newMemoryManifestStore(), + Baselines: newMemoryManifestStore(), Input: bytes.NewBuffer(nil), Output: bytes.NewBuffer(nil), }) @@ -204,7 +208,7 @@ func TestRunUsesExplicitSelectionsWithoutTerminal(t *testing.T) { err = Run(Options{ RepositoryRoot: root, Store: store, - Manifest: manifestStore, + Baselines: manifestStore, Input: bytes.NewBuffer(nil), Output: bytes.NewBuffer(nil), Selections: []syncdomain.ResourceID{selection}, @@ -225,7 +229,7 @@ func TestRunReportsWhenNoResourcesAreSynced(t *testing.T) { err := Run(Options{ RepositoryRoot: root, Store: synclocal.NewStore(root), - Manifest: newMemoryManifestStore(), + Baselines: newMemoryManifestStore(), Input: bytes.NewBuffer(nil), Output: &output, }) @@ -241,7 +245,7 @@ func TestRunRejectsExplicitSelectionWhenNoResourcesAreSynced(t *testing.T) { err := Run(Options{ RepositoryRoot: root, Store: synclocal.NewStore(root), - Manifest: newMemoryManifestStore(), + Baselines: newMemoryManifestStore(), Input: bytes.NewBuffer(nil), Output: &output, Selections: []syncdomain.ResourceID{syncdomain.VariationID("production", "support", "default")}, @@ -261,17 +265,14 @@ func newMemoryManifestStore() *memoryManifestStore { return &memoryManifestStore{manifest: syncmanifest.New()} } -func (store *memoryManifestStore) Load(projectKeys []string) (syncmanifest.Manifest, error) { +func (store *memoryManifestStore) Load(projectKeys []string) (syncmanifest.Baseline, error) { store.loadedProjectKeys = append([]string(nil), projectKeys...) - return store.manifest, nil + return syncmanifest.Baseline{Lock: store.manifest}, nil } -func (store *memoryManifestStore) Update( - _ syncmanifest.Manifest, - next syncmanifest.Manifest, -) (syncmanifest.Manifest, error) { +func (store *memoryManifestStore) Save(_ syncmanifest.Baseline, next syncmanifest.Manifest) (syncmanifest.Baseline, error) { store.manifest = next - return next, nil + return syncmanifest.Baseline{Lock: next}, nil } func (store *memoryManifestStore) Write(manifest syncmanifest.Manifest) error { diff --git a/internal/sync/local/layout.go b/internal/sync/local/layout.go index 8e26ce4d..f992d7d3 100644 --- a/internal/sync/local/layout.go +++ b/internal/sync/local/layout.go @@ -15,6 +15,9 @@ import ( // .launchdarkly//configs//.prompt.md // .launchdarkly//tools/.json // .launchdarkly//skills/.md +// .launchdarkly/sync.lock +// +// A file directly in .launchdarkly, such as sync.lock, is not a resource. const ( configsDir = "configs" toolsDir = "tools" diff --git a/internal/sync/local/lock.go b/internal/sync/local/lock.go new file mode 100644 index 00000000..22abdc8f --- /dev/null +++ b/internal/sync/local/lock.go @@ -0,0 +1,50 @@ +package local + +import ( + "bytes" + "errors" + "os" + "path/filepath" +) + +// lockFileName is the file that stores the sync baseline of the working copy. +// The manifest package owns its format. +const lockFileName = "sync.lock" + +// ReadLock returns the content of the sync.lock file. The error matches +// fs.ErrNotExist when the file does not exist. +func (store Store) ReadLock() ([]byte, error) { + path := filepath.Join(store.root, lockFileName) + if err := rejectSymlinkedPath(store.root, path); err != nil { + return nil, err + } + return os.ReadFile(path) +} + +// WriteLock replaces the sync.lock file atomically. It does not write a file +// whose content is the same. Empty content removes the file. +func (store Store) WriteLock(content []byte) error { + path := filepath.Join(store.root, lockFileName) + if err := rejectSymlinkedPath(store.root, path); err != nil { + return err + } + if len(content) == 0 { + if err := os.Remove(path); err != nil && !errors.Is(err, os.ErrNotExist) { + return err + } + return nil + } + if current, err := os.ReadFile(path); err == nil && bytes.Equal(current, content) { + return nil + } + + tempPath, err := writeTempFile(path, content, 0o644) + if err != nil { + return err + } + if err := os.Rename(tempPath, path); err != nil { + _ = os.Remove(tempPath) + return err + } + return nil +} diff --git a/internal/sync/local/lock_test.go b/internal/sync/local/lock_test.go new file mode 100644 index 00000000..6e1e9caa --- /dev/null +++ b/internal/sync/local/lock_test.go @@ -0,0 +1,47 @@ +package local + +import ( + "io/fs" + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + syncdomain "github.com/launchdarkly/ldcli/internal/sync" +) + +func TestStoreReadsWritesAndRemovesLock(t *testing.T) { + root := t.TempDir() + require.NoError(t, os.Mkdir(filepath.Join(root, syncdomain.RootDir), 0o755)) + store := NewStore(root) + + _, err := store.ReadLock() + require.ErrorIs(t, err, fs.ErrNotExist) + + require.NoError(t, store.WriteLock([]byte("formatVersion: 1\n"))) + content, err := store.ReadLock() + require.NoError(t, err) + assert.Equal(t, "formatVersion: 1\n", string(content)) + + require.NoError(t, store.WriteLock(nil)) + _, err = store.ReadLock() + require.ErrorIs(t, err, fs.ErrNotExist) + require.NoError(t, store.WriteLock(nil)) +} + +func TestStoreRejectsSymlinkedLock(t *testing.T) { + root := t.TempDir() + require.NoError(t, os.Mkdir(filepath.Join(root, syncdomain.RootDir), 0o755)) + target := filepath.Join(t.TempDir(), "outside.lock") + require.NoError(t, os.WriteFile(target, []byte("outside"), 0o644)) + require.NoError(t, os.Symlink(target, filepath.Join(root, syncdomain.RootDir, lockFileName))) + + err := NewStore(root).WriteLock([]byte("formatVersion: 1\n")) + + require.ErrorContains(t, err, "symbolic links are not supported") + content, err := os.ReadFile(target) + require.NoError(t, err) + assert.Equal(t, "outside", string(content)) +} diff --git a/internal/sync/manifest/baseline.go b/internal/sync/manifest/baseline.go new file mode 100644 index 00000000..c0343833 --- /dev/null +++ b/internal/sync/manifest/baseline.go @@ -0,0 +1,97 @@ +package manifest + +import ( + "slices" + + syncdomain "github.com/launchdarkly/ldcli/internal/sync" +) + +// Baseline is the sync baseline of one working copy. +// +// Lock is the state that this working copy last agreed on with LaunchDarkly. +// Sync compares the local files and LaunchDarkly with Lock to find which side +// changed. The remote manifest is shared by every working copy of the +// repository. It supplies the version of each entry, which LaunchDarkly uses +// to reject two saves of one entry at the same time. +type Baseline struct { + Lock Manifest + remote Manifest + lockFileExists bool +} + +// HasLockFile reports whether the working copy has a sync.lock file. A +// working copy without one uses the remote manifest as its lock. +func (baseline Baseline) HasLockFile() bool { + return baseline.lockFileExists +} + +// Stale reports whether another working copy synced a different state of the +// resource after this working copy wrote its lock. +// +// The fingerprints decide, not the versions. A newer version with the same +// fingerprint means that LaunchDarkly came back to the state of the lock, so +// there is nothing to pull. A missing remote entry means that another working +// copy stopped syncing the resource, which does not change this working copy. +func (baseline Baseline) Stale(id syncdomain.ResourceID) bool { + lockIndex, remoteIndex := baseline.Lock.index(id), baseline.remote.index(id) + if lockIndex < 0 || remoteIndex < 0 { + return false + } + return baseline.Lock.Resources[lockIndex].Fingerprint != baseline.remote.Resources[remoteIndex].Fingerprint +} + +// Baselines loads and saves the baseline of a working copy. BaselineStore +// implements it. +type Baselines interface { + Load(projectKeys []string) (Baseline, error) + Save(current Baseline, next Manifest) (Baseline, error) +} + +// BaselineStore keeps the baseline in the sync.lock file and the entry +// versions in the remote manifest. +type BaselineStore struct { + remote Store + lock LockFile +} + +var _ Baselines = BaselineStore{} + +// NewBaselineStore creates a store for one working copy. +func NewBaselineStore(remote Store, lock LockFile) BaselineStore { + return BaselineStore{remote: remote, lock: lock} +} + +// Load reads the sync.lock file and the remote manifest of each project that +// projectKeys or the lock names. A working copy without a sync.lock file uses +// the remote manifest as its lock. Its next save then writes the file. +func (store BaselineStore) Load(projectKeys []string) (Baseline, error) { + lock, hasLock, err := ReadLock(store.lock) + if err != nil { + return Baseline{}, err + } + remote, err := store.remote.Load(append(slices.Clone(projectKeys), lock.ProjectKeys()...)) + if err != nil { + return Baseline{}, err + } + if !hasLock { + lock = remote.Clone() + } + return Baseline{Lock: lock, remote: remote, lockFileExists: hasLock}, nil +} + +// Save records next as the new baseline. It sends the entries that changed +// from current.Lock to next to the remote manifest, and then writes next to +// the sync.lock file with the versions that LaunchDarkly returned. If the +// remote save fails, the lock file does not change. +func (store BaselineStore) Save(current Baseline, next Manifest) (Baseline, error) { + remote, err := store.remote.Update(current.remote, current.remote.WithChanges(current.Lock, next)) + if err != nil { + return Baseline{}, err + } + saved := Baseline{Lock: next.WithVersionsFrom(remote), remote: remote, lockFileExists: true} + saved.Lock.Sort() + if err := writeLock(store.lock, saved.Lock); err != nil { + return Baseline{}, err + } + return saved, nil +} diff --git a/internal/sync/manifest/baseline_test.go b/internal/sync/manifest/baseline_test.go new file mode 100644 index 00000000..c84b5199 --- /dev/null +++ b/internal/sync/manifest/baseline_test.go @@ -0,0 +1,222 @@ +package manifest + +import ( + "io/fs" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + syncdomain "github.com/launchdarkly/ldcli/internal/sync" + syncapi "github.com/launchdarkly/ldcli/internal/sync/api" +) + +const testSource = "git:example/repo" + +func TestLockFormatIsStable(t *testing.T) { + content, err := encodeLock(Manifest{Resources: []Resource{ + {ResourceKind: syncdomain.KindVariation, ProjectKey: "production", LookupKey: "support/default", Fingerprint: fingerprint("a"), Version: 6}, + {ResourceKind: syncdomain.KindTool, ProjectKey: "production", LookupKey: "search", Fingerprint: fingerprint("b"), Version: 2}, + }}) + + require.NoError(t, err) + assert.Equal(t, `# Written by ldcli sync. Commit this file with the .launchdarkly files. +formatVersion: 1 +resources: + - kind: tool + project: production + key: search + fingerprint: `+fingerprint("b")+` + version: 2 + - kind: variation + project: production + key: support/default + fingerprint: `+fingerprint("a")+` + version: 6 +`, string(content)) + + decoded, err := decodeLock(content) + require.NoError(t, err) + assert.Len(t, decoded.Resources, 2) +} + +func TestDecodeLockRejectsInvalidContent(t *testing.T) { + for name, content := range map[string]string{ + "unknown field": "formatVersion: 1\nresources: []\nextra: true\n", + "unknown version": "formatVersion: 2\nresources: []\n", + "bad fingerprint": "formatVersion: 1\nresources:\n - kind: tool\n project: p\n key: k\n fingerprint: x\n", + } { + _, err := decodeLock([]byte(content)) + assert.Error(t, err, name) + } +} + +func TestBaselineStoreUsesRemoteManifestWithoutLockFile(t *testing.T) { + client := &manifestClient{manifests: map[string]syncapi.SyncManifest{"production": remoteManifest(fingerprint("a"), 3)}} + lock := &memoryLock{} + + baseline, err := NewBaselineStore(NewStore(client, testSource), lock).Load([]string{"production"}) + + require.NoError(t, err) + assert.False(t, baseline.HasLockFile()) + require.Len(t, baseline.Lock.Resources, 1) + assert.Equal(t, fingerprint("a"), baseline.Lock.Resources[0].Fingerprint) + assert.False(t, baseline.Stale(variationID())) +} + +func TestBaselineStoreUsesLockFileAsBaselineAndReportsStaleness(t *testing.T) { + client := &manifestClient{manifests: map[string]syncapi.SyncManifest{"production": remoteManifest(fingerprint("b"), 5)}} + lock := lockWith(t, fingerprint("a"), 4) + + baseline, err := NewBaselineStore(NewStore(client, testSource), lock).Load(nil) + + require.NoError(t, err) + assert.True(t, baseline.HasLockFile()) + assert.Equal(t, fingerprint("a"), baseline.Lock.Resources[0].Fingerprint) + assert.True(t, baseline.Stale(variationID())) +} + +func TestBaselineStaleComparesFingerprints(t *testing.T) { + tests := map[string]struct { + remote syncapi.SyncManifest + stale bool + }{ + "newer version with a different fingerprint": {remote: remoteManifest(fingerprint("b"), 5), stale: true}, + "newer version with the same fingerprint": {remote: remoteManifest(fingerprint("a"), 5)}, + "same version with a different fingerprint": {remote: remoteManifest(fingerprint("b"), 4), stale: true}, + "remote entry removed": {remote: syncapi.SyncManifest{Source: testSource}}, + } + + for name, test := range tests { + t.Run(name, func(t *testing.T) { + client := &manifestClient{manifests: map[string]syncapi.SyncManifest{"production": test.remote}} + baseline, err := NewBaselineStore(NewStore(client, testSource), lockWith(t, fingerprint("a"), 4)).Load(nil) + + require.NoError(t, err) + assert.Equal(t, test.stale, baseline.Stale(variationID())) + }) + } +} + +// The version in each request must be the version that sync read from +// LaunchDarkly, so that LaunchDarkly rejects a save that races another save. +func TestBaselineStoreSaveSendsRemoteVersionsAndWritesLock(t *testing.T) { + client := &manifestClient{ + manifests: map[string]syncapi.SyncManifest{"production": remoteManifest(fingerprint("b"), 5)}, + patchResponses: []syncapi.SyncManifest{{ + Source: testSource, + Items: []syncapi.SyncManifestResource{ + {ResourceKind: syncdomain.KindVariation, ResourceLookupKey: "support/default", Fingerprint: fingerprint("c"), Version: 6}, + {ResourceKind: syncdomain.KindTool, ResourceLookupKey: "search", Fingerprint: fingerprint("d"), Version: 1}, + }, + }}, + } + lock := lockWith(t, fingerprint("a"), 4) + store := NewBaselineStore(NewStore(client, testSource), lock) + baseline, err := store.Load(nil) + require.NoError(t, err) + + next := baseline.Lock.Clone() + next.SetFingerprint(variationID(), fingerprint("c")) + next.SetFingerprint(syncdomain.ResourceID{Kind: syncdomain.KindTool, ProjectKey: "production", LookupKey: "search"}, fingerprint("d")) + saved, err := store.Save(baseline, next) + + require.NoError(t, err) + require.Len(t, client.patches, 1) + assert.Equal(t, []syncapi.SyncManifestUpsert{ + {ResourceKind: syncdomain.KindTool, ResourceLookupKey: "search", Fingerprint: fingerprint("d"), Version: 0}, + {ResourceKind: syncdomain.KindVariation, ResourceLookupKey: "support/default", Fingerprint: fingerprint("c"), Version: 5}, + }, client.patches[0].upserts) + written, _, err := ReadLock(lock) + require.NoError(t, err) + assert.Equal(t, saved.Lock, written) + assert.Equal(t, 6, written.Resources[written.index(variationID())].Version) + assert.False(t, saved.Stale(variationID())) +} + +func TestBaselineStoreSaveKeepsRemoteEntryThatThisCopyDidNotChange(t *testing.T) { + client := &manifestClient{manifests: map[string]syncapi.SyncManifest{"production": remoteManifest(fingerprint("b"), 5)}} + lock := lockWith(t, fingerprint("a"), 4) + store := NewBaselineStore(NewStore(client, testSource), lock) + baseline, err := store.Load(nil) + require.NoError(t, err) + + _, err = store.Save(baseline, baseline.Lock) + + require.NoError(t, err) + assert.Empty(t, client.patches) +} + +func TestBaselineStoreSaveDoesNotWriteLockWhenRemoteSaveFails(t *testing.T) { + client := &manifestClient{ + manifests: map[string]syncapi.SyncManifest{"production": remoteManifest(fingerprint("a"), 4)}, + patchErr: uncertainPatchError(t), + } + lock := lockWith(t, fingerprint("a"), 4) + before := string(lock.content) + store := NewBaselineStore(NewStore(client, testSource), lock) + baseline, err := store.Load(nil) + require.NoError(t, err) + + next := baseline.Lock.Clone() + next.SetFingerprint(variationID(), fingerprint("c")) + _, err = store.Save(baseline, next) + + require.Error(t, err) + assert.Equal(t, before, string(lock.content)) +} + +func TestBaselineStoreSaveRemovesEmptyLock(t *testing.T) { + client := &manifestClient{ + manifests: map[string]syncapi.SyncManifest{"production": remoteManifest(fingerprint("a"), 4)}, + patchResponses: []syncapi.SyncManifest{{Source: testSource}}, + } + lock := lockWith(t, fingerprint("a"), 4) + store := NewBaselineStore(NewStore(client, testSource), lock) + baseline, err := store.Load(nil) + require.NoError(t, err) + + _, err = store.Save(baseline, New()) + + require.NoError(t, err) + assert.Nil(t, lock.content) + assert.Equal(t, []syncapi.SyncManifestDeletion{{ + ResourceKind: syncdomain.KindVariation, ResourceLookupKey: "support/default", Version: 4, + }}, client.patches[0].deletions) +} + +func variationID() syncdomain.ResourceID { + return syncdomain.VariationID("production", "support", "default") +} + +func remoteManifest(fingerprint string, version int) syncapi.SyncManifest { + return syncapi.SyncManifest{Source: testSource, Items: []syncapi.SyncManifestResource{{ + ResourceKind: syncdomain.KindVariation, ResourceLookupKey: "support/default", Fingerprint: fingerprint, Version: version, + }}} +} + +func lockWith(t *testing.T, fingerprint string, version int) *memoryLock { + t.Helper() + content, err := encodeLock(Manifest{Resources: []Resource{{ + ResourceKind: syncdomain.KindVariation, ProjectKey: "production", LookupKey: "support/default", + Fingerprint: fingerprint, Version: version, + }}}) + require.NoError(t, err) + return &memoryLock{content: content} +} + +type memoryLock struct { + content []byte +} + +func (lock *memoryLock) ReadLock() ([]byte, error) { + if lock.content == nil { + return nil, fs.ErrNotExist + } + return lock.content, nil +} + +func (lock *memoryLock) WriteLock(content []byte) error { + lock.content = content + return nil +} diff --git a/internal/sync/manifest/lock.go b/internal/sync/manifest/lock.go new file mode 100644 index 00000000..800671c1 --- /dev/null +++ b/internal/sync/manifest/lock.go @@ -0,0 +1,105 @@ +package manifest + +import ( + "bytes" + "errors" + "fmt" + "io/fs" + + "gopkg.in/yaml.v3" +) + +// The sync.lock file stores the baseline of one working copy. Engineers commit +// it with the .launchdarkly files, so that the baseline moves with the files. +const ( + lockFormatVersion = 1 + lockHeader = "# Written by ldcli sync. Commit this file with the .launchdarkly files.\n" +) + +// LockFile reads and writes the sync.lock file of a working copy. ReadLock +// returns an error that matches fs.ErrNotExist when the file does not exist. +// WriteLock with empty content removes the file. +type LockFile interface { + ReadLock() ([]byte, error) + WriteLock(content []byte) error +} + +type lockDocument struct { + FormatVersion int `yaml:"formatVersion"` + Resources []Resource `yaml:"resources"` +} + +// ReadLock decodes the sync.lock file. The bool result is false when the file +// does not exist. +func ReadLock(lock LockFile) (Manifest, bool, error) { + content, err := lock.ReadLock() + if errors.Is(err, fs.ErrNotExist) { + return New(), false, nil + } + if err != nil { + return Manifest{}, false, fmt.Errorf("read sync.lock: %w", err) + } + manifest, err := decodeLock(content) + if err != nil { + return Manifest{}, false, fmt.Errorf("read sync.lock: %w", err) + } + return manifest, true, nil +} + +// writeLock encodes the manifest to the sync.lock file. A manifest without +// entries removes the file, so that a workspace without resources has no +// .launchdarkly directory. +func writeLock(lock LockFile, manifest Manifest) error { + content, err := encodeLock(manifest) + if err != nil { + return err + } + if err := lock.WriteLock(content); err != nil { + return fmt.Errorf("write sync.lock: %w", err) + } + return nil +} + +// encodeLock returns the lock content, with the entries in identity order so +// that a Git diff shows only the entries that changed. +func encodeLock(manifest Manifest) ([]byte, error) { + if len(manifest.Resources) == 0 { + return nil, nil + } + manifest = manifest.Clone() + manifest.Sort() + + var content bytes.Buffer + content.WriteString(lockHeader) + encoder := yaml.NewEncoder(&content) + encoder.SetIndent(2) + if err := encoder.Encode(lockDocument{FormatVersion: lockFormatVersion, Resources: manifest.Resources}); err != nil { + return nil, fmt.Errorf("encode sync.lock: %w", err) + } + if err := encoder.Close(); err != nil { + return nil, fmt.Errorf("encode sync.lock: %w", err) + } + return content.Bytes(), nil +} + +func decodeLock(content []byte) (Manifest, error) { + var document lockDocument + decoder := yaml.NewDecoder(bytes.NewReader(content)) + decoder.KnownFields(true) + if err := decoder.Decode(&document); err != nil { + return Manifest{}, fmt.Errorf("decode: %w", err) + } + if document.FormatVersion != lockFormatVersion { + return Manifest{}, fmt.Errorf("unsupported formatVersion %d", document.FormatVersion) + } + + manifest := Manifest{Resources: document.Resources} + if manifest.Resources == nil { + manifest = New() + } + if err := manifest.Validate(); err != nil { + return Manifest{}, err + } + manifest.Sort() + return manifest, nil +} diff --git a/internal/sync/manifest/model.go b/internal/sync/manifest/model.go index 95cddf4e..92dd0ef6 100644 --- a/internal/sync/manifest/model.go +++ b/internal/sync/manifest/model.go @@ -20,13 +20,14 @@ type Manifest struct { } // Resource is the baseline of one resource. Version is the version of the -// remote manifest entry, which LaunchDarkly uses for optimistic locking. +// remote manifest entry, which LaunchDarkly uses for optimistic locking. The +// YAML tags are the format of the sync.lock file. type Resource struct { - ResourceKind syncdomain.Kind - ProjectKey string - LookupKey string - Fingerprint string - Version int + ResourceKind syncdomain.Kind `yaml:"kind"` + ProjectKey string `yaml:"project"` + LookupKey string `yaml:"key"` + Fingerprint string `yaml:"fingerprint"` + Version int `yaml:"version"` } // ID returns the identity of the resource. @@ -110,6 +111,47 @@ func (manifest *Manifest) RemoveUnusedAttachments(variations []syncdomain.Synced }) } +// 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. +func (manifest Manifest) WithChanges(before, after Manifest) Manifest { + result := manifest.Clone() + for _, resource := range after.Resources { + if index := before.index(resource.ID()); index < 0 || before.Resources[index].Fingerprint != resource.Fingerprint { + result.SetFingerprint(resource.ID(), resource.Fingerprint) + } + } + for _, resource := range before.Resources { + if after.index(resource.ID()) < 0 { + result.Remove(resource.ID()) + } + } + return result +} + +// WithVersionsFrom returns a copy of the manifest in which each entry has the +// version of the same entry in source, or 0 if source does not have it. +func (manifest Manifest) WithVersionsFrom(source Manifest) Manifest { + versions := make(map[syncdomain.ResourceID]int, len(source.Resources)) + for _, resource := range source.Resources { + versions[resource.ID()] = resource.Version + } + result := manifest.Clone() + for index := range result.Resources { + result.Resources[index].Version = versions[result.Resources[index].ID()] + } + return result +} + +// ProjectKeys returns each project that has an entry, in order. +func (manifest Manifest) ProjectKeys() []string { + keys := make([]string, 0, len(manifest.Resources)) + for _, resource := range manifest.Resources { + keys = append(keys, resource.ProjectKey) + } + return uniqueSorted(keys) +} + // Validate makes sure that each entry has a safe identity, a valid // fingerprint, and a unique identity. func (manifest Manifest) Validate() error { diff --git a/internal/sync/manifest/store.go b/internal/sync/manifest/store.go index 23fccf08..66c8d9f6 100644 --- a/internal/sync/manifest/store.go +++ b/internal/sync/manifest/store.go @@ -77,7 +77,8 @@ func (store Store) Update(previous, next Manifest) (Manifest, error) { after := next.project(projectKey) upserts, deletions := changes(before, after) if len(upserts) == 0 && len(deletions) == 0 { - result.Resources = append(result.Resources, withVersions(after, before)...) + unchanged := Manifest{Resources: after}.WithVersionsFrom(Manifest{Resources: before}) + result.Resources = append(result.Resources, unchanged.Resources...) continue } @@ -220,20 +221,6 @@ func changesApplied( return true } -// withVersions copies the remote version of each entry in before to the -// matching entry in after. -func withVersions(after, before []Resource) []Resource { - versions := make(map[syncdomain.ResourceID]int, len(before)) - for _, resource := range before { - versions[resource.ID()] = resource.Version - } - result := slices.Clone(after) - for index := range result { - result[index].Version = versions[result[index].ID()] - } - return result -} - // project returns the entries of one project. func (manifest Manifest) project(projectKey string) []Resource { var resources []Resource @@ -251,14 +238,8 @@ func sorted(resources []Resource) []Resource { return manifest.Resources } -func projectKeys(manifests ...Manifest) []string { - var keys []string - for _, manifest := range manifests { - for _, resource := range manifest.Resources { - keys = append(keys, resource.ProjectKey) - } - } - return uniqueSorted(keys) +func projectKeys(previous, next Manifest) []string { + return uniqueSorted(append(previous.ProjectKeys(), next.ProjectKeys()...)) } func uniqueSorted(values []string) []string { diff --git a/internal/sync/prompt/acceptance_test.go b/internal/sync/prompt/acceptance_test.go index 6a3f923f..81bd5c89 100644 --- a/internal/sync/prompt/acceptance_test.go +++ b/internal/sync/prompt/acceptance_test.go @@ -758,20 +758,52 @@ func TestPromptServerDeletionLeavesReferencedFile(t *testing.T) { assert.Empty(t, resources) } +// A working copy that is behind another working copy must pull the newer +// LaunchDarkly state. It must not write its older file back to LaunchDarkly. +func TestPromptStaleWorkingCopyPullsInsteadOfReverting(t *testing.T) { + root := initRepository(t) + baseline := variation("Baseline") + writeVariation(t, root, baseline, false) + writeManifest(t, root, baseline) + api := &directAPI{variation: pointer(baseline)} + _, _, err := runPrompt(t, root, api, "--yes") + require.NoError(t, err) + + // Another working copy syncs a change. The shared remote manifest moves. + other := variation("Other working copy") + api.variation = pointer(other) + writeManifest(t, root, other) + manifestsByRoot[root].Items[0].Version = 2 + + _, review, err := runPrompt(t, root, api, "--yes") + + require.NoError(t, err) + assert.Equal(t, other.Name, api.variation.Name) + 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.") + resources, err := synclocal.CompileWorkspace(root) + require.NoError(t, err) + require.Len(t, resources, 1) + assert.Equal(t, other.Name, resources[0].Variation.Name) +} + func TestPromptPropagatesTrackedLocalDeletion(t *testing.T) { root := initRepository(t) baseline := variation("Baseline") writeVariation(t, root, baseline, false) writeManifest(t, root, baseline) - command := exec.Command("git", "add", ".launchdarkly") - command.Dir = root - output, err := command.CombinedOutput() - require.NoError(t, err, "%s", output) + api := &directAPI{variation: pointer(baseline)} + + // The first sync writes sync.lock, which tracks the project after its + // files are gone. + _, _, err := runPrompt(t, root, api, "--yes") + require.NoError(t, err) _, err = synclocal.NewStore(root).DeleteVariations([]synclocal.VariationDeletion{{ ProjectKey: "production", ConfigKey: "support", VariationKey: "default", }}) require.NoError(t, err) - api := &directAPI{variation: pointer(baseline)} _, _, err = runPrompt(t, root, api, "--yes") diff --git a/internal/sync/prompt/conflict_test.go b/internal/sync/prompt/conflict_test.go index f87b2f76..26089b55 100644 --- a/internal/sync/prompt/conflict_test.go +++ b/internal/sync/prompt/conflict_test.go @@ -267,7 +267,7 @@ func TestRunWorkspaceSyncAppliesConflictChoiceAfterRevalidation(t *testing.T) { AccessToken: "token", BaseURI: "https://example.test", Yes: true, Input: input, Output: &output, ErrorOutput: &output, }, - syncWorkspace{root: root, local: localStore, manifest: manifestStore}, + syncWorkspace{root: root, local: localStore, baselines: manifestStore}, nil, ) @@ -292,7 +292,7 @@ func TestRunWorkspaceSyncAbortsConflictWithoutWriting(t *testing.T) { AccessToken: "token", BaseURI: "https://example.test", Yes: true, Input: input, Output: &output, ErrorOutput: &output, }, - syncWorkspace{root: root, local: localStore, manifest: manifestStore}, + syncWorkspace{root: root, local: localStore, baselines: manifestStore}, nil, ) @@ -318,7 +318,7 @@ func TestRunWorkspaceSyncAbortsAttachmentConflictWithoutWriting(t *testing.T) { AccessToken: "token", BaseURI: "https://example.test", Yes: true, Input: input, Output: &output, ErrorOutput: &output, }, - syncWorkspace{root: root, local: localStore, manifest: manifestStore}, + syncWorkspace{root: root, local: localStore, baselines: manifestStore}, nil, ) @@ -349,7 +349,7 @@ func TestRunWorkspaceSyncUsesLaunchDarklyForAttachmentConflict(t *testing.T) { AccessToken: "token", BaseURI: "https://example.test", Yes: true, Input: input, Output: &output, ErrorOutput: &output, }, - syncWorkspace{root: root, local: localStore, manifest: manifestStore}, + syncWorkspace{root: root, local: localStore, baselines: manifestStore}, nil, ) @@ -416,7 +416,7 @@ func divergentPlan(t *testing.T) Plan { }) } -func writeConflictWorkspace(t *testing.T, root string, baseline, local syncdomain.Variation) (synclocal.Store, manifestStore) { +func writeConflictWorkspace(t *testing.T, root string, baseline, local syncdomain.Variation) (synclocal.Store, syncmanifest.Baselines) { t.Helper() command := exec.Command("git", "init", "--quiet") @@ -445,16 +445,13 @@ type memoryManifestStore struct { manifest syncmanifest.Manifest } -func (store *memoryManifestStore) Load([]string) (syncmanifest.Manifest, error) { - return store.manifest, nil +func (store *memoryManifestStore) Load([]string) (syncmanifest.Baseline, error) { + return syncmanifest.Baseline{Lock: store.manifest}, nil } -func (store *memoryManifestStore) Update( - _ syncmanifest.Manifest, - next syncmanifest.Manifest, -) (syncmanifest.Manifest, error) { +func (store *memoryManifestStore) Save(_ syncmanifest.Baseline, next syncmanifest.Manifest) (syncmanifest.Baseline, error) { store.manifest = next - return next, nil + return syncmanifest.Baseline{Lock: next}, nil } type conflictAPI struct { diff --git a/internal/sync/prompt/output.go b/internal/sync/prompt/output.go index 7b1fff26..8f22c2d2 100644 --- a/internal/sync/prompt/output.go +++ b/internal/sync/prompt/output.go @@ -57,17 +57,21 @@ func writePlanOutput(out io.Writer, outputKind string, plan Plan) error { type planResourceOutput struct { resourceOutput - Action Action `json:"action"` - Error string `json:"error,omitempty"` - Diff variationDiffFields `json:"diff,omitempty"` + Action Action `json:"action"` + SyncedElsewhere bool `json:"syncedElsewhere,omitempty"` + Suggestion string `json:"suggestion,omitempty"` + Error string `json:"error,omitempty"` + Diff variationDiffFields `json:"diff,omitempty"` } resources := make([]planResourceOutput, 0, len(plan.Resources)) for _, resource := range plan.Resources { resources = append(resources, planResourceOutput{ - resourceOutput: newResourceOutput(resource.ID), - Action: resource.Action, - Error: resource.Error, - Diff: resource.Diff, + resourceOutput: newResourceOutput(resource.ID), + Action: resource.Action, + SyncedElsewhere: resource.SyncedElsewhere, + Suggestion: staleSuggestion(resource), + Error: resource.Error, + Diff: resource.Diff, }) } return writeJSON(out, map[string]any{"resources": resources}) @@ -127,6 +131,13 @@ func writePlanReview(out io.Writer, outputKind string, plan Plan, width int) err 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) + } + } currentProject, currentConfig := "", "" for _, resource := range plan.Resources { @@ -157,12 +168,12 @@ func writePlanReview(out io.Writer, outputKind string, plan Plan, width int) err _ = 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)) + } if resource.Error != "" { - if markdown { - _ = console.Printf("Error: %s\n", resource.Error) - } else { - _ = console.Printf(" Error: %s\n", resource.Error) - } + detail("Error", resource.Error) } if len(resource.Diff) != 0 { rendered, err := renderVariationDiff(resource.Diff, outputKind, width, diffPresentation(resource.Action)) @@ -175,6 +186,23 @@ func writePlanReview(out io.Writer, outputKind string, plan Plan, width int) err return nil } +// staleSuggestion tells the user what to do when another working copy synced +// the resource. It returns an empty string for a resource that is not stale. +func staleSuggestion(resource PlannedResource) string { + switch { + case !resource.SyncedElsewhere: + return "" + case resource.Action == ActionUpdateLocal: + return "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." + case resource.Action == ActionConflict: + return "Run git pull to get the other change, and then run sync again. " + + "If the conflict remains, choose a side with --conflict or --resolve." + default: + return "Run git pull to get the latest .launchdarkly files and sync.lock, and then run sync again." + } +} + // reviewHeading colors a heading when the output is a terminal. func reviewHeading(value string, width int) string { if width <= 0 { diff --git a/internal/sync/prompt/output_test.go b/internal/sync/prompt/output_test.go index ff170880..d09b661a 100644 --- a/internal/sync/prompt/output_test.go +++ b/internal/sync/prompt/output_test.go @@ -57,6 +57,26 @@ func TestWritePlanDescribesArchivedLaunchDarklyVariation(t *testing.T) { assert.Contains(t, output.String(), "(archived in LaunchDarkly)") } +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)) + + 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.Contains(t, data.String(), `"syncedElsewhere": true`) + assert.Contains(t, data.String(), `"suggestion": "If the other working copy pushed its change to Git`) +} + +func TestStaleSuggestionMatchesTheAction(t *testing.T) { + assert.Empty(t, staleSuggestion(PlannedResource{Action: ActionUpdateLocal})) + assert.Contains(t, staleSuggestion(PlannedResource{Action: ActionUpdateLocal, SyncedElsewhere: true}), "run git pull before you sync") + assert.Contains(t, staleSuggestion(PlannedResource{Action: ActionConflict, SyncedElsewhere: true}), "--conflict or --resolve") + assert.Contains(t, staleSuggestion(PlannedResource{Action: ActionInSync, SyncedElsewhere: true}), "run sync again") +} + func TestWritePlanRendersHumanFriendlyAttachmentDiff(t *testing.T) { server := testVariation("Support") local := server diff --git a/internal/sync/prompt/plan.go b/internal/sync/prompt/plan.go index 79c47f83..f58cfef1 100644 --- a/internal/sync/prompt/plan.go +++ b/internal/sync/prompt/plan.go @@ -57,6 +57,10 @@ type PlannedResource struct { ServerMode syncdomain.VariationMode Local *syncdomain.Variation Server *syncdomain.Variation + // SyncedElsewhere is true when another working copy synced the resource + // after this working copy wrote its sync.lock file. The plan is still + // correct, because it compares against this working copy's lock. + SyncedElsewhere bool // ServerHasStaleAttachmentPins is true when LaunchDarkly pins an older // version of a tool or skill than its latest version. ServerHasStaleAttachmentPins bool diff --git a/internal/sync/prompt/runner.go b/internal/sync/prompt/runner.go index 8b770ce1..f372147b 100644 --- a/internal/sync/prompt/runner.go +++ b/internal/sync/prompt/runner.go @@ -24,16 +24,12 @@ import ( syncsource "github.com/launchdarkly/ldcli/internal/sync/source" ) -type manifestStore interface { - Load(projectKeys []string) (syncmanifest.Manifest, error) - Update(previous, next syncmanifest.Manifest) (syncmanifest.Manifest, error) -} - // syncWorkspace is the Git repository that one command syncs. type syncWorkspace struct { - root string - local synclocal.Store - manifest manifestStore + root string + local synclocal.Store + // baselines keeps the sync.lock file and the remote manifest. + baselines syncmanifest.Baselines } // Runner runs the prompt sync commands. Tests replace its function fields. @@ -71,10 +67,11 @@ func (runner Runner) Run(options Options) error { if err != nil { return err } + local := synclocal.NewStore(resolved.Root) workspace := syncWorkspace{ - root: resolved.Root, - local: synclocal.NewStore(resolved.Root), - manifest: syncmanifest.NewStore(runner.api(options), resolved.Source), + root: resolved.Root, + local: local, + baselines: syncmanifest.NewBaselineStore(syncmanifest.NewStore(runner.api(options), resolved.Source), local), } switch action := options.Action.(type) { @@ -109,7 +106,7 @@ func (runner Runner) runSync(options Options, workspace syncWorkspace, action Sy if err != nil { return err } - projectKeys, err := discoverProjectKeys(workspace.root) + projectKeys, err := workspace.projectKeys() if err != nil { return err } @@ -145,14 +142,14 @@ func (runner Runner) runAdd(options Options, workspace syncWorkspace, action Add } func (runner Runner) runDetach(options Options, workspace syncWorkspace, action DetachAction) error { - projectKeys, err := discoverProjectKeys(workspace.root) + projectKeys, err := workspace.projectKeys() if err != nil { return err } return runner.detach(syncdetach.Options{ RepositoryRoot: workspace.root, Store: workspace.local, - Manifest: workspace.manifest, + Baselines: workspace.baselines, ProjectKeys: projectKeys, Input: options.Input, Output: options.Output, @@ -224,7 +221,7 @@ func (runner Runner) bootstrapOptions( Catalog: api, Attachments: api, Store: workspace.local, - Manifest: workspace.manifest, + Baselines: workspace.baselines, Input: options.Input, Output: options.Output, Initial: initial, diff --git a/internal/sync/prompt/runner_test.go b/internal/sync/prompt/runner_test.go index 55aef41a..ec0a45da 100644 --- a/internal/sync/prompt/runner_test.go +++ b/internal/sync/prompt/runner_test.go @@ -164,7 +164,7 @@ func TestRunnerDetachesWithoutCallingTheAPI(t *testing.T) { runner.detach = func(options syncdetach.Options) error { called = true assert.NotZero(t, options.Store) - assert.NotZero(t, options.Manifest) + assert.NotZero(t, options.Baselines) assert.Equal(t, os.Stdin, options.Input) assert.Equal(t, io.Discard, options.Output) return nil diff --git a/internal/sync/prompt/state.go b/internal/sync/prompt/state.go index 7c7a0c98..083d0981 100644 --- a/internal/sync/prompt/state.go +++ b/internal/sync/prompt/state.go @@ -8,7 +8,6 @@ import ( syncapi "github.com/launchdarkly/ldcli/internal/sync/api" synclocal "github.com/launchdarkly/ldcli/internal/sync/local" syncmanifest "github.com/launchdarkly/ldcli/internal/sync/manifest" - syncrepository "github.com/launchdarkly/ldcli/internal/sync/repository" ) // workspaceState is everything that one plan depends on. The plan compares @@ -16,37 +15,37 @@ import ( // stores it, so that a local write keeps the form of the file. type workspaceState struct { projectKeys []string - manifest syncmanifest.Manifest + baseline syncmanifest.Baseline plan Plan localFiles map[ResourceID]syncdomain.SyncedResource } -// loadState reads the manifest, the local files, and LaunchDarkly, and -// builds the plan. +// loadState reads the baseline, the local files, and LaunchDarkly, and builds +// the plan. func (workspace syncWorkspace) loadState(client syncapi.Client) (workspaceState, error) { - projectKeys, err := discoverProjectKeys(workspace.root) + projectKeys, err := workspace.projectKeys() if err != nil { return workspaceState{}, err } - // The manifest is the common ancestor in a three-way comparison of the - // local files and LaunchDarkly. - manifest, err := workspace.manifest.Load(projectKeys) + // The lock is the common ancestor in a three-way comparison of the local + // files and LaunchDarkly. + baseline, err := workspace.baselines.Load(projectKeys) if err != nil { return workspaceState{}, err } - plan, localFiles, err := loadWorkspacePlan(workspace.root, manifest, client) + plan, localFiles, err := loadWorkspacePlan(workspace.root, baseline, client) if err != nil { return workspaceState{}, err } - return workspaceState{projectKeys: projectKeys, manifest: manifest, plan: plan, localFiles: localFiles}, nil + return workspaceState{projectKeys: projectKeys, baseline: baseline, plan: plan, localFiles: localFiles}, nil } // loadWorkspacePlan reads the local variations and their LaunchDarkly -// versions, and compares both with the baseline. It also returns the local +// versions, and compares both with the lock. It also returns the local // variations as their files store them. func loadWorkspacePlan( repositoryRoot string, - baseline syncmanifest.Manifest, + baseline syncmanifest.Baseline, client syncapi.Client, ) (Plan, map[ResourceID]syncdomain.SyncedResource, error) { localFiles, err := compileWorkspace(repositoryRoot) @@ -59,12 +58,12 @@ func loadWorkspacePlan( } localFilesByID := make(map[ResourceID]syncdomain.SyncedResource, len(localFiles)) - ids := make(map[ResourceID]struct{}, len(localFiles)+len(baseline.Resources)) + ids := make(map[ResourceID]struct{}, len(localFiles)+len(baseline.Lock.Resources)) for _, resource := range localFiles { localFilesByID[resource.ID()] = resource ids[resource.ID()] = struct{}{} } - for _, resource := range baseline.Resources { + for _, resource := range baseline.Lock.Resources { if resource.ResourceKind == syncdomain.KindVariation { ids[resource.ID()] = struct{}{} } @@ -79,7 +78,11 @@ func loadWorkspacePlan( } server[id] = resource } - return BuildPlan(baseline, local, server), localFilesByID, nil + plan := BuildPlan(baseline.Lock, local, server) + for index := range plan.Resources { + plan.Resources[index].SyncedElsewhere = baseline.Stale(plan.Resources[index].ID) + } + return plan, localFilesByID, nil } // readServerResource reads one variation from LaunchDarkly with the content @@ -104,20 +107,21 @@ func readServerResource(client syncapi.Client, attachments *attachmentCache, id return resource, nil } -// discoverProjectKeys returns each project that has a local sync file, or -// that had one before a deletion that Git reports. -func discoverProjectKeys(repositoryRoot string) ([]string, error) { - files, err := synclocal.SourceFiles(repositoryRoot) +// projectKeys returns each project that has a local sync file or an entry in +// the sync.lock file. A project whose files are all missing is still in the +// lock, so sync can restore its files. +func (workspace syncWorkspace) projectKeys() ([]string, error) { + files, err := synclocal.SourceFiles(workspace.root) if err != nil { return nil, err } - deleted, err := syncrepository.DeletedPaths(repositoryRoot) + lock, _, err := syncmanifest.ReadLock(workspace.local) if err != nil { return nil, err } - var projectKeys []string - for _, file := range append(files, deleted...) { + projectKeys := lock.ProjectKeys() + for _, file := range files { if id, ok := synclocal.ParseManagedPath(file); ok { projectKeys = append(projectKeys, id.ProjectKey) } diff --git a/internal/sync/prompt/sync.go b/internal/sync/prompt/sync.go index f70b2733..77e35535 100644 --- a/internal/sync/prompt/sync.go +++ b/internal/sync/prompt/sync.go @@ -10,7 +10,7 @@ import ( // runWorkspaceSync runs one sync in five steps: // -// 1. Read the manifest, the local files, and LaunchDarkly, and build a plan. +// 1. Read the baseline, the local files, and LaunchDarkly, and build a plan. // 2. Ask the user to resolve each conflict. // 3. Show the plan and ask the user to apply it. // 4. Read the state again, and stop if it changed after the review. @@ -60,6 +60,12 @@ func (runner Runner) runWorkspaceSync(options Options, workspace syncWorkspace, } if !proceed { if !resolved.HasChanges() { + // A workspace from before sync.lock gets the file on its first sync. + if !reviewed.baseline.HasLockFile() { + if _, err := workspace.baselines.Save(reviewed.baseline, reviewed.baseline.Lock); err != nil { + return err + } + } return cleanupOrphanedAttachments(options, workspace.local, interactive) } return nil @@ -102,10 +108,10 @@ func (runner Runner) applyPlan( interactive bool, ) error { plan := applyConflictResolutions(current.plan, resolutions) - outcomes, next, err := executePlan(workspace.root, workspace.local, client, current.manifest, plan, current.localFiles) + outcomes, next, err := executePlan(workspace.root, workspace.local, client, current.baseline.Lock, plan, current.localFiles) failures := []error{err} - if _, err := workspace.manifest.Update(current.manifest, next); err != nil { + if _, err := workspace.baselines.Save(current.baseline, next); err != nil { failures = append(failures, err) } if err := workspace.local.RemoveEmptyDirectories(); err != nil { diff --git a/internal/sync/prompt/watch_test.go b/internal/sync/prompt/watch_test.go index 7b4ea549..4b60bdba 100644 --- a/internal/sync/prompt/watch_test.go +++ b/internal/sync/prompt/watch_test.go @@ -457,3 +457,16 @@ func TestSourceWatcherRecognizesNewManagedResourceKinds(t *testing.T) { Op: fsnotify.Create, })) } + +func TestSourceWatcherIgnoresSyncLock(t *testing.T) { + root := t.TempDir() + managedRoot := filepath.Join(root, syncdomain.RootDir) + lockFile := filepath.Join(managedRoot, "sync.lock") + require.NoError(t, os.MkdirAll(managedRoot, 0o755)) + require.NoError(t, os.WriteFile(lockFile, []byte("formatVersion: 1\n"), 0o644)) + watcher := sourceWatcher{managedRoot: managedRoot, files: make(map[string]struct{})} + + for _, op := range []fsnotify.Op{fsnotify.Create, fsnotify.Write, fsnotify.Rename} { + require.False(t, watcher.relevant(fsnotify.Event{Name: lockFile, Op: op}), op.String()) + } +} diff --git a/internal/sync/repository/git.go b/internal/sync/repository/git.go index 076a837a..4cdd5ea3 100644 --- a/internal/sync/repository/git.go +++ b/internal/sync/repository/git.go @@ -5,7 +5,6 @@ import ( "fmt" "os" "os/exec" - "slices" "strings" ) @@ -55,11 +54,6 @@ func FindGitRepository(dir string) (GitRepository, bool, error) { return findGitRepository(execGit{}, dir) } -// DeletedPaths returns staged and unstaged deleted paths relative to the repository. -func DeletedPaths(repositoryRoot string) ([]string, error) { - return deletedPaths(execGit{}, repositoryRoot) -} - // findGitRepository contains the injectable repository-discovery workflow used // by the real command and focused tests. func findGitRepository(git gitRunner, dir string) (GitRepository, bool, error) { @@ -87,33 +81,3 @@ func findGitRepository(git gitRunner, dir string) (GitRepository, bool, error) { } return GitRepository{Root: root, Origin: origin}, true, nil } - -func deletedPaths(git gitRunner, repositoryRoot string) ([]string, error) { - var paths []string - commands := [][]string{ - {"diff", "--name-only", "--diff-filter=D", "-z", "--", ".launchdarkly"}, - {"diff", "--cached", "--name-only", "--diff-filter=D", "-z", "--", ".launchdarkly"}, - } - for _, command := range commands { - output, stderr, err := git.output(repositoryRoot, command...) - if err != nil { - if stderr != "" { - return nil, fmt.Errorf("find deleted sync files: %s: %w", stderr, err) - } - return nil, fmt.Errorf("find deleted sync files: %w", err) - } - paths = append(paths, splitNullTerminated(output)...) - } - slices.Sort(paths) - return slices.Compact(paths), nil -} - -func splitNullTerminated(value string) []string { - var values []string - for _, item := range strings.Split(value, "\x00") { - if item != "" { - values = append(values, item) - } - } - return values -} diff --git a/internal/sync/repository/git_test.go b/internal/sync/repository/git_test.go index f3b829d9..3c57b317 100644 --- a/internal/sync/repository/git_test.go +++ b/internal/sync/repository/git_test.go @@ -99,25 +99,6 @@ func TestFindGitRepositoryReturnsOperationalError(t *testing.T) { }) } -func TestDeletedPathsCombinesStagedAndUnstagedChanges(t *testing.T) { - git := &fakeGit{outputs: map[string]gitResult{ - "diff --name-only --diff-filter=D -z -- .launchdarkly": { - output: ".launchdarkly/project/configs/config/unstaged.prompt.md\x00", - }, - "diff --cached --name-only --diff-filter=D -z -- .launchdarkly": { - output: ".launchdarkly/project/configs/config/staged.prompt.md\x00", - }, - }} - - paths, err := deletedPaths(git, "/tmp/example") - - require.NoError(t, err) - require.Equal(t, []string{ - ".launchdarkly/project/configs/config/staged.prompt.md", - ".launchdarkly/project/configs/config/unstaged.prompt.md", - }, paths) -} - type gitResult struct { output string stderr string