From c814fd8c5cd3b9e98a44be401b2f2be4a280e7c3 Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Tue, 14 Jan 2025 18:28:50 +0100 Subject: [PATCH 1/6] Default to forward slash separated paths during path translation --- bundle/config/mutator/translate_paths.go | 48 +++++++++++-------- bundle/config/mutator/translate_paths_test.go | 27 +++++------ 2 files changed, 42 insertions(+), 33 deletions(-) diff --git a/bundle/config/mutator/translate_paths.go b/bundle/config/mutator/translate_paths.go index a2c830be36..a609a25a4c 100644 --- a/bundle/config/mutator/translate_paths.go +++ b/bundle/config/mutator/translate_paths.go @@ -136,13 +136,15 @@ func (t *translateContext) rewritePath( } // Local path is relative to the directory the resource was defined in. - localPath := filepath.Join(dir, filepath.FromSlash(input)) + // Note: the initialized variable is a platform-native path. + localPath := filepath.Join(dir, input) if interp, ok := t.seen[localPath]; ok { return interp, nil } // Local path must be contained in the sync root. // If it isn't, it won't be synchronized into the workspace. + // Note: the initialized variable is a platform-native path. localRelPath, err := filepath.Rel(t.b.SyncRootPath, localPath) if err != nil { return "", err @@ -151,6 +153,10 @@ func (t *translateContext) rewritePath( return "", fmt.Errorf("path %s is not contained in sync root path", localPath) } + // Normalize paths we pass to the translation functions to use forward slashes. + localPath = filepath.ToSlash(localPath) + localRelPath = filepath.ToSlash(localRelPath) + // Convert local path into workspace path via specified function. var interp string switch opts.Mode { @@ -180,9 +186,9 @@ func (t *translateContext) rewritePath( } func (t *translateContext) translateNotebookPath(ctx context.Context, literal, localFullPath, localRelPath string) (string, error) { - nb, _, err := notebook.DetectWithFS(t.b.SyncRoot, filepath.ToSlash(localRelPath)) + nb, _, err := notebook.DetectWithFS(t.b.SyncRoot, localRelPath) if errors.Is(err, fs.ErrNotExist) { - if filepath.Ext(localFullPath) != notebook.ExtensionNone { + if path.Ext(localFullPath) != notebook.ExtensionNone { return "", fmt.Errorf("notebook %s not found", literal) } @@ -198,7 +204,7 @@ func (t *translateContext) translateNotebookPath(ctx context.Context, literal, l // way we can provide a more targeted error message. for _, ext := range extensions { literalWithExt := literal + ext - localRelPathWithExt := filepath.ToSlash(localRelPath + ext) + localRelPathWithExt := localRelPath + ext if _, err := fs.Stat(t.b.SyncRoot, localRelPathWithExt); err == nil { return "", fmt.Errorf(`notebook %s not found. Did you mean %s? Local notebook references are expected to contain one of the following @@ -218,61 +224,65 @@ to contain one of the following file extensions: [%s]`, literal, strings.Join(ex } // Upon import, notebooks are stripped of their extension. - localRelPathNoExt := strings.TrimSuffix(localRelPath, filepath.Ext(localRelPath)) - return path.Join(t.remoteRoot, filepath.ToSlash(localRelPathNoExt)), nil + localRelPathNoExt := strings.TrimSuffix(localRelPath, path.Ext(localRelPath)) + return path.Join(t.remoteRoot, localRelPathNoExt), nil } func (t *translateContext) translateFilePath(ctx context.Context, literal, localFullPath, localRelPath string) (string, error) { - nb, _, err := notebook.DetectWithFS(t.b.SyncRoot, filepath.ToSlash(localRelPath)) + nb, _, err := notebook.DetectWithFS(t.b.SyncRoot, localRelPath) if errors.Is(err, fs.ErrNotExist) { return "", fmt.Errorf("file %s not found", literal) } if err != nil { - return "", fmt.Errorf("unable to determine if %s is not a notebook: %w", localFullPath, err) + return "", fmt.Errorf("unable to determine if %s is not a notebook: %w", filepath.FromSlash(localFullPath), err) } if nb { return "", ErrIsNotebook{localFullPath} } - return path.Join(t.remoteRoot, filepath.ToSlash(localRelPath)), nil + return path.Join(t.remoteRoot, localRelPath), nil } func (t *translateContext) translateDirectoryPath(ctx context.Context, literal, localFullPath, localRelPath string) (string, error) { - info, err := t.b.SyncRoot.Stat(filepath.ToSlash(localRelPath)) + info, err := t.b.SyncRoot.Stat(localRelPath) if err != nil { return "", err } if !info.IsDir() { - return "", fmt.Errorf("%s is not a directory", localFullPath) + return "", fmt.Errorf("%s is not a directory", filepath.FromSlash(localFullPath)) } - return path.Join(t.remoteRoot, filepath.ToSlash(localRelPath)), nil + return path.Join(t.remoteRoot, localRelPath), nil } func (t *translateContext) translateLocalAbsoluteFilePath(ctx context.Context, literal, localFullPath, localRelPath string) (string, error) { - info, err := t.b.SyncRoot.Stat(filepath.ToSlash(localRelPath)) + info, err := t.b.SyncRoot.Stat(localRelPath) if errors.Is(err, fs.ErrNotExist) { return "", fmt.Errorf("file %s not found", literal) } if err != nil { - return "", fmt.Errorf("unable to determine if %s is a file: %w", localFullPath, err) + return "", fmt.Errorf("unable to determine if %s is a file: %w", filepath.FromSlash(localFullPath), err) } if info.IsDir() { return "", fmt.Errorf("expected %s to be a file but found a directory", literal) } - return localFullPath, nil + // Absolute paths are never externalized and always intended to be used locally. + // Return a platform-native path. + return filepath.FromSlash(localFullPath), nil } func (t *translateContext) translateLocalAbsoluteDirectoryPath(ctx context.Context, literal, localFullPath, _ string) (string, error) { - info, err := os.Stat(localFullPath) + info, err := os.Stat(filepath.FromSlash(localFullPath)) if errors.Is(err, fs.ErrNotExist) { return "", fmt.Errorf("directory %s not found", literal) } if err != nil { - return "", fmt.Errorf("unable to determine if %s is a directory: %w", localFullPath, err) + return "", fmt.Errorf("unable to determine if %s is a directory: %w", filepath.FromSlash(localFullPath), err) } if !info.IsDir() { return "", fmt.Errorf("expected %s to be a directory but found a file", literal) } - return localFullPath, nil + // Absolute paths are never externalized and always intended to be used locally. + // Return a platform-native path. + return filepath.FromSlash(localFullPath), nil } func (t *translateContext) translateLocalRelativePath(ctx context.Context, literal, localFullPath, localRelPath string) (string, error) { @@ -281,7 +291,7 @@ func (t *translateContext) translateLocalRelativePath(ctx context.Context, liter func (t *translateContext) translateLocalRelativeWithPrefixPath(ctx context.Context, literal, localFullPath, localRelPath string) (string, error) { if !strings.HasPrefix(localRelPath, ".") { - localRelPath = "." + string(filepath.Separator) + localRelPath + localRelPath = "./" + localRelPath } return localRelPath, nil } diff --git a/bundle/config/mutator/translate_paths_test.go b/bundle/config/mutator/translate_paths_test.go index 493abb8c5f..aa6488ab09 100644 --- a/bundle/config/mutator/translate_paths_test.go +++ b/bundle/config/mutator/translate_paths_test.go @@ -6,7 +6,6 @@ import ( "os" "path/filepath" "runtime" - "strings" "testing" "github.com/databricks/cli/bundle" @@ -226,7 +225,7 @@ func TestTranslatePaths(t *testing.T) { ) assert.Equal( t, - filepath.Join("dist", "task.whl"), + "dist/task.whl", b.Config.Resources.Jobs["job"].Tasks[0].Libraries[0].Whl, ) assert.Equal( @@ -251,7 +250,7 @@ func TestTranslatePaths(t *testing.T) { ) assert.Equal( t, - filepath.Join("dist", "task.jar"), + "dist/task.jar", b.Config.Resources.Jobs["job"].Tasks[5].Libraries[0].Jar, ) assert.Equal( @@ -362,7 +361,7 @@ func TestTranslatePathsInSubdirectories(t *testing.T) { ) assert.Equal( t, - filepath.Join("job", "dist", "task.jar"), + "job/dist/task.jar", b.Config.Resources.Jobs["job"].Tasks[1].Libraries[0].Jar, ) assert.Equal( @@ -774,8 +773,8 @@ func TestTranslatePathJobEnvironments(t *testing.T) { diags := bundle.Apply(context.Background(), b, mutator.TranslatePaths()) require.NoError(t, diags.Error()) - assert.Equal(t, strings.Join([]string{".", "job", "dist", "env1.whl"}, string(os.PathSeparator)), b.Config.Resources.Jobs["job"].JobSettings.Environments[0].Spec.Dependencies[0]) - assert.Equal(t, strings.Join([]string{".", "dist", "env2.whl"}, string(os.PathSeparator)), b.Config.Resources.Jobs["job"].JobSettings.Environments[0].Spec.Dependencies[1]) + assert.Equal(t, "./job/dist/env1.whl", b.Config.Resources.Jobs["job"].JobSettings.Environments[0].Spec.Dependencies[0]) + assert.Equal(t, "./dist/env2.whl", b.Config.Resources.Jobs["job"].JobSettings.Environments[0].Spec.Dependencies[1]) assert.Equal(t, "simplejson", b.Config.Resources.Jobs["job"].JobSettings.Environments[0].Spec.Dependencies[2]) assert.Equal(t, "/Workspace/Users/foo@bar.com/test.whl", b.Config.Resources.Jobs["job"].JobSettings.Environments[0].Spec.Dependencies[3]) assert.Equal(t, "--extra-index-url https://name:token@gitlab.com/api/v4/projects/9876/packages/pypi/simple foobar", b.Config.Resources.Jobs["job"].JobSettings.Environments[0].Spec.Dependencies[4]) @@ -839,7 +838,7 @@ func TestTranslatePathWithComplexVariables(t *testing.T) { assert.Equal( t, - filepath.Join("variables", "local", "whl.whl"), + "variables/local/whl.whl", b.Config.Resources.Jobs["job"].Tasks[0].Libraries[0].Whl, ) } @@ -952,34 +951,34 @@ func TestTranslatePathsWithSourceLinkedDeployment(t *testing.T) { // updated to source path assert.Equal( t, - filepath.Join(dir, "my_job_notebook"), + dir+"/my_job_notebook", b.Config.Resources.Jobs["job"].Tasks[0].NotebookTask.NotebookPath, ) assert.Equal( t, - filepath.Join(dir, "requirements.txt"), + dir+"/requirements.txt", b.Config.Resources.Jobs["job"].Tasks[2].Libraries[0].Requirements, ) assert.Equal( t, - filepath.Join(dir, "my_python_file.py"), + dir+"/my_python_file.py", b.Config.Resources.Jobs["job"].Tasks[3].SparkPythonTask.PythonFile, ) assert.Equal( t, - filepath.Join(dir, "my_pipeline_notebook"), + dir+"/my_pipeline_notebook", b.Config.Resources.Pipelines["pipeline"].Libraries[0].Notebook.Path, ) assert.Equal( t, - filepath.Join(dir, "my_python_file.py"), + dir+"/my_python_file.py", b.Config.Resources.Pipelines["pipeline"].Libraries[2].File.Path, ) // left as is assert.Equal( t, - filepath.Join("dist", "task.whl"), + "dist/task.whl", b.Config.Resources.Jobs["job"].Tasks[0].Libraries[0].Whl, ) assert.Equal( @@ -989,7 +988,7 @@ func TestTranslatePathsWithSourceLinkedDeployment(t *testing.T) { ) assert.Equal( t, - filepath.Join("dist", "task.jar"), + "dist/task.jar", b.Config.Resources.Jobs["job"].Tasks[4].Libraries[0].Jar, ) assert.Equal( From 751b4808c9a48de12e91ae0d06f267dad5867269 Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Tue, 14 Jan 2025 18:40:24 +0100 Subject: [PATCH 2/6] Update test --- bundle/tests/relative_path_with_includes_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bundle/tests/relative_path_with_includes_test.go b/bundle/tests/relative_path_with_includes_test.go index 8efac00394..c3966a3ae7 100644 --- a/bundle/tests/relative_path_with_includes_test.go +++ b/bundle/tests/relative_path_with_includes_test.go @@ -37,6 +37,6 @@ func TestRelativePathsWithIncludes(t *testing.T) { b.Config.Sync.Exclude, ) - assert.Equal(t, filepath.Join("dist", "job_a.whl"), b.Config.Resources.Jobs["job_a"].Tasks[0].Libraries[0].Whl) - assert.Equal(t, filepath.Join("subfolder", "dist", "job_b.whl"), b.Config.Resources.Jobs["job_b"].Tasks[0].Libraries[0].Whl) + assert.Equal(t, "dist/job_a.whl", b.Config.Resources.Jobs["job_a"].Tasks[0].Libraries[0].Whl) + assert.Equal(t, "subfolder/dist/job_b.whl", b.Config.Resources.Jobs["job_b"].Tasks[0].Libraries[0].Whl) } From 4dcdf7412d92d0fc86cb62cb586a2feb61d05e94 Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Thu, 16 Jan 2025 10:26:01 +0100 Subject: [PATCH 3/6] Always translate into forward slash separated paths --- bundle/config/mutator/translate_paths.go | 8 ++------ bundle/config/mutator/translate_paths_artifacts_test.go | 4 ++-- bundle/config/mutator/translate_paths_dashboards_test.go | 2 +- 3 files changed, 5 insertions(+), 9 deletions(-) diff --git a/bundle/config/mutator/translate_paths.go b/bundle/config/mutator/translate_paths.go index a609a25a4c..3929011e3f 100644 --- a/bundle/config/mutator/translate_paths.go +++ b/bundle/config/mutator/translate_paths.go @@ -264,9 +264,7 @@ func (t *translateContext) translateLocalAbsoluteFilePath(ctx context.Context, l if info.IsDir() { return "", fmt.Errorf("expected %s to be a file but found a directory", literal) } - // Absolute paths are never externalized and always intended to be used locally. - // Return a platform-native path. - return filepath.FromSlash(localFullPath), nil + return localFullPath, nil } func (t *translateContext) translateLocalAbsoluteDirectoryPath(ctx context.Context, literal, localFullPath, _ string) (string, error) { @@ -280,9 +278,7 @@ func (t *translateContext) translateLocalAbsoluteDirectoryPath(ctx context.Conte if !info.IsDir() { return "", fmt.Errorf("expected %s to be a directory but found a file", literal) } - // Absolute paths are never externalized and always intended to be used locally. - // Return a platform-native path. - return filepath.FromSlash(localFullPath), nil + return localFullPath, nil } func (t *translateContext) translateLocalRelativePath(ctx context.Context, literal, localFullPath, localRelPath string) (string, error) { diff --git a/bundle/config/mutator/translate_paths_artifacts_test.go b/bundle/config/mutator/translate_paths_artifacts_test.go index fb402b488e..0d1af61569 100644 --- a/bundle/config/mutator/translate_paths_artifacts_test.go +++ b/bundle/config/mutator/translate_paths_artifacts_test.go @@ -46,7 +46,7 @@ func TestTranslatePathsArtifacts_InsideSyncRoot(t *testing.T) { require.NoError(t, diags.Error()) // Assert that the artifact path has been converted to a local absolute path. - assert.Equal(t, lib, b.Config.Artifacts["my_artifact"].Path) + assert.Equal(t, filepath.ToSlash(lib), b.Config.Artifacts["my_artifact"].Path) } func TestTranslatePathsArtifacts_OutsideSyncRoot(t *testing.T) { @@ -79,5 +79,5 @@ func TestTranslatePathsArtifacts_OutsideSyncRoot(t *testing.T) { require.NoError(t, diags.Error()) // Assert that the artifact path has been converted to a local absolute path. - assert.Equal(t, lib, b.Config.Artifacts["my_artifact"].Path) + assert.Equal(t, filepath.ToSlash(lib), b.Config.Artifacts["my_artifact"].Path) } diff --git a/bundle/config/mutator/translate_paths_dashboards_test.go b/bundle/config/mutator/translate_paths_dashboards_test.go index 5e4e69f5d8..02fba92e0a 100644 --- a/bundle/config/mutator/translate_paths_dashboards_test.go +++ b/bundle/config/mutator/translate_paths_dashboards_test.go @@ -48,7 +48,7 @@ func TestTranslatePathsDashboards_FilePathRelativeSubDirectory(t *testing.T) { // Assert that the file path for the dashboard has been converted to its local absolute path. assert.Equal( t, - filepath.Join(dir, "src", "my_dashboard.lvdash.json"), + filepath.ToSlash(filepath.Join(dir, "src", "my_dashboard.lvdash.json")), b.Config.Resources.Dashboards["dashboard"].FilePath, ) } From 53fb8055319602dfd4808a0bfb6e344dd902d231 Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Thu, 16 Jan 2025 10:34:11 +0100 Subject: [PATCH 4/6] Update test --- bundle/tests/relative_path_with_includes_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bundle/tests/relative_path_with_includes_test.go b/bundle/tests/relative_path_with_includes_test.go index c3966a3ae7..c0d96b8240 100644 --- a/bundle/tests/relative_path_with_includes_test.go +++ b/bundle/tests/relative_path_with_includes_test.go @@ -17,8 +17,8 @@ func TestRelativePathsWithIncludes(t *testing.T) { diags := bundle.Apply(context.Background(), b, m) assert.NoError(t, diags.Error()) - assert.Equal(t, filepath.Join(b.SyncRootPath, "artifact_a"), b.Config.Artifacts["test_a"].Path) - assert.Equal(t, filepath.Join(b.SyncRootPath, "subfolder", "artifact_b"), b.Config.Artifacts["test_b"].Path) + assert.Equal(t, b.SyncRootPath+"artifact_a", b.Config.Artifacts["test_a"].Path) + assert.Equal(t, b.SyncRootPath+"subfolder/artifact_b", b.Config.Artifacts["test_b"].Path) assert.ElementsMatch( t, From 046ad4837f9b8a6dda09c555ebf654a2d4957515 Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Thu, 16 Jan 2025 10:38:18 +0100 Subject: [PATCH 5/6] Mistake --- bundle/tests/relative_path_with_includes_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bundle/tests/relative_path_with_includes_test.go b/bundle/tests/relative_path_with_includes_test.go index c0d96b8240..7249cac1f7 100644 --- a/bundle/tests/relative_path_with_includes_test.go +++ b/bundle/tests/relative_path_with_includes_test.go @@ -17,8 +17,8 @@ func TestRelativePathsWithIncludes(t *testing.T) { diags := bundle.Apply(context.Background(), b, m) assert.NoError(t, diags.Error()) - assert.Equal(t, b.SyncRootPath+"artifact_a", b.Config.Artifacts["test_a"].Path) - assert.Equal(t, b.SyncRootPath+"subfolder/artifact_b", b.Config.Artifacts["test_b"].Path) + assert.Equal(t, b.SyncRootPath+"/artifact_a", b.Config.Artifacts["test_a"].Path) + assert.Equal(t, b.SyncRootPath+"/subfolder/artifact_b", b.Config.Artifacts["test_b"].Path) assert.ElementsMatch( t, From 3a29719146514c18af4d430843e348c4867c60b9 Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Thu, 16 Jan 2025 10:42:09 +0100 Subject: [PATCH 6/6] Comments --- bundle/config/mutator/translate_paths.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/bundle/config/mutator/translate_paths.go b/bundle/config/mutator/translate_paths.go index 3929011e3f..1eda578fae 100644 --- a/bundle/config/mutator/translate_paths.go +++ b/bundle/config/mutator/translate_paths.go @@ -136,7 +136,6 @@ func (t *translateContext) rewritePath( } // Local path is relative to the directory the resource was defined in. - // Note: the initialized variable is a platform-native path. localPath := filepath.Join(dir, input) if interp, ok := t.seen[localPath]; ok { return interp, nil @@ -144,7 +143,6 @@ func (t *translateContext) rewritePath( // Local path must be contained in the sync root. // If it isn't, it won't be synchronized into the workspace. - // Note: the initialized variable is a platform-native path. localRelPath, err := filepath.Rel(t.b.SyncRootPath, localPath) if err != nil { return "", err @@ -153,7 +151,7 @@ func (t *translateContext) rewritePath( return "", fmt.Errorf("path %s is not contained in sync root path", localPath) } - // Normalize paths we pass to the translation functions to use forward slashes. + // Normalize paths to separated by forward slashes. localPath = filepath.ToSlash(localPath) localRelPath = filepath.ToSlash(localRelPath)