Skip to content

Commit dff97a0

Browse files
simonfaltumsimonfaltum
authored
Cover resource_path and all occurrences in workspace prefix rewrite (#5507)
## Why Found during a full-repo review of the CLI. The `RewriteWorkspacePrefix` mutator strips the legacy `/Workspace` prefix from strings that reference `${workspace.*}` path variables, but it had three gaps: 1. `${workspace.resource_path}` was missing from the rewrite table even though `PrependWorkspacePrefix` prefixes it, so a legacy `/Workspace/${workspace.resource_path}` reference silently interpolated to a doubled `/Workspace/Workspace/...` path. 2. It used `strings.Replace(..., 1)` and returned on the first match, so only the first occurrence of the first matching pattern was fixed; any further occurrences in the same string stayed broken. 3. Patterns were iterated in map order, so which pattern won (and which warning was emitted) was nondeterministic when a string matched more than one. ## Changes Before, a string was rewritten once for one nondeterministically chosen pattern and `resource_path` references were never rewritten; now every occurrence of every known pattern, including `resource_path`, is rewritten in a fixed order. In `bundle/config/mutator/rewrite_workspace_prefix.go`: - Added `/Workspace/${workspace.resource_path}` and `/Workspace${workspace.resource_path}` to the rewrite table, matching the path set handled by `PrependWorkspacePrefix`. - Replaced the map with an ordered slice of pattern/replacement pairs so rewrites and warnings are deterministic. - Switched to `strings.ReplaceAll` and continue through all patterns instead of returning after the first match. One warning is emitted per matched pattern; strings with a single match produce exactly the same warning as before. ## Test plan - [x] Extended `TestNoWorkspacePrefixUsed` with `resource_path` cases (both slash and no-slash variants), a multi-occurrence string, and a string matching two different patterns; verified warnings and rewritten values. - [x] `go test ./bundle/config/mutator` passes. - [x] `./task fmt-q`, `./task lint-q` (0 issues), and `./task checks` pass. This pull request and its description were written by Isaac.
1 parent b0b9394 commit dff97a0

2 files changed

Lines changed: 71 additions & 27 deletions

File tree

‎bundle/config/mutator/rewrite_workspace_prefix.go‎

Lines changed: 37 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -24,15 +24,24 @@ func (m *rewriteWorkspacePrefix) Name() string {
2424

2525
func (m *rewriteWorkspacePrefix) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics {
2626
diags := diag.Diagnostics{}
27-
paths := map[string]string{
28-
"/Workspace/${workspace.root_path}": "${workspace.root_path}",
29-
"/Workspace${workspace.root_path}": "${workspace.root_path}",
30-
"/Workspace/${workspace.file_path}": "${workspace.file_path}",
31-
"/Workspace${workspace.file_path}": "${workspace.file_path}",
32-
"/Workspace/${workspace.artifact_path}": "${workspace.artifact_path}",
33-
"/Workspace${workspace.artifact_path}": "${workspace.artifact_path}",
34-
"/Workspace/${workspace.state_path}": "${workspace.state_path}",
35-
"/Workspace${workspace.state_path}": "${workspace.state_path}",
27+
// The patterns must cover every path that PrependWorkspacePrefix prefixes,
28+
// otherwise interpolation produces a doubled "/Workspace/Workspace/..." path.
29+
// A slice (not a map) keeps the rewrite and warning order deterministic when
30+
// multiple patterns occur in the same string.
31+
paths := []struct {
32+
pattern string
33+
replacement string
34+
}{
35+
{"/Workspace/${workspace.root_path}", "${workspace.root_path}"},
36+
{"/Workspace${workspace.root_path}", "${workspace.root_path}"},
37+
{"/Workspace/${workspace.file_path}", "${workspace.file_path}"},
38+
{"/Workspace${workspace.file_path}", "${workspace.file_path}"},
39+
{"/Workspace/${workspace.artifact_path}", "${workspace.artifact_path}"},
40+
{"/Workspace${workspace.artifact_path}", "${workspace.artifact_path}"},
41+
{"/Workspace/${workspace.state_path}", "${workspace.state_path}"},
42+
{"/Workspace${workspace.state_path}", "${workspace.state_path}"},
43+
{"/Workspace/${workspace.resource_path}", "${workspace.resource_path}"},
44+
{"/Workspace${workspace.resource_path}", "${workspace.resource_path}"},
3645
}
3746

3847
err := b.Config.Mutate(func(root dyn.Value) (dyn.Value, error) {
@@ -44,23 +53,28 @@ func (m *rewriteWorkspacePrefix) Apply(ctx context.Context, b *bundle.Bundle) di
4453
return v, nil
4554
}
4655

47-
for path, replacePath := range paths {
48-
if strings.Contains(vv, path) {
49-
newPath := strings.Replace(vv, path, replacePath, 1)
50-
diags = append(diags, diag.Diagnostic{
51-
Severity: diag.Warning,
52-
Summary: fmt.Sprintf("substring %q found in %q. Please update this to %q.", path, vv, newPath),
53-
Detail: "For more information, please refer to: https://docs.databricks.com/en/release-notes/dev-tools/bundles.html#workspace-paths",
54-
Locations: v.Locations(),
55-
Paths: []dyn.Path{p},
56-
})
57-
58-
// Remove the workspace prefix from the string.
59-
return dyn.NewValue(newPath, v.Locations()), nil
56+
newPath := vv
57+
for _, rewrite := range paths {
58+
if !strings.Contains(newPath, rewrite.pattern) {
59+
continue
6060
}
61+
62+
newPath = strings.ReplaceAll(newPath, rewrite.pattern, rewrite.replacement)
63+
diags = append(diags, diag.Diagnostic{
64+
Severity: diag.Warning,
65+
Summary: fmt.Sprintf("substring %q found in %q. Please update this to %q.", rewrite.pattern, vv, newPath),
66+
Detail: "For more information, please refer to: https://docs.databricks.com/en/release-notes/dev-tools/bundles.html#workspace-paths",
67+
Locations: v.Locations(),
68+
Paths: []dyn.Path{p},
69+
})
70+
}
71+
72+
if newPath == vv {
73+
return v, nil
6174
}
6275

63-
return v, nil
76+
// Remove the workspace prefix from the string.
77+
return dyn.NewValue(newPath, v.Locations()), nil
6478
})
6579
})
6680
if err != nil {

‎bundle/config/mutator/rewrite_workspace_prefix_test.go‎

Lines changed: 34 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ func TestNoWorkspacePrefixUsed(t *testing.T) {
2020
ArtifactPath: "/Workspace/Users/test/artifacts",
2121
FilePath: "/Workspace/Users/test/files",
2222
StatePath: "/Workspace/Users/test/state",
23+
ResourcePath: "/Workspace/Users/test/resources",
2324
},
2425

2526
Resources: config.Resources{
@@ -52,6 +53,25 @@ func TestNoWorkspacePrefixUsed(t *testing.T) {
5253
},
5354
},
5455
},
56+
{
57+
NotebookTask: &jobs.NotebookTask{
58+
NotebookPath: "/Workspace/${workspace.resource_path}/notebook3",
59+
},
60+
Libraries: []compute.Library{
61+
{
62+
Jar: "/Workspace${workspace.resource_path}/jar3.jar",
63+
},
64+
},
65+
},
66+
{
67+
SparkPythonTask: &jobs.SparkPythonTask{
68+
PythonFile: "${workspace.file_path}/file2.py",
69+
Parameters: []string{
70+
"--input=/Workspace/${workspace.file_path}/in.txt --output=/Workspace/${workspace.file_path}/out.txt",
71+
"--cp=/Workspace/${workspace.root_path}/lib.jar:/Workspace${workspace.artifact_path}/dep.jar",
72+
},
73+
},
74+
},
5575
},
5676
},
5777
},
@@ -61,12 +81,17 @@ func TestNoWorkspacePrefixUsed(t *testing.T) {
6181
}
6282

