From eeb68e3a8dd06a32519058ef2acf97f78f9875aa Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Thu, 16 Jul 2026 11:12:43 +0000 Subject: [PATCH] Add explicit read-only validation of the remote sync path bundle validate currently checks that workspace.file_path is a usable remote destination only as a side effect of building a file syncer: FilesToSync -> files.GetSync -> sync.New, whose constructor calls EnsureRemotePathIsUsable. That call also creates the directory as a side effect, which is undesirable from a read-only validator. An upcoming refactor centralizes the sync file-list computation and stops constructing a syncer during validation, which would silently drop this destination check. Extract it into an explicit, read-only ValidateRemoteFilePath validator that calls EnsureRemotePathIsUsable with dryRun=true, so the path is asserted to be a directory/repo without creating it. Wire it into the slow Validate() set. sync.New is intentionally left untouched, so bundle deploy and the sync commands keep creating the remote directory as before. During this transition bundle validate reports the destination error twice (once from this validator, once from FilesToSync via sync.New); a follow-up PR removes the check from sync.New by moving it to the syncer launch path. Co-authored-by: Isaac --- .../validate/remote_file_path/databricks.yml | 11 ++ .../validate/remote_file_path/out.test.toml | 5 + .../validate/remote_file_path/output.txt | 20 ++++ .../bundle/validate/remote_file_path/script | 15 +++ .../validate/remote_file_path/test.toml | 2 + bundle/config/validate/validate.go | 1 + .../validate/validate_remote_file_path.go | 55 +++++++++ .../validate_remote_file_path_test.go | 111 ++++++++++++++++++ 8 files changed, 220 insertions(+) create mode 100644 acceptance/bundle/validate/remote_file_path/databricks.yml create mode 100644 acceptance/bundle/validate/remote_file_path/out.test.toml create mode 100644 acceptance/bundle/validate/remote_file_path/output.txt create mode 100644 acceptance/bundle/validate/remote_file_path/script create mode 100644 acceptance/bundle/validate/remote_file_path/test.toml create mode 100644 bundle/config/validate/validate_remote_file_path.go create mode 100644 bundle/config/validate/validate_remote_file_path_test.go diff --git a/acceptance/bundle/validate/remote_file_path/databricks.yml b/acceptance/bundle/validate/remote_file_path/databricks.yml new file mode 100644 index 00000000000..6de3f4f05e2 --- /dev/null +++ b/acceptance/bundle/validate/remote_file_path/databricks.yml @@ -0,0 +1,11 @@ +bundle: + name: remote_file_path + +targets: + ok: + default: true + workspace: + file_path: /Workspace/tmp/remote_file_path/dir + bad: + workspace: + file_path: /Workspace/tmp/remote_file_path/file diff --git a/acceptance/bundle/validate/remote_file_path/out.test.toml b/acceptance/bundle/validate/remote_file_path/out.test.toml new file mode 100644 index 00000000000..22701b52f25 --- /dev/null +++ b/acceptance/bundle/validate/remote_file_path/out.test.toml @@ -0,0 +1,5 @@ +Local = true +Cloud = false +GOOSOnPR.darwin = false +GOOSOnPR.windows = false +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = [] diff --git a/acceptance/bundle/validate/remote_file_path/output.txt b/acceptance/bundle/validate/remote_file_path/output.txt new file mode 100644 index 00000000000..2c494f94d81 --- /dev/null +++ b/acceptance/bundle/validate/remote_file_path/output.txt @@ -0,0 +1,20 @@ + +>>> [CLI] workspace mkdirs /Workspace/tmp/remote_file_path/dir + +>>> [CLI] workspace import /Workspace/tmp/remote_file_path/file --format AUTO --file ./databricks.yml + +=== file_path is a directory: valid destination + +>>> [CLI] bundle validate -t ok +Name: remote_file_path +Target: ok +Workspace: + User: [USERNAME] + Path: /Workspace/Users/[USERNAME]/.bundle/remote_file_path/ok + +Validation OK! + +=== file_path is a file: invalid destination +Error: /Workspace/tmp/remote_file_path/file points to a file +Error: /Workspace/tmp/remote_file_path/file points to a file +Found 2 errors diff --git a/acceptance/bundle/validate/remote_file_path/script b/acceptance/bundle/validate/remote_file_path/script new file mode 100644 index 00000000000..5ceb9d90712 --- /dev/null +++ b/acceptance/bundle/validate/remote_file_path/script @@ -0,0 +1,15 @@ +# Git Bash on Windows rewrites leading-slash paths; keep them verbatim. +export MSYS_NO_PATHCONV=1 + +trace $CLI workspace mkdirs /Workspace/tmp/remote_file_path/dir +trace $CLI workspace import /Workspace/tmp/remote_file_path/file --format AUTO --file ./databricks.yml + +title "file_path is a directory: valid destination\n" +trace $CLI bundle validate -t ok + +# validate's slow validators run in parallel and stream diagnostics in +# nondeterministic order, so sort a projection (full output in LOG.validate). The +# destination error appears twice for now: from this validator and from FilesToSync. +title "file_path is a file: invalid destination\n" +errcode $CLI bundle validate -t bad > LOG.validate 2>&1 +grep -E "^(Error|Warning|Found)" LOG.validate | sort diff --git a/acceptance/bundle/validate/remote_file_path/test.toml b/acceptance/bundle/validate/remote_file_path/test.toml new file mode 100644 index 00000000000..3043bc6d17a --- /dev/null +++ b/acceptance/bundle/validate/remote_file_path/test.toml @@ -0,0 +1,2 @@ +# This test does not deploy, so it is engine-independent. +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = [] diff --git a/bundle/config/validate/validate.go b/bundle/config/validate/validate.go index e531d87684c..640823ee12b 100644 --- a/bundle/config/validate/validate.go +++ b/bundle/config/validate/validate.go @@ -13,6 +13,7 @@ func Validate(ctx context.Context, b *bundle.Bundle) { // Slow mutators that require network or file i/o. These are only // run in the `bundle validate` command. FilesToSync(), + ValidateRemoteFilePath(), ValidateFolderPermissions(), ValidateSyncPatterns(), ) diff --git a/bundle/config/validate/validate_remote_file_path.go b/bundle/config/validate/validate_remote_file_path.go new file mode 100644 index 00000000000..6abe9e60efa --- /dev/null +++ b/bundle/config/validate/validate_remote_file_path.go @@ -0,0 +1,55 @@ +package validate + +import ( + "context" + + "github.com/databricks/cli/bundle" + "github.com/databricks/cli/libs/diag" + "github.com/databricks/cli/libs/dyn" + "github.com/databricks/cli/libs/sync" + "github.com/databricks/databricks-sdk-go/service/iam" +) + +type validateRemoteFilePath struct{ bundle.RO } + +func (v *validateRemoteFilePath) Name() string { + return "validate:remote_file_path" +} + +func (v *validateRemoteFilePath) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { + // Under immutable_folder, file_path is not a sync destination: it is replaced + // with the content-addressed snapshot path at deploy time. + if b.IsImmutableFolder() { + return nil + } + + // Nothing to check if the user opted out of syncing. Mirrors FilesToSync. + if len(b.Config.Sync.Paths) == 0 || b.Config.Workspace.FilePath == "" { + return nil + } + + var me *iam.User + if b.Config.Workspace.CurrentUser != nil { + me = b.Config.Workspace.CurrentUser.User + } + + // dryRun=true: assert the destination is a usable directory/repo without + // creating it. Deploy still creates it via sync.New. + err := sync.EnsureRemotePathIsUsable(ctx, b.WorkspaceClient(ctx), b.Config.Workspace.FilePath, me, true) + if err != nil { + return diag.Diagnostics{ + diag.Diagnostic{ + Severity: diag.Error, + Summary: err.Error(), + Locations: b.Config.GetLocations("workspace.file_path"), + Paths: []dyn.Path{dyn.MustPathFromString("workspace.file_path")}, + }, + } + } + + return nil +} + +func ValidateRemoteFilePath() bundle.ReadOnlyMutator { + return &validateRemoteFilePath{} +} diff --git a/bundle/config/validate/validate_remote_file_path_test.go b/bundle/config/validate/validate_remote_file_path_test.go new file mode 100644 index 00000000000..8ab167ceb5e --- /dev/null +++ b/bundle/config/validate/validate_remote_file_path_test.go @@ -0,0 +1,111 @@ +package validate + +import ( + "testing" + + "github.com/databricks/cli/bundle" + "github.com/databricks/cli/bundle/config" + "github.com/databricks/cli/libs/diag" + sdkconfig "github.com/databricks/databricks-sdk-go/config" + "github.com/databricks/databricks-sdk-go/experimental/mocks" + "github.com/databricks/databricks-sdk-go/service/iam" + "github.com/databricks/databricks-sdk-go/service/workspace" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +const testRemoteFilePath = "/Workspace/Users/me/.bundle/files" + +func setupBundleForRemoteFilePathTest(t *testing.T, objectType workspace.ObjectType) *bundle.Bundle { + b := &bundle.Bundle{ + Config: config.Root{ + Workspace: config.Workspace{ + FilePath: testRemoteFilePath, + CurrentUser: &config.User{ + User: &iam.User{UserName: "me@example.com"}, + }, + }, + Sync: config.Sync{ + Paths: []string{"."}, + }, + }, + } + + m := mocks.NewMockWorkspaceClient(t) + m.WorkspaceClient.Config = &sdkconfig.Config{ + Host: "https://foo.test", + } + + // No MkdirsByPath expectation: the dry-run check must not mutate remote state, + // so an unexpected mkdir call fails the test. + m.GetMockWorkspaceAPI().EXPECT().GetStatusByPath(mock.Anything, testRemoteFilePath).Return(&workspace.ObjectInfo{ + ObjectType: objectType, + Path: testRemoteFilePath, + }, nil) + + b.SetWorkpaceClient(m.WorkspaceClient) + return b +} + +func TestValidateRemoteFilePath_Directory(t *testing.T) { + b := setupBundleForRemoteFilePathTest(t, workspace.ObjectTypeDirectory) + + ctx := t.Context() + diags := ValidateRemoteFilePath().Apply(ctx, b) + assert.Empty(t, diags) +} + +func TestValidateRemoteFilePath_NotADirectory(t *testing.T) { + b := setupBundleForRemoteFilePathTest(t, workspace.ObjectTypeNotebook) + + ctx := t.Context() + diags := ValidateRemoteFilePath().Apply(ctx, b) + require.Len(t, diags, 1) + assert.Equal(t, diag.Error, diags[0].Severity) + assert.Contains(t, diags[0].Summary, testRemoteFilePath) +} + +func TestValidateRemoteFilePath_NoSyncPaths(t *testing.T) { + // No sync paths: nothing to check, so no workspace client is even set. + b := &bundle.Bundle{ + Config: config.Root{ + Workspace: config.Workspace{ + FilePath: testRemoteFilePath, + }, + Sync: config.Sync{ + Paths: []string{}, + }, + }, + } + + ctx := t.Context() + diags := ValidateRemoteFilePath().Apply(ctx, b) + assert.Empty(t, diags) +} + +func TestValidateRemoteFilePath_ImmutableFolder(t *testing.T) { + // Under immutable_folder, file_path is not a sync destination, so it is not checked. + b := &bundle.Bundle{ + Config: config.Root{ + Workspace: config.Workspace{ + FilePath: testRemoteFilePath, + }, + Sync: config.Sync{ + Paths: []string{"."}, + }, + Experimental: &config.Experimental{ + ImmutableFolder: true, + }, + }, + } + + // Inject a mock client with no expectations: any workspace call (which would + // happen if the check were not skipped) fails the test. + m := mocks.NewMockWorkspaceClient(t) + b.SetWorkpaceClient(m.WorkspaceClient) + + ctx := t.Context() + diags := ValidateRemoteFilePath().Apply(ctx, b) + assert.Empty(t, diags) +}