From fa26e6efb2b88165702dd52985eb480ad5f371dc Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Thu, 18 Apr 2024 14:37:55 +0200 Subject: [PATCH 1/5] Cleanup remote file path on bundle destroy --- bundle/deploy/files/delete.go | 32 +++++++++++++--- internal/bundle/destroy_test.go | 67 +++++++++++++++++++++++++++++++++ 2 files changed, 93 insertions(+), 6 deletions(-) create mode 100644 internal/bundle/destroy_test.go diff --git a/bundle/deploy/files/delete.go b/bundle/deploy/files/delete.go index 9367e2a624..7a9e29da20 100644 --- a/bundle/deploy/files/delete.go +++ b/bundle/deploy/files/delete.go @@ -3,10 +3,12 @@ package files import ( "context" "fmt" + "os" "github.com/databricks/cli/bundle" "github.com/databricks/cli/libs/cmdio" "github.com/databricks/cli/libs/diag" + "github.com/databricks/cli/libs/sync" "github.com/databricks/databricks-sdk-go/service/workspace" "github.com/fatih/color" ) @@ -46,20 +48,38 @@ func (m *delete) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { } // Clean up sync snapshot file - sync, err := GetSync(ctx, b) - if err != nil { - return diag.FromErr(err) - } - err = sync.DestroySnapshot(ctx) + err = deleteSnapshotFile(ctx, b) if err != nil { return diag.FromErr(err) } - cmdio.LogString(ctx, fmt.Sprintf("Deleted snapshot file at %s", sync.SnapshotPath())) cmdio.LogString(ctx, "Successfully deleted files!") return nil } +// TODO: write test that verifies that the snapshot file is deleted. +// TODO: write test to prevent this regression. +func deleteSnapshotFile(ctx context.Context, b *bundle.Bundle) error { + cacheDir, err := b.CacheDir(ctx) + if err != nil { + return err + } + sp, err := sync.SnapshotPath(&sync.SyncOptions{ + SnapshotBasePath: cacheDir, + Host: b.WorkspaceClient().Config.Host, + RemotePath: b.Config.Workspace.FilePath, + }) + if err != nil { + return err + } + err = os.Remove(sp) + if err != nil && !os.IsNotExist(err) { + return fmt.Errorf("failed to destroy sync snapshot file: %s", err) + } + cmdio.LogString(ctx, fmt.Sprintf("Deleted snapshot file at %s", sp)) + return nil +} + func Delete() bundle.Mutator { return &delete{} } diff --git a/internal/bundle/destroy_test.go b/internal/bundle/destroy_test.go new file mode 100644 index 0000000000..9f83b76101 --- /dev/null +++ b/internal/bundle/destroy_test.go @@ -0,0 +1,67 @@ +package bundle + +import ( + "errors" + "os" + "path/filepath" + "testing" + + "github.com/databricks/cli/internal/acc" + "github.com/databricks/databricks-sdk-go/apierr" + "github.com/google/uuid" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestAccBundleDestroy(t *testing.T) { + ctx, wt := acc.WorkspaceTest(t) + w := wt.W + + uniqueId := uuid.New().String() + bundleRoot, err := initTestTemplate(t, ctx, "deploy_then_remove_resources", map[string]any{ + "unique_id": uniqueId, + }) + require.NoError(t, err) + + // Assert the snapshot file does not exist + _, err = os.ReadDir(filepath.Join(bundleRoot, ".databricks", "bundle", "sync-snapshots")) + assert.ErrorIs(t, err, os.ErrNotExist) + + // deploy pipeline + err = deployBundle(t, ctx, bundleRoot) + require.NoError(t, err) + + // Assert the snapshot file exists + entries, err := os.ReadDir(filepath.Join(bundleRoot, ".databricks", "bundle", "default", "sync-snapshots")) + assert.NoError(t, err) + assert.Len(t, entries, 1) + + // Assert bundle deployment path is created + remoteRoot := getBundleRemoteRootPath(w, t, uniqueId) + _, err = w.Workspace.GetStatusByPath(ctx, remoteRoot) + assert.NoError(t, err) + + // assert pipeline is created + pipelineName := "test-bundle-pipeline-" + uniqueId + pipeline, err := w.Pipelines.GetByName(ctx, pipelineName) + require.NoError(t, err) + assert.Equal(t, pipeline.Name, pipelineName) + + // destroy bundle + err = destroyBundle(t, ctx, bundleRoot) + require.NoError(t, err) + + // assert pipeline is deleted + _, err = w.Pipelines.GetByName(ctx, pipelineName) + assert.ErrorContains(t, err, "does not exist") + + // Assert snapshot file is deleted + _, err = os.ReadDir(filepath.Join(bundleRoot, ".databricks", "bundle", "sync-snapshots")) + assert.ErrorIs(t, err, os.ErrNotExist) + + // Assert bundle deployment path is deleted + _, err = w.Workspace.GetStatusByPath(ctx, remoteRoot) + apiErr := &apierr.APIError{} + assert.True(t, errors.As(err, &apiErr)) + assert.Equal(t, "RESOURCE_DOES_NOT_EXIST", apiErr.ErrorCode) +} From aba824dfec721d898bef7c797cb1eeac14669de0 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Thu, 18 Apr 2024 14:41:03 +0200 Subject: [PATCH 2/5] remove unused functions --- libs/sync/snapshot.go | 8 -------- libs/sync/sync.go | 4 ---- 2 files changed, 12 deletions(-) diff --git a/libs/sync/snapshot.go b/libs/sync/snapshot.go index 06b4d13bca..a27a8c84f6 100644 --- a/libs/sync/snapshot.go +++ b/libs/sync/snapshot.go @@ -138,14 +138,6 @@ func (s *Snapshot) Save(ctx context.Context) error { return nil } -func (s *Snapshot) Destroy(ctx context.Context) error { - err := os.Remove(s.SnapshotPath) - if err != nil && !os.IsNotExist(err) { - return fmt.Errorf("failed to destroy sync snapshot file: %s", err) - } - return nil -} - func loadOrNewSnapshot(ctx context.Context, opts *SyncOptions) (*Snapshot, error) { snapshot, err := newSnapshot(ctx, opts) if err != nil { diff --git a/libs/sync/sync.go b/libs/sync/sync.go index 78faa0c8f4..30b68ccf33 100644 --- a/libs/sync/sync.go +++ b/libs/sync/sync.go @@ -216,10 +216,6 @@ func (s *Sync) GetFileList(ctx context.Context) ([]fileset.File, error) { return all.Iter(), nil } -func (s *Sync) DestroySnapshot(ctx context.Context) error { - return s.snapshot.Destroy(ctx) -} - func (s *Sync) SnapshotPath() string { return s.snapshot.SnapshotPath } From bfdd6c120320ff292a06a335a02cb1e33f945f29 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Thu, 18 Apr 2024 14:45:53 +0200 Subject: [PATCH 3/5] cleanup + fix test --- bundle/deploy/files/delete.go | 2 -- internal/bundle/destroy_test.go | 11 +++++++---- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/bundle/deploy/files/delete.go b/bundle/deploy/files/delete.go index 7a9e29da20..91483db259 100644 --- a/bundle/deploy/files/delete.go +++ b/bundle/deploy/files/delete.go @@ -57,8 +57,6 @@ func (m *delete) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { return nil } -// TODO: write test that verifies that the snapshot file is deleted. -// TODO: write test to prevent this regression. func deleteSnapshotFile(ctx context.Context, b *bundle.Bundle) error { cacheDir, err := b.CacheDir(ctx) if err != nil { diff --git a/internal/bundle/destroy_test.go b/internal/bundle/destroy_test.go index 9f83b76101..43c05fbaea 100644 --- a/internal/bundle/destroy_test.go +++ b/internal/bundle/destroy_test.go @@ -23,8 +23,10 @@ func TestAccBundleDestroy(t *testing.T) { }) require.NoError(t, err) + snapshotsDir := filepath.Join(bundleRoot, ".databricks", "bundle", "default", "sync-snapshots") + // Assert the snapshot file does not exist - _, err = os.ReadDir(filepath.Join(bundleRoot, ".databricks", "bundle", "sync-snapshots")) + _, err = os.ReadDir(snapshotsDir) assert.ErrorIs(t, err, os.ErrNotExist) // deploy pipeline @@ -32,7 +34,7 @@ func TestAccBundleDestroy(t *testing.T) { require.NoError(t, err) // Assert the snapshot file exists - entries, err := os.ReadDir(filepath.Join(bundleRoot, ".databricks", "bundle", "default", "sync-snapshots")) + entries, err := os.ReadDir(snapshotsDir) assert.NoError(t, err) assert.Len(t, entries, 1) @@ -56,8 +58,9 @@ func TestAccBundleDestroy(t *testing.T) { assert.ErrorContains(t, err, "does not exist") // Assert snapshot file is deleted - _, err = os.ReadDir(filepath.Join(bundleRoot, ".databricks", "bundle", "sync-snapshots")) - assert.ErrorIs(t, err, os.ErrNotExist) + entries, err = os.ReadDir(snapshotsDir) + require.NoError(t, err) + assert.Len(t, entries, 0) // Assert bundle deployment path is deleted _, err = w.Workspace.GetStatusByPath(ctx, remoteRoot) From e230856f7421930d734e4baacee239b743146dc2 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Thu, 18 Apr 2024 23:18:03 +0200 Subject: [PATCH 4/5] reuse GetSyncOptions --- bundle/deploy/files/delete.go | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/bundle/deploy/files/delete.go b/bundle/deploy/files/delete.go index 91483db259..a99be6b1d4 100644 --- a/bundle/deploy/files/delete.go +++ b/bundle/deploy/files/delete.go @@ -58,15 +58,11 @@ func (m *delete) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { } func deleteSnapshotFile(ctx context.Context, b *bundle.Bundle) error { - cacheDir, err := b.CacheDir(ctx) + opts, err := GetSyncOptions(ctx, b) if err != nil { - return err + return fmt.Errorf("cannot get sync options: %w", err) } - sp, err := sync.SnapshotPath(&sync.SyncOptions{ - SnapshotBasePath: cacheDir, - Host: b.WorkspaceClient().Config.Host, - RemotePath: b.Config.Workspace.FilePath, - }) + sp, err := sync.SnapshotPath(opts) if err != nil { return err } From b697642c3598f8b7ee9d1de361a424434dc169e9 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Fri, 19 Apr 2024 13:42:09 +0200 Subject: [PATCH 5/5] remove print --- bundle/deploy/files/delete.go | 1 - 1 file changed, 1 deletion(-) diff --git a/bundle/deploy/files/delete.go b/bundle/deploy/files/delete.go index bad32af4eb..066368a6b2 100644 --- a/bundle/deploy/files/delete.go +++ b/bundle/deploy/files/delete.go @@ -70,7 +70,6 @@ func deleteSnapshotFile(ctx context.Context, b *bundle.Bundle) error { if err != nil && !os.IsNotExist(err) { return fmt.Errorf("failed to destroy sync snapshot file: %s", err) } - cmdio.LogString(ctx, fmt.Sprintf("Deleted snapshot file at %s", sp)) return nil }