6383
diags := bundle.Apply(t.Context(), b, RewriteWorkspacePrefix())
64-
require.Len(t, diags, 3)
84+
require.Len(t, diags, 8)
6585

6686
expectedErrors := map[string]bool{
67-
`substring "/Workspace/${workspace.root_path}" found in "/Workspace/${workspace.root_path}/file1.py". Please update this to "${workspace.root_path}/file1.py".`: true,
68-
`substring "/Workspace${workspace.file_path}" found in "/Workspace${workspace.file_path}/notebook1". Please update this to "${workspace.file_path}/notebook1".`: true,
69-
`substring "/Workspace/${workspace.artifact_path}" found in "/Workspace/${workspace.artifact_path}/jar1.jar". Please update this to "${workspace.artifact_path}/jar1.jar".`: true,
87+
`substring "/Workspace/${workspace.root_path}" found in "/Workspace/${workspace.root_path}/file1.py". Please update this to "${workspace.root_path}/file1.py".`: true,
88+
`substring "/Workspace${workspace.file_path}" found in "/Workspace${workspace.file_path}/notebook1". Please update this to "${workspace.file_path}/notebook1".`: true,
89+
`substring "/Workspace/${workspace.artifact_path}" found in "/Workspace/${workspace.artifact_path}/jar1.jar". Please update this to "${workspace.artifact_path}/jar1.jar".`: true,
90+
`substring "/Workspace/${workspace.resource_path}" found in "/Workspace/${workspace.resource_path}/notebook3". Please update this to "${workspace.resource_path}/notebook3".`: true,
91+
`substring "/Workspace${workspace.resource_path}" found in "/Workspace${workspace.resource_path}/jar3.jar". Please update this to "${workspace.resource_path}/jar3.jar".`: true,
92+
`substring "/Workspace/${workspace.file_path}" found in "--input=/Workspace/${workspace.file_path}/in.txt --output=/Workspace/${workspace.file_path}/out.txt". Please update this to "--input=${workspace.file_path}/in.txt --output=${workspace.file_path}/out.txt".`: true,
93+
`substring "/Workspace/${workspace.root_path}" found in "--cp=/Workspace/${workspace.root_path}/lib.jar:/Workspace${workspace.artifact_path}/dep.jar". Please update this to "--cp=${workspace.root_path}/lib.jar:/Workspace${workspace.artifact_path}/dep.jar".`: true,
94+
`substring "/Workspace${workspace.artifact_path}" found in "--cp=/Workspace/${workspace.root_path}/lib.jar:/Workspace${workspace.artifact_path}/dep.jar". Please update this to "--cp=${workspace.root_path}/lib.jar:${workspace.artifact_path}/dep.jar".`: true,
7095
}
7196

