Fix processing short pip flags in environment dependencies #3708
Conversation
andrewnester
requested review from
anton-107,
denik,
pietern and
shreyas-goenka
as code owners
andrewnester
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
andrewnester
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
bundle/libraries/local_path.go
Outdated
|
|
||
| func containsPipFlag(input string) bool { | ||
| re := regexp.MustCompile(`--[a-zA-Z0-9-]+`) | ||
| re := regexp.MustCompile(`--?[a-zA-Z0-9-]+`) |
There was a problem hiding this comment.
The caller of this function can now be simplified. It checks for -r right after.
Btw, how do we do local relative path interpretation for -r, and can we extend that to -e as well?
andrewnester
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
| {path: "s3://mybucket/path/to/package", expected: false}, | ||
| {path: "dbfs:/mnt/path/to/package", expected: false}, | ||
| {path: "beautifulsoup4", expected: false}, | ||
| {path: "-e some/local/path", expected: false}, |
There was a problem hiding this comment.
There are some more tricky cases that we should cover here :-/ AI might help identify and handle them
-i http://myindexurl.com
--trusted-host pypi.org
--platform win_amd64
-e ${workspace.file_path}
-e /Workspace/path/to
-e /Volumes/path/to
Btw, Andrew, please consider how to incorporate such cases. It could be that what you have here is just net-better than what we have now and that we should have a followup with refinements. It could also be that we just need to revise this PR to cover things like -i properly, also to make sure we don't regress anything.
There was a problem hiding this comment.
We have quite a good coverage for this overall, but I've added some additional cases for unit / acceptacne tests
|
andrewnester
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
andrewnester
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
| TranslateModeEnvironmentPipFlag, | ||
| func(s string) bool { | ||
| _, ok := libraries.IsLocalRequirementsFile(s) | ||
| _, _, ok := libraries.IsLocalPathInPipFlag(s) |
There was a problem hiding this comment.
Can you make the same change for pipeline environment dependencies?
There was a problem hiding this comment.
We don't handle any flags there yet, so could be a separate follow-up PR with dedicated changelog entry.
There was a problem hiding this comment.
I'll do this as a follow-up for this PR
| ### Dependency updates | ||
|
|
||
| ### Bundles | ||
| * Fix processing short pip flags in environment dependencies ([#3708](https://github.com/databricks/cli/pull/3708)) |
There was a problem hiding this comment.
This PR now also handles relative paths for -e. We can include additional entries for the same PR, for different user-visible changes (eg call out -e specifically).
andrewnester
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
|
|
||
| func IsLocalPathInPipFlag(dep string) (string, string, bool) { | ||
| for _, flag := range PipFlagsWithLocalPaths { | ||
| dep, ok := strings.CutPrefix(dep, flag+" ") |
There was a problem hiding this comment.
dep aliases the input. It works because of := but is error-prone. Recommend to use a different name.
| // Trailing space means the the flag takes an argument or there's multiple arguments in input | ||
| // Alternatively it could be a flag with no argument and no space after it | ||
| // For example: -r myfile.txt or --index-url http://myindexurl.com or -i | ||
| re := regexp.MustCompile(`--?[a-zA-Z0-9-]+(\s|$)`) |
There was a problem hiding this comment.
This made me realize we also need ^ on the regex (or ^\s*), or we risk also matching foo-bar.
While unlikely to be a valid filename, it is never a flag.
There was a problem hiding this comment.
true, it should be ^\s* indeed, will add an use case for this
## Changes Added ^\s* for pip flag regexp so we don't match `foo-bar` as a flag Renamed variable to avoid shadowing ## Why Followups for #3708
## Changes Fix PIP flag processing in pipeline environment dependencies ## Why Follow up for #3708 ## Tests Added an acceptance test <!-- 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. -->
## Release v0.272.0 ### Bundles * Fix processing short pip flags in environment dependencies ([#3708](#3708)) * Add support for referencing local files in -e pip flag for environment dependencies ([#3708](#3708)) * Add error for when an etag is specified in dashboard configuration. Setting etags was never supported / valid in bundles but now users will see this error during validation rather than deployment. ([#3723](#3723)) * Fix PIP flag processing in pipeline environment dependencies ([#3734](#3734))
Changes
Fix processing short pip flags in environment dependencies
Why
Fixes #3674
Tests
Added unit test case