Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Default to forward slash-separated paths for path translation #2145

Merged
merged 7 commits into from
Jan 17, 2025
Merged

Default to forward slash-separated paths for path translation #2145

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c814fd8
Default to forward slash separated paths during path translation
pietern Jan 14, 2025
751b480
Update test
pietern Jan 14, 2025
e5df30f
Merge remote-tracking branch 'origin/main' into path-translation-forw…
pietern Jan 16, 2025
4dcdf74
Always translate into forward slash separated paths
pietern Jan 16, 2025
53fb805
Update test
pietern Jan 16, 2025
046ad48
Mistake
pietern Jan 16, 2025
3a29719
Comments
pietern Jan 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 23 additions & 17 deletions bundle/config/mutator/translate_paths.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
pietern marked this conversation as resolved.
Show resolved Hide resolved
localRelPath, err := filepath.Rel(t.b.SyncRootPath, localPath)
if err != nil {
return "", err
Expand All @@ -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 {
Expand Down Expand Up @@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need to perform similar clean up across the whole codebase, or at least in bundle?

(main) ~/work/cli % git grep filepath bundle | wc -l
     507

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a few other places where we do the same (e.g. sync includes), but definitely not everywhere.

The filepath package must be used for relative path computations because on Windows it makes sure it doesn't escape the drive or UNC path. I recall that also for globbing the input paths need to use the platform-native separator or they don't work.

return "", fmt.Errorf("notebook %s not found", literal)
}

Expand All @@ -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
Expand All @@ -218,42 +224,42 @@ 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)
Expand All @@ -262,12 +268,12 @@ func (t *translateContext) translateLocalAbsoluteFilePath(ctx context.Context, l
}

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)
Expand All @@ -281,7 +287,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
}
Expand Down
4 changes: 2 additions & 2 deletions bundle/config/mutator/translate_paths_artifacts_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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)
}
2 changes: 1 addition & 1 deletion bundle/config/mutator/translate_paths_dashboards_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
)
}
27 changes: 13 additions & 14 deletions bundle/config/mutator/translate_paths_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import (
"os"
"path/filepath"
"runtime"
"strings"
"testing"

"github.com/databricks/cli/bundle"
Expand Down Expand Up @@ -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(
Expand All @@ -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(
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question - might not be in scope for this PR - sometimes the paths have ./ in front ("./job/dist/env1.whl"), sometimes they don't ("job/dist/task.jar").

Should we standardize on that? Why not drop "./" from everywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I recall correctly, this has its roots in the need to differentiate between a path reference and a PyPI package.

Agree that we should revisit and see if it can be normalized.

@andrewnester Do you recall the specifics?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Originally I think that was the reason but now I think we can safely drop it

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/[email protected]/test.whl", b.Config.Resources.Jobs["job"].JobSettings.Environments[0].Spec.Dependencies[3])
assert.Equal(t, "--extra-index-url https://name:[email protected]/api/v4/projects/9876/packages/pypi/simple foobar", b.Config.Resources.Jobs["job"].JobSettings.Environments[0].Spec.Dependencies[4])
Expand Down Expand Up @@ -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,
)
}
Expand Down Expand Up @@ -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(
Expand All @@ -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(
Expand Down
8 changes: 4 additions & 4 deletions bundle/tests/relative_path_with_includes_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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)
}
Loading