diff --git a/.nextchanges/bundles/destroy-removes-bundle-directory.md b/.nextchanges/bundles/destroy-removes-bundle-directory.md new file mode 100644 index 0000000000..5bbba58514 --- /dev/null +++ b/.nextchanges/bundles/destroy-removes-bundle-directory.md @@ -0,0 +1 @@ +`bundle destroy` now also removes the `~/.bundle/` directory the deployment lived under, instead of only `~/.bundle//`. The directory is removed non-recursively, so it stays in place while another target of the same bundle is still deployed there. Previously every destroy left an empty directory behind, which accumulated in the workspace and counted against its child-node limit ([#6317](https://github.com/databricks/cli/pull/6317)). diff --git a/acceptance/bundle/destroy/all-resources/output.txt b/acceptance/bundle/destroy/all-resources/output.txt index f8eb4d90e0..207a85f8c8 100644 --- a/acceptance/bundle/destroy/all-resources/output.txt +++ b/acceptance/bundle/destroy/all-resources/output.txt @@ -21,3 +21,9 @@ Streaming Tables (STs) and Materialized Views (MVs) managed by them. Set 'cascad All files and directories at the following location will be deleted: /Workspace/Users/[USERNAME]/.bundle/test-bundle/default Destroy: 2 deleted + +=== Assert the bundle directory is deleted +>>> errcode [CLI] workspace get-status //Workspace/Users/[USERNAME]/.bundle/test-bundle +Error: Path (//Workspace/Users/[USERNAME]/.bundle/test-bundle) doesn't exist. + +Exit code: 1 diff --git a/acceptance/bundle/destroy/all-resources/script b/acceptance/bundle/destroy/all-resources/script index ceb8acee91..71160f7c3f 100644 --- a/acceptance/bundle/destroy/all-resources/script +++ b/acceptance/bundle/destroy/all-resources/script @@ -1,2 +1,7 @@ trace $CLI bundle deploy trace $CLI bundle destroy --auto-approve + +# Destroy removes the target directory, and the .bundle/ parent goes with it once +# nothing else is deployed under it. Double slash keeps Windows from rewriting the path. +title "Assert the bundle directory is deleted" +trace errcode $CLI workspace get-status "//Workspace/Users/${CURRENT_USER_NAME}/.bundle/test-bundle" diff --git a/acceptance/bundle/destroy/sibling-target/databricks.yml b/acceptance/bundle/destroy/sibling-target/databricks.yml new file mode 100644 index 0000000000..a12a045794 --- /dev/null +++ b/acceptance/bundle/destroy/sibling-target/databricks.yml @@ -0,0 +1,7 @@ +bundle: + name: test-bundle + +targets: + dev: + default: true + prod: diff --git a/acceptance/bundle/destroy/sibling-target/out.test.toml b/acceptance/bundle/destroy/sibling-target/out.test.toml new file mode 100644 index 0000000000..98ea504048 --- /dev/null +++ b/acceptance/bundle/destroy/sibling-target/out.test.toml @@ -0,0 +1,2 @@ +Cloud = false +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["terraform", "direct"] diff --git a/acceptance/bundle/destroy/sibling-target/output.txt b/acceptance/bundle/destroy/sibling-target/output.txt new file mode 100644 index 0000000000..48cbff888c --- /dev/null +++ b/acceptance/bundle/destroy/sibling-target/output.txt @@ -0,0 +1,35 @@ + +>>> [CLI] bundle deploy -t dev +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/test-bundle/dev/files... +Files: 3 uploaded, 0 deleted +Resources: 0 created, 0 changed, 0 deleted, 0 unchanged + +>>> [CLI] bundle deploy -t prod +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/test-bundle/prod/files... +Files: 3 uploaded, 0 deleted +Resources: 0 created, 0 changed, 0 deleted, 0 unchanged + +=== Destroy one target while the other is still deployed +>>> [CLI] bundle destroy -t dev --auto-approve +All files and directories at the following location will be deleted: /Workspace/Users/[USERNAME]/.bundle/test-bundle/dev + +Destroy: 0 deleted + +=== Assert the bundle directory is kept for the remaining target +>>> [CLI] workspace get-status //Workspace/Users/[USERNAME]/.bundle/test-bundle +{ + "path": "/Users/[USERNAME]/.bundle/test-bundle", + "object_type": "DIRECTORY" +} + +=== Destroy the remaining target +>>> [CLI] bundle destroy -t prod --auto-approve +All files and directories at the following location will be deleted: /Workspace/Users/[USERNAME]/.bundle/test-bundle/prod + +Destroy: 0 deleted + +=== Assert the bundle directory is deleted +>>> errcode [CLI] workspace get-status //Workspace/Users/[USERNAME]/.bundle/test-bundle +Error: Path (//Workspace/Users/[USERNAME]/.bundle/test-bundle) doesn't exist. + +Exit code: 1 diff --git a/acceptance/bundle/destroy/sibling-target/script b/acceptance/bundle/destroy/sibling-target/script new file mode 100644 index 0000000000..1f2db9fec6 --- /dev/null +++ b/acceptance/bundle/destroy/sibling-target/script @@ -0,0 +1,17 @@ +# Double slash keeps Windows from rewriting the path. +BUNDLE_DIR="//Workspace/Users/${CURRENT_USER_NAME}/.bundle/test-bundle" + +trace $CLI bundle deploy -t dev +trace $CLI bundle deploy -t prod + +title "Destroy one target while the other is still deployed" +trace $CLI bundle destroy -t dev --auto-approve + +title "Assert the bundle directory is kept for the remaining target" +trace $CLI workspace get-status "${BUNDLE_DIR}" | jq '{path, object_type}' + +title "Destroy the remaining target" +trace $CLI bundle destroy -t prod --auto-approve + +title "Assert the bundle directory is deleted" +trace errcode $CLI workspace get-status "${BUNDLE_DIR}" diff --git a/acceptance/bundle/resource_deps/remote_app_url/output.txt b/acceptance/bundle/resource_deps/remote_app_url/output.txt index 84d8da92e3..9301329af1 100644 --- a/acceptance/bundle/resource_deps/remote_app_url/output.txt +++ b/acceptance/bundle/resource_deps/remote_app_url/output.txt @@ -107,6 +107,13 @@ Destroy: 2 deleted "method": "DELETE", "path": "/api/2.0/pipelines/[UUID]" } +{ + "method": "POST", + "path": "/api/2.0/workspace/delete", + "body": { + "path": "/Workspace/Users/[USERNAME]/.bundle/test-bundle" + } +} { "method": "POST", "path": "/api/2.0/workspace/delete", diff --git a/acceptance/bundle/user_agent/simple/out.requests.destroy.direct.json b/acceptance/bundle/user_agent/simple/out.requests.destroy.direct.json index c67f22e373..1e97537e4d 100644 --- a/acceptance/bundle/user_agent/simple/out.requests.destroy.direct.json +++ b/acceptance/bundle/user_agent/simple/out.requests.destroy.direct.json @@ -132,6 +132,18 @@ "User": "[USERNAME]" } } +{ + "headers": { + "User-Agent": [ + "cli/[CLI_VERSION] databricks-sdk-go/[SDK_VERSION] go/[GO_VERSION] os/[OS] cmd/bundle_destroy cmd-exec-id/[UUID] interactive/none engine/direct auth/pat" + ] + }, + "method": "POST", + "path": "/api/2.0/workspace/delete", + "body": { + "path": "/Workspace/Users/[USERNAME]/.bundle/test-bundle" + } +} { "headers": { "User-Agent": [ diff --git a/acceptance/bundle/user_agent/simple/out.requests.destroy.terraform.json b/acceptance/bundle/user_agent/simple/out.requests.destroy.terraform.json index 09b923be4d..325ff5b60a 100644 --- a/acceptance/bundle/user_agent/simple/out.requests.destroy.terraform.json +++ b/acceptance/bundle/user_agent/simple/out.requests.destroy.terraform.json @@ -111,6 +111,18 @@ "User": "[USERNAME]" } } +{ + "headers": { + "User-Agent": [ + "cli/[CLI_VERSION] databricks-sdk-go/[SDK_VERSION] go/[GO_VERSION] os/[OS] cmd/bundle_destroy cmd-exec-id/[UUID] interactive/none engine/terraform auth/pat" + ] + }, + "method": "POST", + "path": "/api/2.0/workspace/delete", + "body": { + "path": "/Workspace/Users/[USERNAME]/.bundle/test-bundle" + } +} { "headers": { "User-Agent": [ diff --git a/bundle/deploy/files/delete.go b/bundle/deploy/files/delete.go index 562437e543..7f3502cd06 100644 --- a/bundle/deploy/files/delete.go +++ b/bundle/deploy/files/delete.go @@ -6,13 +6,17 @@ import ( "fmt" "io/fs" "os" + "path" "github.com/databricks/cli/bundle" "github.com/databricks/cli/libs/diag" + "github.com/databricks/cli/libs/log" "github.com/databricks/cli/libs/sync" "github.com/databricks/databricks-sdk-go/service/workspace" ) +const bundleDirName = ".bundle" + type delete struct{} func (m *delete) Name() string { @@ -28,6 +32,8 @@ func (m *delete) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { return diag.FromErr(err) } + removeEmptyBundleDir(ctx, b) + // Clean up sync snapshot file err = deleteSnapshotFile(ctx, b) if err != nil { @@ -36,6 +42,26 @@ func (m *delete) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { return nil } +// removeEmptyBundleDir removes the ~/.bundle/ directory left behind once the +// target subdirectory under it is gone. The delete is not recursive, so it only +// succeeds while no other target of the bundle is still deployed there; both a +// remaining sibling target and an already-removed directory surface as an error that +// is expected and ignored. +func removeEmptyBundleDir(ctx context.Context, b *bundle.Bundle) { + dir := path.Dir(b.Config.Workspace.RootPath) + + // root_path is user-configurable, so only clean up the layout the CLI generates + // (~/.bundle//) instead of deleting the parent of an arbitrary path. + if path.Base(path.Dir(dir)) != bundleDirName { + return + } + + err := b.WorkspaceClient(ctx).Workspace.Delete(ctx, workspace.Delete{Path: dir}) + if err != nil { + log.Debugf(ctx, "Leaving %s in place: %s", dir, err) + } +} + func deleteSnapshotFile(ctx context.Context, b *bundle.Bundle) error { opts, err := GetSyncOptions(ctx, b) if err != nil { diff --git a/libs/testserver/fake_workspace.go b/libs/testserver/fake_workspace.go index 1caec84d1e..7625c4d854 100644 --- a/libs/testserver/fake_workspace.go +++ b/libs/testserver/fake_workspace.go @@ -698,9 +698,21 @@ func (s *FakeWorkspace) WorkspaceExport(path string) []byte { return s.files[path].Data } -func (s *FakeWorkspace) WorkspaceDelete(path string, recursive bool) { +// WorkspaceDelete implements POST /api/2.0/workspace/delete. As in the real API, a +// non-recursive delete of a directory that still has children fails instead of removing +// it, which is what lets a caller delete a directory only if it is empty. +func (s *FakeWorkspace) WorkspaceDelete(path string, recursive bool) Response { defer s.LockUnlock()() if !recursive { + if _, isDir := s.directories[path]; isDir && s.hasChildren(path) { + return Response{ + StatusCode: 400, + Body: map[string]string{ + "error_code": "DIRECTORY_NOT_EMPTY", + "message": "Directory " + path + " is not empty", + }, + } + } delete(s.files, path) delete(s.directories, path) } else { @@ -715,6 +727,24 @@ func (s *FakeWorkspace) WorkspaceDelete(path string, recursive bool) { } } } + return Response{} +} + +// hasChildren reports whether any file or directory lives under dirPath. Callers must +// hold the lock. +func (s *FakeWorkspace) hasChildren(dirPath string) bool { + prefix := dirPath + "/" + for key := range s.files { + if strings.HasPrefix(key, prefix) { + return true + } + } + for key := range s.directories { + if strings.HasPrefix(key, prefix) { + return true + } + } + return false } func (s *FakeWorkspace) WorkspaceFilesImportFile(filePath string, body []byte, overwrite bool) Response { diff --git a/libs/testserver/handlers.go b/libs/testserver/handlers.go index 099230ad94..1cc037dcb6 100644 --- a/libs/testserver/handlers.go +++ b/libs/testserver/handlers.go @@ -143,8 +143,7 @@ func AddDefaultHandlers(server *Server) { StatusCode: 500, } } - req.Workspace.WorkspaceDelete(request.Path, request.Recursive) - return "" + return req.Workspace.WorkspaceDelete(request.Path, request.Recursive) }) server.Handle("POST", "/api/2.0/workspace-files/import-file/{path...}", func(req Request) any { diff --git a/libs/testserver/workspace_test.go b/libs/testserver/workspace_test.go index 5e4753246c..e8d8c4a12a 100644 --- a/libs/testserver/workspace_test.go +++ b/libs/testserver/workspace_test.go @@ -31,6 +31,18 @@ func mkdirs(t *testing.T, baseURL, path string) { require.Equal(t, 200, resp.StatusCode) } +func workspaceDelete(t *testing.T, baseURL, path string, recursive bool) int { + t.Helper() + body, err := json.Marshal(map[string]any{"path": path, "recursive": recursive}) + require.NoError(t, err) + req, _ := http.NewRequest(http.MethodPost, baseURL+"/api/2.0/workspace/delete", strings.NewReader(string(body))) + req.Header.Set("Authorization", "Bearer test-token") + resp, err := http.DefaultClient.Do(req) + require.NoError(t, err) + defer resp.Body.Close() + return resp.StatusCode +} + func getStatus(t *testing.T, baseURL, path string) int { t.Helper() req, _ := http.NewRequest(http.MethodGet, baseURL+"/api/2.0/workspace/get-status?path="+path, nil) @@ -53,6 +65,22 @@ func TestWorkspaceImportRejectsMissingParent(t *testing.T) { assert.Equal(t, 200, importFile(t, server.URL, "/test-dir/file.py", "content")) } +// A non-recursive delete only removes an empty directory, so a caller can use it to +// clean up a parent directory without touching one that still holds a sibling. +func TestWorkspaceDeleteNonRecursiveRequiresEmptyDirectory(t *testing.T) { + server := testserver.New(t) + testserver.AddDefaultHandlers(server) + + mkdirs(t, server.URL, "/a/b/c") + + assert.Equal(t, 400, workspaceDelete(t, server.URL, "/a/b", false)) + assert.Equal(t, 200, getStatus(t, server.URL, "/a/b")) + + assert.Equal(t, 200, workspaceDelete(t, server.URL, "/a/b/c", false)) + assert.Equal(t, 200, workspaceDelete(t, server.URL, "/a/b", false)) + assert.Equal(t, 404, getStatus(t, server.URL, "/a/b")) +} + // mkdirs creates all intermediate directories, matching "mkdir -p". func TestWorkspaceMkdirsRecursive(t *testing.T) { server := testserver.New(t)