7297
for _, d := range diags {
@@ -80,4 +105,9 @@ func TestNoWorkspacePrefixUsed(t *testing.T) {
80105
require.Equal(t, "${workspace.artifact_path}/jar1.jar", b.Config.Resources.Jobs["test_job"].JobSettings.Tasks[1].Libraries[0].Jar)
81106
require.Equal(t, "${workspace.file_path}/notebook2", b.Config.Resources.Jobs["test_job"].Tasks[2].NotebookTask.NotebookPath)
82107
require.Equal(t, "${workspace.artifact_path}/jar2.jar", b.Config.Resources.Jobs["test_job"].JobSettings.Tasks[2].Libraries[0].Jar)
108+
require.Equal(t, "${workspace.resource_path}/notebook3", b.Config.Resources.Jobs["test_job"].Tasks[3].NotebookTask.NotebookPath)
109+
require.Equal(t, "${workspace.resource_path}/jar3.jar", b.Config.Resources.Jobs["test_job"].JobSettings.Tasks[3].Libraries[0].Jar)
110+
require.Equal(t, "${workspace.file_path}/file2.py", b.Config.Resources.Jobs["test_job"].Tasks[4].SparkPythonTask.PythonFile)
111+
require.Equal(t, "--input=${workspace.file_path}/in.txt --output=${workspace.file_path}/out.txt", b.Config.Resources.Jobs["test_job"].Tasks[4].SparkPythonTask.Parameters[0])
112+
require.Equal(t, "--cp=${workspace.root_path}/lib.jar:${workspace.artifact_path}/dep.jar", b.Config.Resources.Jobs["test_job"].Tasks[4].SparkPythonTask.Parameters[1])
83113
}

0 commit comments

Comments
 (0)