From e008c2bd8cf0e3394b680c069c32316341e3cd4f Mon Sep 17 00:00:00 2001
From: shreyas-goenka <88374338+shreyas-goenka@users.noreply.github.com>
Date: Fri, 19 Apr 2024 17:18:04 +0530
Subject: [PATCH] Cleanup remote file path on bundle destroy (#1374)

## Changes
The sync struct initialization would recreate the deleted `file_path`.
This PR moves to not initializing the sync object to delete the
snapshot, thus fixing the lingering `file_path` after `bundle destroy`.

## Tests
Manually, and a integration test to prevent regression.
---
 bundle/deploy/files/delete.go   | 25 +++++++++---
 internal/bundle/destroy_test.go | 70 +++++++++++++++++++++++++++++++++
 libs/sync/snapshot.go           |  8 ----
 libs/sync/sync.go               |  4 --
 4 files changed, 89 insertions(+), 18 deletions(-)
 create mode 100644 internal/bundle/destroy_test.go

diff --git a/bundle/deploy/files/delete.go b/bundle/deploy/files/delete.go
index 46c55446..066368a6 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,31 @@ func (m *delete) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics {
 	}
 
 	// Clean up sync snapshot file
-	sync, err := GetSync(ctx, bundle.ReadOnly(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
 }
 
+func deleteSnapshotFile(ctx context.Context, b *bundle.Bundle) error {
+	opts, err := GetSyncOptions(ctx, bundle.ReadOnly(b))
+	if err != nil {
+		return fmt.Errorf("cannot get sync options: %w", err)
+	}
+	sp, err := sync.SnapshotPath(opts)
+	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)
+	}
+	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 00000000..43c05fba
--- /dev/null
+++ b/internal/bundle/destroy_test.go
@@ -0,0 +1,70 @@
+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)
+
+	snapshotsDir := filepath.Join(bundleRoot, ".databricks", "bundle", "default", "sync-snapshots")
+
+	// Assert the snapshot file does not exist
+	_, err = os.ReadDir(snapshotsDir)
+	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(snapshotsDir)
+	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
+	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)
+	apiErr := &apierr.APIError{}
+	assert.True(t, errors.As(err, &apiErr))
+	assert.Equal(t, "RESOURCE_DOES_NOT_EXIST", apiErr.ErrorCode)
+}
diff --git a/libs/sync/snapshot.go b/libs/sync/snapshot.go
index 06b4d13b..a27a8c84 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 78faa0c8..30b68ccf 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
 }