From fa57532f9eaa79a50f9aa9e527617d8d579f5264 Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Fri, 9 Oct 2026 12:23:17 +0200 Subject: [PATCH 1/4] Fixed immutable folder failing with DMS or top level permissions --- .../bundles/immutable-folder-paths.md | 1 + .../dms/immutable-folder/databricks.yml.tmpl | 19 ++++++++ .../bundle/dms/immutable-folder/out.test.toml | 3 ++ .../bundle/dms/immutable-folder/output.txt | 43 +++++++++++++++++ acceptance/bundle/dms/immutable-folder/script | 30 ++++++++++++ .../bundle/dms/immutable-folder/src/main.py | 1 + .../bundle/dms/immutable-folder/test.toml | 20 ++++++++ .../databricks.yml | 9 ++++ .../out.test.toml | 2 + .../immutable_folder_permissions/output.txt | 21 +++++++++ .../immutable_folder_permissions/script | 3 ++ .../immutable_folder_permissions/test.toml | 2 + bundle/config/validate/folder_permissions.go | 9 +++- bundle/phases/dms.go | 47 +++++++++++++++++-- libs/testserver/bundledeployments.go | 10 +++- libs/testserver/fake_workspace.go | 7 +++ 16 files changed, 222 insertions(+), 5 deletions(-) create mode 100644 .nextchanges/bundles/immutable-folder-paths.md create mode 100644 acceptance/bundle/dms/immutable-folder/databricks.yml.tmpl create mode 100644 acceptance/bundle/dms/immutable-folder/out.test.toml create mode 100644 acceptance/bundle/dms/immutable-folder/output.txt create mode 100644 acceptance/bundle/dms/immutable-folder/script create mode 100644 acceptance/bundle/dms/immutable-folder/src/main.py create mode 100644 acceptance/bundle/dms/immutable-folder/test.toml create mode 100644 acceptance/bundle/validate/immutable_folder_permissions/databricks.yml create mode 100644 acceptance/bundle/validate/immutable_folder_permissions/out.test.toml create mode 100644 acceptance/bundle/validate/immutable_folder_permissions/output.txt create mode 100644 acceptance/bundle/validate/immutable_folder_permissions/script create mode 100644 acceptance/bundle/validate/immutable_folder_permissions/test.toml diff --git a/.nextchanges/bundles/immutable-folder-paths.md b/.nextchanges/bundles/immutable-folder-paths.md new file mode 100644 index 00000000000..72571e42502 --- /dev/null +++ b/.nextchanges/bundles/immutable-folder-paths.md @@ -0,0 +1 @@ +* Fixed `experimental.immutable_folder` failing with `experimental.deployment_history` ("workspace_info.file_path must be an absolute workspace path") and failing `bundle validate` when top-level `permissions` are set. diff --git a/acceptance/bundle/dms/immutable-folder/databricks.yml.tmpl b/acceptance/bundle/dms/immutable-folder/databricks.yml.tmpl new file mode 100644 index 00000000000..dad517c9cfa --- /dev/null +++ b/acceptance/bundle/dms/immutable-folder/databricks.yml.tmpl @@ -0,0 +1,19 @@ +bundle: + name: dms-immutable-folder-$UNIQUE_NAME +experimental: + immutable_folder: true + deployment_history: true + +resources: + jobs: + foo: + name: foo + tasks: + - task_key: main + spark_python_task: + python_file: ./src/main.py + environment_key: env + environments: + - environment_key: env + spec: + environment_version: "4" diff --git a/acceptance/bundle/dms/immutable-folder/out.test.toml b/acceptance/bundle/dms/immutable-folder/out.test.toml new file mode 100644 index 00000000000..0cedb09be0f --- /dev/null +++ b/acceptance/bundle/dms/immutable-folder/out.test.toml @@ -0,0 +1,3 @@ +Cloud = false +EnvMatrix.DMS = ["true"] +EnvMatrix.READPLAN = ["", "1"] diff --git a/acceptance/bundle/dms/immutable-folder/output.txt b/acceptance/bundle/dms/immutable-folder/output.txt new file mode 100644 index 00000000000..a45aa8ba189 --- /dev/null +++ b/acceptance/bundle/dms/immutable-folder/output.txt @@ -0,0 +1,43 @@ + +=== Deploying with an immutable folder records the resolved snapshot path as the deployment's file_path +>>> [CLI] bundle deploy +Created internal_immutable_snapshots.immutable +Created jobs.foo +Files: 0 uploaded, 0 deleted +Resources: 2 created, 0 changed, 0 deleted, 0 unchanged + +>>> MSYS_NO_PATHCONV=1 [CLI] api get /api/2.0/bundle/deployments/[DEPLOYMENT_ID] +{ + "workspace_info": { + "root_path": "/Workspace/Users/[USERNAME]/.bundle/dms-immutable-folder-[UNIQUE_NAME]/default", + "file_path": "/Workspace/Users/[UUID]/.snapshots/[SNAPSHOT_HASH]/[SNAPSHOT_HASH]/files" + } +} + +=== A new snapshot moves file_path, which is then updated on the deployment +>>> [CLI] bundle deploy +Recreated internal_immutable_snapshots.immutable +Updated jobs.foo +Files: 0 uploaded, 0 deleted +Resources: 1 created, 1 changed, 1 deleted, 0 unchanged + +>>> MSYS_NO_PATHCONV=1 [CLI] api get /api/2.0/bundle/deployments/[DEPLOYMENT_ID] +{ + "workspace_info": { + "root_path": "/Workspace/Users/[USERNAME]/.bundle/dms-immutable-folder-[UNIQUE_NAME]/default", + "file_path": "/Workspace/Users/[UUID]/.snapshots/[SNAPSHOT_HASH]/[SNAPSHOT_HASH]/files" + } +} + +=== An unchanged snapshot keeps the recorded file_path +>>> [CLI] bundle deploy +Files: 0 uploaded, 0 deleted +Resources: 0 created, 0 changed, 0 deleted, 2 unchanged + +>>> MSYS_NO_PATHCONV=1 [CLI] api get /api/2.0/bundle/deployments/[DEPLOYMENT_ID] +{ + "workspace_info": { + "root_path": "/Workspace/Users/[USERNAME]/.bundle/dms-immutable-folder-[UNIQUE_NAME]/default", + "file_path": "/Workspace/Users/[UUID]/.snapshots/[SNAPSHOT_HASH]/[SNAPSHOT_HASH]/files" + } +} diff --git a/acceptance/bundle/dms/immutable-folder/script b/acceptance/bundle/dms/immutable-folder/script new file mode 100644 index 00000000000..ff70d11a08f --- /dev/null +++ b/acceptance/bundle/dms/immutable-folder/script @@ -0,0 +1,30 @@ +envsubst < databricks.yml.tmpl > databricks.yml + +deployment_workspace_info() { + deployment_id=$(MSYS_NO_PATHCONV=1 $CLI workspace get-status "/Workspace/Users/${CURRENT_USER_NAME}/.bundle/dms-immutable-folder-${UNIQUE_NAME}/default/state/resources.deployment.json" -o json | python3 -c 'import sys,json; print(json.load(sys.stdin)["object_id"])') + add_repl "$deployment_id" DEPLOYMENT_ID + # MSYS_NO_PATHCONV: Git Bash on Windows would rewrite the leading-'/' API path. + trace MSYS_NO_PATHCONV=1 $CLI api get "/api/2.0/bundle/deployments/${deployment_id}" | jq '{workspace_info}' +} + +deploy() { + mkdir -p .databricks + $CLI bundle plan -o json > .databricks/plan.json + trace $CLI bundle deploy $(readplanarg .databricks/plan.json) + rm .databricks/plan.json +} + +title "Deploying with an immutable folder records the resolved snapshot path as the deployment's file_path" +deploy +deployment_workspace_info + +title "A new snapshot moves file_path, which is then updated on the deployment" +echo 'print("changed")' > src/main.py +deploy +deployment_workspace_info + +title "An unchanged snapshot keeps the recorded file_path" +deploy +deployment_workspace_info + +rm -f "$OUT_REQUESTS" diff --git a/acceptance/bundle/dms/immutable-folder/src/main.py b/acceptance/bundle/dms/immutable-folder/src/main.py new file mode 100644 index 00000000000..11b15b1a458 --- /dev/null +++ b/acceptance/bundle/dms/immutable-folder/src/main.py @@ -0,0 +1 @@ +print("hello") diff --git a/acceptance/bundle/dms/immutable-folder/test.toml b/acceptance/bundle/dms/immutable-folder/test.toml new file mode 100644 index 00000000000..0aabadfa242 --- /dev/null +++ b/acceptance/bundle/dms/immutable-folder/test.toml @@ -0,0 +1,20 @@ +# The immutable folder API is not available against a real workspace yet. +Cloud = false + +EnvMatrix.READPLAN = ["", "1"] + +Ignore = [ + '.databricks', + 'databricks.yml', +] + +# The snapshot path is content-addressed. +[[Repls]] +Old = '[0-9a-f]{64}' +New = '[SNAPSHOT_HASH]' + +# When READPLAN=1, "bundle deploy" is called as "bundle deploy --plan .databricks/plan.json". +# Normalize so both variants produce identical output. +[[Repls]] +Old = ' --plan .databricks/plan.json' +New = '' diff --git a/acceptance/bundle/validate/immutable_folder_permissions/databricks.yml b/acceptance/bundle/validate/immutable_folder_permissions/databricks.yml new file mode 100644 index 00000000000..9d9cb5407eb --- /dev/null +++ b/acceptance/bundle/validate/immutable_folder_permissions/databricks.yml @@ -0,0 +1,9 @@ +bundle: + name: my-bundle + +experimental: + immutable_folder: true + +permissions: + - level: CAN_VIEW + user_name: viewer@example.com diff --git a/acceptance/bundle/validate/immutable_folder_permissions/out.test.toml b/acceptance/bundle/validate/immutable_folder_permissions/out.test.toml new file mode 100644 index 00000000000..a927a5fbc06 --- /dev/null +++ b/acceptance/bundle/validate/immutable_folder_permissions/out.test.toml @@ -0,0 +1,2 @@ +Cloud = false +EnvMatrix.DMS = ["", "true"] diff --git a/acceptance/bundle/validate/immutable_folder_permissions/output.txt b/acceptance/bundle/validate/immutable_folder_permissions/output.txt new file mode 100644 index 00000000000..73c193f0520 --- /dev/null +++ b/acceptance/bundle/validate/immutable_folder_permissions/output.txt @@ -0,0 +1,21 @@ + +>>> [CLI] bundle validate +Recommendation: permissions section should explicitly include the current deployment identity '[USERNAME]' or one of its groups +If it is not included, CAN_MANAGE permissions are only applied if the present identity is used to deploy. + +Consider using a adding a top-level permissions section such as the following: + + permissions: + - user_name: [USERNAME] + level: CAN_MANAGE + +See https://docs.databricks.com/dev-tools/bundles/permissions.html to learn more about permission configuration. + in databricks.yml:8:3 + +Name: my-bundle +Target: default +Workspace: + User: [USERNAME] + Path: /Workspace/Users/[USERNAME]/.bundle/my-bundle/default + +Found 1 recommendation diff --git a/acceptance/bundle/validate/immutable_folder_permissions/script b/acceptance/bundle/validate/immutable_folder_permissions/script new file mode 100644 index 00000000000..408f51f529f --- /dev/null +++ b/acceptance/bundle/validate/immutable_folder_permissions/script @@ -0,0 +1,3 @@ +# file_path and artifact_path point into the snapshot, which does not exist until deploy, +# so validate must not check their folder permissions. +trace $CLI bundle validate diff --git a/acceptance/bundle/validate/immutable_folder_permissions/test.toml b/acceptance/bundle/validate/immutable_folder_permissions/test.toml new file mode 100644 index 00000000000..8f238a7f609 --- /dev/null +++ b/acceptance/bundle/validate/immutable_folder_permissions/test.toml @@ -0,0 +1,2 @@ +Cloud = false +Ignore = [".databricks"] diff --git a/bundle/config/validate/folder_permissions.go b/bundle/config/validate/folder_permissions.go index bd8a416f609..0a201a272d8 100644 --- a/bundle/config/validate/folder_permissions.go +++ b/bundle/config/validate/folder_permissions.go @@ -18,7 +18,14 @@ func (f *folderPermissions) Apply(ctx context.Context, b *bundle.Bundle) diag.Di return nil } - bundlePaths := paths.CollectUniqueWorkspacePathPrefixes(b.Config.Workspace).Paths + workspace := b.Config.Workspace + if b.IsImmutableFolder() { + // file_path and artifact_path reference the snapshot, which does not exist until + // deploy, so there is no folder to check. See permissions.ApplyWorkspaceRootPermissions. + workspace.FilePath = "" + workspace.ArtifactPath = "" + } + bundlePaths := paths.CollectUniqueWorkspacePathPrefixes(workspace).Paths var diags diag.Diagnostics g, ctx := errgroup.WithContext(ctx) diff --git a/bundle/phases/dms.go b/bundle/phases/dms.go index 534b749daaa..751c20db23f 100644 --- a/bundle/phases/dms.go +++ b/bundle/phases/dms.go @@ -2,14 +2,18 @@ package phases import ( "context" + "encoding/json" "fmt" "net/url" + "path" "strconv" "strings" "github.com/databricks/cli/bundle" "github.com/databricks/cli/bundle/config" + "github.com/databricks/cli/bundle/config/resources" "github.com/databricks/cli/bundle/deployplan" + "github.com/databricks/cli/bundle/direct/dresources" "github.com/databricks/cli/internal/build" "github.com/databricks/cli/libs/cmdio" "github.com/databricks/cli/libs/dms" @@ -71,7 +75,11 @@ func actionToSDK(a deployplan.ActionType) (bundledeployments.OperationActionType func createOrUpdateDeployment(ctx context.Context, b *bundle.Bundle, current *bundledeployments.Deployment) { db := &b.DeploymentBundle w := b.WorkspaceClient(ctx) - metadata := deploymentMetadata(b) + metadata, err := deploymentMetadata(b) + if err != nil { + logdiag.LogError(ctx, fmt.Errorf("failed to compute deployment metadata: %w", err)) + return + } deploymentID := db.StateDB.DeploymentID if deploymentID == "" { dep := metadata.Deployment() @@ -180,7 +188,7 @@ func logDeploymentVersion(ctx context.Context, b *bundle.Bundle) { // deploymentMetadata describes the bundle this deploy came from and where it // landed, mirroring what bundle/deploy/metadata computes for the metadata file. -func deploymentMetadata(b *bundle.Bundle) dms.Metadata { +func deploymentMetadata(b *bundle.Bundle) (dms.Metadata, error) { p := dms.Metadata{ DisplayName: b.Config.Bundle.Name, TargetName: b.Config.Bundle.Target, @@ -191,6 +199,15 @@ func deploymentMetadata(b *bundle.Bundle) dms.Metadata { RootPath: b.Config.Workspace.RootPath, FilePath: b.Config.Workspace.FilePath, } + // With an immutable folder, file_path is a reference to the snapshot, which only + // resolves once the snapshot is uploaded. Its path is already known from the plan. + if b.IsImmutableFolder() { + snapshotPath, err := immutableSnapshotPath(b) + if err != nil { + return dms.Metadata{}, err + } + ws.FilePath = path.Join(snapshotPath, "files") + } // In a source-linked deployment files are not copied, so resources read them // from the sync root instead of file_path (see bundle/deploy/metadata.Compute). if config.IsExplicitlyEnabled(b.Config.Presets.SourceLinkedDeployment) { @@ -204,7 +221,31 @@ func deploymentMetadata(b *bundle.Bundle) dms.Metadata { ws.BundleRootPath = b.Config.Bundle.Git.BundleRootPath } p.Workspace = ws - return p + return p, nil +} + +// immutableSnapshotPath returns the workspace path of the immutable folder snapshot. The +// snapshot is not uploaded yet at this point, but its content-addressed path is already known. +func immutableSnapshotPath(b *bundle.Bundle) (string, error) { + if sv, ok := b.DeploymentBundle.StateCache.Load(resources.SnapshotKey); ok { + state, ok := sv.Value.(*dresources.SnapshotState) + if !ok { + return "", fmt.Errorf("unexpected state type %T for %s", sv.Value, resources.SnapshotKey) + } + return state.FullPath, nil + } + + // An unchanged snapshot has no planned state when deploying from a saved plan. Its + // recorded state is what the plan would have held. + entry, ok := b.DeploymentBundle.StateDB.GetResourceEntry(resources.SnapshotKey) + if !ok { + return "", fmt.Errorf("no state for %s", resources.SnapshotKey) + } + var state dresources.SnapshotState + if err := json.Unmarshal(entry.State, &state); err != nil { + return "", fmt.Errorf("failed to read state of %s: %w", resources.SnapshotKey, err) + } + return state.FullPath, nil } // deploymentModeToSDK maps the bundle target's mode to the DMS enum. An unset mode diff --git a/libs/testserver/bundledeployments.go b/libs/testserver/bundledeployments.go index 9e2e864d63b..b4019b75d0a 100644 --- a/libs/testserver/bundledeployments.go +++ b/libs/testserver/bundledeployments.go @@ -178,8 +178,16 @@ type dmsWorkspaceInfo struct { var dmsUpdatableDeploymentFields = []string{"display_name", "target_name", "deployment_mode", "workspace_info"} // checkWorkspaceInfo rejects a bundle_root_path without the git_folder_path it is relative to, -// which is what the service does. +// and a root_path or file_path that is not absolute, which is what the service does. func checkWorkspaceInfo(ws *bundledeployments.WorkspaceInfo) (Response, bool) { + if ws != nil { + if ws.RootPath != "" && !strings.HasPrefix(ws.RootPath, "/") { + return dmsInvalidArgument("workspace_info.root_path must be an absolute workspace path"), false + } + if ws.FilePath != "" && !strings.HasPrefix(ws.FilePath, "/") { + return dmsInvalidArgument("workspace_info.file_path must be an absolute workspace path"), false + } + } if ws != nil && (ws.GitFolderPath == "") != (ws.BundleRootPath == "") { return dmsInvalidArgument("workspace_info.git_folder_path and workspace_info.bundle_root_path must be set together"), false } diff --git a/libs/testserver/fake_workspace.go b/libs/testserver/fake_workspace.go index b73803751b6..f478f25c3a2 100644 --- a/libs/testserver/fake_workspace.go +++ b/libs/testserver/fake_workspace.go @@ -638,6 +638,13 @@ func isGitCliFolder(repoPath string) bool { } func (s *FakeWorkspace) WorkspaceGetStatus(requestPath string, returnGitInfo bool) Response { + if !strings.HasPrefix(requestPath, "/") { + return Response{ + StatusCode: 400, + Body: map[string]string{"error_code": "INVALID_PARAMETER_VALUE", "message": fmt.Sprintf("Path (%s) doesn't start with '/'", requestPath)}, + } + } + defer s.LockUnlock()() // The real API collapses duplicate slashes, so look up the cleaned path. From 0e17c9a0075e734de260a09789d282f91d64bbc8 Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Fri, 9 Oct 2026 12:27:05 +0200 Subject: [PATCH 2/4] fixed changelog link --- .nextchanges/bundles/immutable-folder-paths.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.nextchanges/bundles/immutable-folder-paths.md b/.nextchanges/bundles/immutable-folder-paths.md index 72571e42502..af701d275de 100644 --- a/.nextchanges/bundles/immutable-folder-paths.md +++ b/.nextchanges/bundles/immutable-folder-paths.md @@ -1 +1 @@ -* Fixed `experimental.immutable_folder` failing with `experimental.deployment_history` ("workspace_info.file_path must be an absolute workspace path") and failing `bundle validate` when top-level `permissions` are set. +* Fixed `experimental.immutable_folder` failing with `experimental.deployment_history` ("workspace_info.file_path must be an absolute workspace path") and failing `bundle validate` when top-level `permissions` are set. ([#7001](https://github.com/databricks/cli/pull/7001)) From 9200508816149378b706d8754c42c5b72cb5304d Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Fri, 9 Oct 2026 13:33:02 +0200 Subject: [PATCH 3/4] fixed tests --- .../bundle/variables/resolve-builtin/databricks.yml | 2 +- acceptance/bundle/variables/resolve-builtin/output.txt | 10 +++++----- .../variables/resolve-vars-in-root-path/databricks.yml | 2 +- .../variables/resolve-vars-in-root-path/output.txt | 10 +++++----- 4 files changed, 12 insertions(+), 12 deletions(-) diff --git a/acceptance/bundle/variables/resolve-builtin/databricks.yml b/acceptance/bundle/variables/resolve-builtin/databricks.yml index 4bb71c8db44..c358df84fe2 100644 --- a/acceptance/bundle/variables/resolve-builtin/databricks.yml +++ b/acceptance/bundle/variables/resolve-builtin/databricks.yml @@ -2,5 +2,5 @@ bundle: name: TestResolveVariableReferences workspace: - root_path: "${bundle.name}/bar" + root_path: "/${bundle.name}/bar" file_path: "${workspace.root_path}/baz" diff --git a/acceptance/bundle/variables/resolve-builtin/output.txt b/acceptance/bundle/variables/resolve-builtin/output.txt index f37a2a19e39..769decea0ad 100644 --- a/acceptance/bundle/variables/resolve-builtin/output.txt +++ b/acceptance/bundle/variables/resolve-builtin/output.txt @@ -1,7 +1,7 @@ { - "artifact_path": "TestResolveVariableReferences/bar/artifacts", - "file_path": "TestResolveVariableReferences/bar/baz", - "resource_path": "TestResolveVariableReferences/bar/resources", - "root_path": "TestResolveVariableReferences/bar", - "state_path": "TestResolveVariableReferences/bar/state" + "artifact_path": "/Workspace/TestResolveVariableReferences/bar/artifacts", + "file_path": "/Workspace/TestResolveVariableReferences/bar/baz", + "resource_path": "/Workspace/TestResolveVariableReferences/bar/resources", + "root_path": "/Workspace/TestResolveVariableReferences/bar", + "state_path": "/Workspace/TestResolveVariableReferences/bar/state" } diff --git a/acceptance/bundle/variables/resolve-vars-in-root-path/databricks.yml b/acceptance/bundle/variables/resolve-vars-in-root-path/databricks.yml index 6a45de33076..b555742ddc6 100644 --- a/acceptance/bundle/variables/resolve-vars-in-root-path/databricks.yml +++ b/acceptance/bundle/variables/resolve-vars-in-root-path/databricks.yml @@ -2,7 +2,7 @@ bundle: name: TestResolveVariableReferencesToBundleVariables workspace: - root_path: "${bundle.name}/${var.foo}" + root_path: "/${bundle.name}/${var.foo}" variables: foo: diff --git a/acceptance/bundle/variables/resolve-vars-in-root-path/output.txt b/acceptance/bundle/variables/resolve-vars-in-root-path/output.txt index fb828d82678..fd25be92f5c 100644 --- a/acceptance/bundle/variables/resolve-vars-in-root-path/output.txt +++ b/acceptance/bundle/variables/resolve-vars-in-root-path/output.txt @@ -1,7 +1,7 @@ { - "artifact_path": "TestResolveVariableReferencesToBundleVariables/bar/artifacts", - "file_path": "TestResolveVariableReferencesToBundleVariables/bar/files", - "resource_path": "TestResolveVariableReferencesToBundleVariables/bar/resources", - "root_path": "TestResolveVariableReferencesToBundleVariables/bar", - "state_path": "TestResolveVariableReferencesToBundleVariables/bar/state" + "artifact_path": "/Workspace/TestResolveVariableReferencesToBundleVariables/bar/artifacts", + "file_path": "/Workspace/TestResolveVariableReferencesToBundleVariables/bar/files", + "resource_path": "/Workspace/TestResolveVariableReferencesToBundleVariables/bar/resources", + "root_path": "/Workspace/TestResolveVariableReferencesToBundleVariables/bar", + "state_path": "/Workspace/TestResolveVariableReferencesToBundleVariables/bar/state" } From 015df16d54668033f705a04510d358f20b3b82bb Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Mon, 12 Oct 2026 12:13:50 +0200 Subject: [PATCH 4/4] do not use reading from state --- bundle/deploy/snapshot/upload.go | 75 ++++++++++++------- bundle/deploy/snapshot/upload_warning_test.go | 33 +++++--- bundle/phases/dms.go | 36 ++------- 3 files changed, 77 insertions(+), 67 deletions(-) diff --git a/bundle/deploy/snapshot/upload.go b/bundle/deploy/snapshot/upload.go index df56e8fda5e..3b7d7d1fc1a 100644 --- a/bundle/deploy/snapshot/upload.go +++ b/bundle/deploy/snapshot/upload.go @@ -21,8 +21,8 @@ type snapshotUpload struct { // clean is true for a fresh plan or deploy: the mutator discards zips staged by a // previous run (so content-addressed ".zip" files don't accumulate) and // stages the current one. It is false when applying a pre-existing plan - // (deploy --plan): the plan already records the resource and the zip staged when it - // was produced, so the mutator reuses that staged file instead of rebuilding it. + // (deploy --plan): the mutator reuses the zip staged when the plan was produced + // instead of rebuilding it. clean bool } @@ -34,8 +34,9 @@ type PlanUploadOptions struct { // resource. When Clean is true it discards zips staged by a previous run, builds the // bundle zip, stages it under the bundle's local state directory as ".zip", and // records the path on the resource; the staged file is uploaded when the resource is -// created on apply. Pass Clean=false when applying a pre-existing plan: the plan -// already carries the resource and its staged zip, so the mutator reuses them. +// created on apply. Pass Clean=false when applying a pre-existing plan: the mutator reuses the +// zip staged when the plan was produced. Either way the resource is registered in the config, +// so its path is known to the rest of the deploy before the snapshot is uploaded. func PlanUpload(opts PlanUploadOptions) bundle.Mutator { return &snapshotUpload{clean: opts.Clean} } @@ -45,13 +46,6 @@ func (m *snapshotUpload) Name() string { } func (m *snapshotUpload) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { - // Applying a pre-existing plan: the plan already records the snapshot resource and - // the path of the zip staged when the plan was produced (InitForApply restores that - // state and DoCreate reads the staged file). Reuse it instead of rebuilding. - if !m.clean { - return nil - } - uploader, err := snapshot.NewSnapshotClient(b.WorkspaceClient(ctx)) if err != nil { return diag.FromErr(err) @@ -69,41 +63,68 @@ func (m *snapshotUpload) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagn return nil } + var zipPath string + var diags diag.Diagnostics + if m.clean { + zipPath, diags, err = stageZip(ctx, b) + } else { + zipPath, err = stagedZipPath(ctx, b) + } + if err != nil { + return diags.Extend(diag.FromErr(err)) + } + + b.Config.Resources.Snapshots[resources.SnapshotResourceKey] = &resources.Snapshot{ + BundleID: BundleID(b), + ACL: BuildACL(b), + RemoteRoot: remoteRoot, + ZipPath: filepath.ToSlash(zipPath), + } + return diags +} + +// stageZip builds the bundle zip and stages it under the bundle's local state directory as +// ".zip", discarding zips staged by a previous run so content-addressed files don't +// accumulate. +func stageZip(ctx context.Context, b *bundle.Bundle) (string, diag.Diagnostics, error) { zipContent, fileCount, err := BundleZip(ctx, b) if err != nil { - return diag.FromErr(fmt.Errorf("failed to build snapshot zip: %w", err)) + return "", nil, fmt.Errorf("failed to build snapshot zip: %w", err) } - // Discard zips staged by a previous run so content-addressed ".zip" files - // don't accumulate. if err := os.RemoveAll(b.GetLocalStateDir(ctx, "snapshots")); err != nil { - return diag.FromErr(fmt.Errorf("failed to clean snapshot dir: %w", err)) + return "", nil, fmt.Errorf("failed to clean snapshot dir: %w", err) } dir, err := b.LocalStateDir(ctx, "snapshots") if err != nil { - return diag.FromErr(err) + return "", nil, err } zipPath := filepath.Join(dir, snapshot.HashFromContent(zipContent)+".zip") if err := os.WriteFile(zipPath, zipContent, 0o600); err != nil { - return diag.FromErr(fmt.Errorf("failed to write snapshot zip: %w", err)) - } - - b.Config.Resources.Snapshots[resources.SnapshotResourceKey] = &resources.Snapshot{ - BundleID: BundleID(b), - ACL: BuildACL(b), - RemoteRoot: remoteRoot, - ZipPath: filepath.ToSlash(zipPath), + return "", nil, fmt.Errorf("failed to write snapshot zip: %w", err) } var diags diag.Diagnostics if fileCount > fileLimitWarning { - diags = append(diags, diag.Warningf( + diags = diag.Warningf( "immutable folder deployment may not work correctly: bundle contains %d files (limit is %d)", fileCount, fileLimitWarning, - )...) + ) } + return zipPath, diags, nil +} - return diags +// stagedZipPath returns the zip staged when the plan being applied was produced. Each fresh +// plan clears the directory before staging, so it holds exactly that one zip. +func stagedZipPath(ctx context.Context, b *bundle.Bundle) (string, error) { + zips, err := filepath.Glob(filepath.Join(b.GetLocalStateDir(ctx, "snapshots"), "*.zip")) + if err != nil { + return "", err + } + if len(zips) != 1 { + return "", fmt.Errorf("expected one staged snapshot zip, found %d; run 'bundle plan' again", len(zips)) + } + return zips[0], nil } // BundleID returns a stable hash that identifies the bundle deployment. diff --git a/bundle/deploy/snapshot/upload_warning_test.go b/bundle/deploy/snapshot/upload_warning_test.go index 8020405b211..85ff5a21b51 100644 --- a/bundle/deploy/snapshot/upload_warning_test.go +++ b/bundle/deploy/snapshot/upload_warning_test.go @@ -3,7 +3,6 @@ package snapshot import ( "context" "fmt" - "io/fs" "math" "os" "path/filepath" @@ -11,6 +10,7 @@ import ( "github.com/databricks/cli/bundle" "github.com/databricks/cli/bundle/config" + "github.com/databricks/cli/bundle/config/resources" "github.com/databricks/cli/libs/cmdio" "github.com/databricks/cli/libs/diag" "github.com/databricks/cli/libs/logdiag" @@ -91,18 +91,33 @@ func TestUploadNoWarningBelowFileLimit(t *testing.T) { } func TestUploadReusesStagedZipWhenNotClean(t *testing.T) { - // clean=false is the deploy --plan path: the plan already carries the resource and - // its staged zip, so Apply must not rebuild, stage, or register anything (and must - // not warn), even for a bundle that would otherwise exceed the file limit. + // clean=false is the deploy --plan path: the zip staged when the plan was produced is + // registered on the config as is, without rebuilding or warning again, even for a + // bundle that would otherwise exceed the file limit. ctx := testContext(t) b := makeBundle(t, fileLimitWarning+1) b.SetWorkpaceClient(setupTestClient(t)) - m := &snapshotUpload{clean: false} - diags := m.Apply(ctx, b) + require.Len(t, (&snapshotUpload{clean: true}).Apply(ctx, b), 1) + staged := b.Config.Resources.Snapshots[resources.SnapshotResourceKey] + require.NotNil(t, staged) + + b.Config.Resources.Snapshots = nil + diags := (&snapshotUpload{clean: false}).Apply(ctx, b) require.Empty(t, diags) - assert.Nil(t, b.Config.Resources.Snapshots) - _, err := os.Stat(b.GetLocalStateDir(ctx, "snapshots")) - assert.ErrorIs(t, err, fs.ErrNotExist, "no snapshots dir should be created") + reused := b.Config.Resources.Snapshots[resources.SnapshotResourceKey] + require.NotNil(t, reused) + assert.Equal(t, staged.ZipPath, reused.ZipPath) + assert.Equal(t, staged.FullPath(), reused.FullPath()) +} + +func TestUploadNotCleanWithoutStagedZip(t *testing.T) { + b := makeBundle(t, 5) + b.SetWorkpaceClient(setupTestClient(t)) + + diags := (&snapshotUpload{clean: false}).Apply(testContext(t), b) + + require.True(t, diags.HasError()) + assert.Contains(t, diags[0].Summary, "run 'bundle plan' again") } diff --git a/bundle/phases/dms.go b/bundle/phases/dms.go index 751c20db23f..abd89f03f2f 100644 --- a/bundle/phases/dms.go +++ b/bundle/phases/dms.go @@ -2,7 +2,6 @@ package phases import ( "context" - "encoding/json" "fmt" "net/url" "path" @@ -13,7 +12,6 @@ import ( "github.com/databricks/cli/bundle/config" "github.com/databricks/cli/bundle/config/resources" "github.com/databricks/cli/bundle/deployplan" - "github.com/databricks/cli/bundle/direct/dresources" "github.com/databricks/cli/internal/build" "github.com/databricks/cli/libs/cmdio" "github.com/databricks/cli/libs/dms" @@ -200,13 +198,13 @@ func deploymentMetadata(b *bundle.Bundle) (dms.Metadata, error) { FilePath: b.Config.Workspace.FilePath, } // With an immutable folder, file_path is a reference to the snapshot, which only - // resolves once the snapshot is uploaded. Its path is already known from the plan. + // resolves once the snapshot is uploaded. Its path is already known from the staged zip. if b.IsImmutableFolder() { - snapshotPath, err := immutableSnapshotPath(b) - if err != nil { - return dms.Metadata{}, err + snapshot, ok := b.Config.Resources.Snapshots[resources.SnapshotResourceKey] + if !ok { + return dms.Metadata{}, fmt.Errorf("no %s in the bundle", resources.SnapshotKey) } - ws.FilePath = path.Join(snapshotPath, "files") + ws.FilePath = path.Join(snapshot.FullPath(), "files") } // In a source-linked deployment files are not copied, so resources read them // from the sync root instead of file_path (see bundle/deploy/metadata.Compute). @@ -224,30 +222,6 @@ func deploymentMetadata(b *bundle.Bundle) (dms.Metadata, error) { return p, nil } -// immutableSnapshotPath returns the workspace path of the immutable folder snapshot. The -// snapshot is not uploaded yet at this point, but its content-addressed path is already known. -func immutableSnapshotPath(b *bundle.Bundle) (string, error) { - if sv, ok := b.DeploymentBundle.StateCache.Load(resources.SnapshotKey); ok { - state, ok := sv.Value.(*dresources.SnapshotState) - if !ok { - return "", fmt.Errorf("unexpected state type %T for %s", sv.Value, resources.SnapshotKey) - } - return state.FullPath, nil - } - - // An unchanged snapshot has no planned state when deploying from a saved plan. Its - // recorded state is what the plan would have held. - entry, ok := b.DeploymentBundle.StateDB.GetResourceEntry(resources.SnapshotKey) - if !ok { - return "", fmt.Errorf("no state for %s", resources.SnapshotKey) - } - var state dresources.SnapshotState - if err := json.Unmarshal(entry.State, &state); err != nil { - return "", fmt.Errorf("failed to read state of %s: %w", resources.SnapshotKey, err) - } - return state.FullPath, nil -} - // deploymentModeToSDK maps the bundle target's mode to the DMS enum. An unset mode // maps to empty, which the service reads as "not reported". func deploymentModeToSDK(mode config.Mode) bundledeployments.DeploymentMode {