Skip to content

Commit 4246149

Browse files
authored
Fix processing short pip flags in environment dependencies (#3708)
## Changes Fix processing short pip flags in environment dependencies ## Why Fixes #3674 ## Tests Added unit test case <!-- If your PR needs to be included in the release notes for next release, add a separate entry in NEXT_CHANGELOG.md as part of your PR. -->
1 parent f171f8f commit 4246149

File tree

11 files changed

+70
-36
lines changed

11 files changed

+70
-36
lines changed

NEXT_CHANGELOG.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,5 +9,7 @@
99
### Dependency updates
1010

1111
### Bundles
12+
* Fix processing short pip flags in environment dependencies ([#3708](https://github.com/databricks/cli/pull/3708))
13+
* Add support for referencing local files in -e pip flag for environment dependencies ([#3708](https://github.com/databricks/cli/pull/3708))
1214

1315
### API Changes

acceptance/bundle/environments/dependencies/databricks.yml

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,9 @@ resources:
2121
client: "1"
2222
dependencies:
2323
- "-r ./requirements.txt"
24+
- "-e ./file.py"
25+
- "-i http://myindexurl.com"
26+
- "--index-url http://myindexurl.com"
2427
- "test_package"
2528
- "test_package==2.0.1"
2629
- "test_package>=2.0.1"

acceptance/bundle/environments/dependencies/file.py

Whitespace-only changes.

acceptance/bundle/environments/dependencies/output.txt

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,9 @@ Deployment complete!
2323
"client": "1",
2424
"dependencies": [
2525
"-r /Workspace/Users/[USERNAME]/.bundle/dependencies/default/files/requirements.txt",
26+
"-e /Workspace/Users/[USERNAME]/.bundle/dependencies/default/files/file.py",
27+
"-i http://myindexurl.com",
28+
"--index-url http://myindexurl.com",
2629
"test_package",
2730
"test_package==2.0.1",
2831
"test_package>=2.0.1",
@@ -79,6 +82,9 @@ Deployment complete!
7982
"client": "1",
8083
"dependencies": [
8184
"-r /Workspace/Users/[USERNAME]/.bundle/dependencies/default/files/requirements.txt",
85+
"-e /Workspace/Users/[USERNAME]/.bundle/dependencies/default/files/file.py",
86+
"-i http://myindexurl.com",
87+
"--index-url http://myindexurl.com",
8288
"test_package",
8389
"test_package==2.0.1",
8490
"test_package>=2.0.1",

bundle/config/mutator/normalize_paths.go

Lines changed: 14 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import (
1111

1212
"github.com/databricks/cli/bundle"
1313
"github.com/databricks/cli/bundle/config/mutator/paths"
14+
"github.com/databricks/cli/bundle/libraries"
1415
"github.com/databricks/cli/libs/diag"
1516
"github.com/databricks/cli/libs/dyn"
1617
)
@@ -85,18 +86,20 @@ func collectGitSourcePaths(b *bundle.Bundle) []dyn.Path {
8586
}
8687

8788
func normalizePath(path string, location dyn.Location, bundleRootPath string) (string, error) {
88-
// Handle requirements file paths with -r flag
89-
reqPath, ok := strings.CutPrefix(path, "-r ")
90-
if ok {
91-
// Normalize the path part
92-
reqPath = strings.TrimSpace(reqPath)
93-
normalizedPath, err := normalizePath(reqPath, location, bundleRootPath)
94-
if err != nil {
95-
return "", err
96-
}
89+
// Handle local file paths used inside pip flags
90+
for _, flag := range libraries.PipFlagsWithLocalPaths {
91+
reqPath, ok := strings.CutPrefix(path, flag+" ")
92+
if ok {
93+
// Normalize the path part
94+
reqPath = strings.TrimSpace(reqPath)
95+
normalizedPath, err := normalizePath(reqPath, location, bundleRootPath)
96+
if err != nil {
97+
return "", err
98+
}
9799

98-
// Reconstruct the path with -r flag
99-
return "-r " + normalizedPath, nil
100+
// Reconstruct the path with -r flag
101+
return flag + " " + normalizedPath, nil
102+
}
100103
}
101104

102105
pathAsUrl, err := url.Parse(path)

bundle/config/mutator/normalize_paths_test.go

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -78,6 +78,14 @@ func TestNormalizePath_requirementsFile(t *testing.T) {
7878
assert.Equal(t, "-r requirements.txt", value)
7979
}
8080

81+
func TestNormalizePath_environmentDependency(t *testing.T) {
82+
tmpDir := t.TempDir()
83+
location := dyn.Location{File: filepath.Join(tmpDir, "resources", "job_1.yml")}
84+
value, err := normalizePath("-e ../file.py", location, tmpDir)
85+
assert.NoError(t, err)
86+
assert.Equal(t, "-e file.py", value)
87+
}
88+
8189
func TestLocationDirectory(t *testing.T) {
8290
loc := dyn.Location{File: "file", Line: 1, Column: 2}
8391
dir, err := locationDirectory(loc)

bundle/config/mutator/paths/job_libraries_paths_visitor.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -62,9 +62,9 @@ func jobLibrariesRewritePatterns() []jobRewritePattern {
6262
dyn.Key("dependencies"),
6363
dyn.AnyIndex(),
6464
),
65-
TranslateModeEnvironmentRequirements,
65+
TranslateModeEnvironmentPipFlag,
6666
func(s string) bool {
67-
_, ok := libraries.IsLocalRequirementsFile(s)
67+
_, _, ok := libraries.IsLocalPathInPipFlag(s)
6868
return !ok
6969
},
7070
},

bundle/config/mutator/paths/translation_mode.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,6 @@ const (
3030
// This allows for disambiguating between paths and PyPI package names.
3131
TranslateModeLocalRelativeWithPrefix
3232

33-
// TranslateModeEnvironmentRequirements translates a local requirements file path to be absolute.
34-
TranslateModeEnvironmentRequirements
33+
// TranslateModeEnvironmentPipFlag translates a local file path in a pip flag to be absolute.
34+
TranslateModeEnvironmentPipFlag
3535
)

bundle/config/mutator/translate_paths.go

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -104,8 +104,10 @@ func (t *translateContext) rewritePath(
104104
opts translateOptions,
105105
) (string, error) {
106106
// If the input is a local requirements file, we need to translate it to an absolute path.
107-
if reqPath, ok := libraries.IsLocalRequirementsFile(input); ok {
107+
var flag string
108+
if reqPath, flagPrefix, ok := libraries.IsLocalPathInPipFlag(input); ok {
108109
input = reqPath
110+
flag = flagPrefix
109111
}
110112

111113
// We assume absolute paths point to a location in the workspace
@@ -160,10 +162,10 @@ func (t *translateContext) rewritePath(
160162
interp, err = t.translateLocalRelativePath(ctx, input, localPath, localRelPath)
161163
case paths.TranslateModeLocalRelativeWithPrefix:
162164
interp, err = t.translateLocalRelativeWithPrefixPath(ctx, input, localPath, localRelPath)
163-
case paths.TranslateModeEnvironmentRequirements:
165+
case paths.TranslateModeEnvironmentPipFlag:
164166
interp, err = t.translateFilePath(ctx, input, localPath, localRelPath)
165-
// Add the -r flag to the path to indicate it's a requirements file used for environment dependencies.
166-
interp = "-r " + interp
167+
// Add the flag prefix to the path to indicate it's a local file used in pip flags for environment dependencies.
168+
interp = flag + " " + interp
167169
default:
168170
return "", fmt.Errorf("unsupported translate mode: %d", opts.Mode)
169171
}

bundle/libraries/local_path.go

Lines changed: 20 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -58,17 +58,11 @@ func IsLibraryLocal(dep string) bool {
5858
}
5959
}
6060

61-
// If the dependency starts with --, it's a pip flag option which is a valid
62-
// entry for environment dependencies but not a local path
63-
if containsPipFlag(dep) {
64-
return false
65-
}
66-
67-
// If the dependency is a requirements file, it can either be a local path or a remote path.
68-
// Even though the path to requirements.txt can be local we don't return true in this function anyway
61+
// If the dependency starts with - or --, it's a pip flag option which is a valid
62+
// entry for environment dependencies. Even though the path in the flag can be local we don't return true in this function anyway
6963
// and don't treat such path as a local library path.
7064
// Instead we handle translation of these paths in a separate code path in TranslatePath mutator.
71-
if strings.HasPrefix(dep, "-r") {
65+
if containsPipFlag(dep) {
7266
return false
7367
}
7468

@@ -80,18 +74,28 @@ func IsLibraryLocal(dep string) bool {
8074
return IsLocalPath(dep)
8175
}
8276

83-
func IsLocalRequirementsFile(dep string) (string, bool) {
84-
dep, ok := strings.CutPrefix(dep, "-r ")
85-
if !ok {
86-
return "", false
77+
var PipFlagsWithLocalPaths = []string{
78+
"-r",
79+
"-e",
80+
}
81+
82+
func IsLocalPathInPipFlag(dep string) (string, string, bool) {
83+
for _, flag := range PipFlagsWithLocalPaths {
84+
dep, ok := strings.CutPrefix(dep, flag+" ")
85+
if ok {
86+
dep = strings.TrimSpace(dep)
87+
return dep, flag, IsLocalPath(dep)
88+
}
8789
}
8890

89-
dep = strings.TrimSpace(dep)
90-
return dep, IsLocalPath(dep)
91+
return "", "", false
9192
}
9293

9394
func containsPipFlag(input string) bool {
94-
re := regexp.MustCompile(`--[a-zA-Z0-9-]+`)
95+
// Trailing space means the the flag takes an argument or there's multiple arguments in input
96+
// Alternatively it could be a flag with no argument and no space after it
97+
// For example: -r myfile.txt or --index-url http://myindexurl.com or -i
98+
re := regexp.MustCompile(`--?[a-zA-Z0-9-]+(\s|$)`)
9599
return re.MatchString(input)
96100
}
97101

0 commit comments

Comments
 (0)