From e23ca61270ff47dc3430a15835e6f2719839e687 Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Mon, 28 Aug 2023 12:30:42 +0200 Subject: [PATCH 1/3] Correctly identify local paths in libraries section --- bundle/libraries/libraries.go | 22 +++++++++++++++------- bundle/libraries/libraries_test.go | 30 ++++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 7 deletions(-) create mode 100644 bundle/libraries/libraries_test.go diff --git a/bundle/libraries/libraries.go b/bundle/libraries/libraries.go index 29848236cc..61160d64de 100644 --- a/bundle/libraries/libraries.go +++ b/bundle/libraries/libraries.go @@ -3,6 +3,7 @@ package libraries import ( "context" "fmt" + "net/url" "path/filepath" "strings" @@ -92,13 +93,13 @@ func findArtifactsAndMarkForUpload(ctx context.Context, lib *compute.Library, b } if len(matches) == 0 && isLocalLibrary(lib) { - return fmt.Errorf("no library found for %s", libPath(lib)) + return fmt.Errorf("file %s is referenced in libraries section but no such file found on local file system", libPath(lib)) } for _, match := range matches { af, err := findArtifactFileByLocalPath(match, b) if err != nil { - cmdio.LogString(ctx, fmt.Sprintf("%s. Skipping %s. In order to use the library upload it manually", err.Error(), match)) + cmdio.LogString(ctx, fmt.Sprintf("%s. Skipping uploading. In order to use the define 'artifacts' section", err.Error())) } else { af.Libraries = append(af.Libraries, lib) } @@ -116,7 +117,7 @@ func findArtifactFileByLocalPath(path string, b *bundle.Bundle) (*config.Artifac } } - return nil, fmt.Errorf("artifact file is not found for path %s", path) + return nil, fmt.Errorf("artifact section is not defined for file at %s", path) } func libPath(library *compute.Library) string { @@ -139,11 +140,18 @@ func isLocalLibrary(library *compute.Library) bool { return false } - return !isDbfsPath(path) && !isWorkspacePath(path) -} + url, err := url.Parse(path) + if err != nil { + return true + } + + // If scheme exists in path and it's "file" than it's a local path + // If scheme exists but it's not in the format of "scheme://" it means it's a Windows full path + if url.Scheme != "" { + return url.Scheme == "file" || !strings.HasPrefix(path, url.Scheme+"://") + } -func isDbfsPath(path string) bool { - return strings.HasPrefix(path, "dbfs:/") + return !isWorkspacePath(path) } func isWorkspacePath(path string) bool { diff --git a/bundle/libraries/libraries_test.go b/bundle/libraries/libraries_test.go new file mode 100644 index 0000000000..050efe7498 --- /dev/null +++ b/bundle/libraries/libraries_test.go @@ -0,0 +1,30 @@ +package libraries + +import ( + "fmt" + "testing" + + "github.com/databricks/databricks-sdk-go/service/compute" + "github.com/stretchr/testify/require" +) + +var testCases map[string]bool = map[string]bool{ + "./some/local/path": true, + "/some/full/path": true, + "/Workspace/path/to/package": false, + "/Users/path/to/package": false, + "file://path/to/package": true, + "C:\\path\\to\\package": true, + "dbfs://path/to/package": false, + "s3://path/to/package": false, + "abfss://path/to/package": false, +} + +func TestIsLocalLbrary(t *testing.T) { + for p, result := range testCases { + lib := compute.Library{ + Whl: p, + } + require.Equal(t, result, isLocalLibrary(&lib), fmt.Sprintf("isLocalLibrary must return %t for path %s ", result, p)) + } +} From 2aa75ee99864ffdc652fd69eecaecb76a09f02a3 Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Mon, 28 Aug 2023 18:14:14 +0200 Subject: [PATCH 2/3] fixes --- bundle/libraries/libraries.go | 30 +++++++++++++++++++++++------- bundle/tests/bundle/wheel_test.go | 13 ------------- 2 files changed, 23 insertions(+), 20 deletions(-) diff --git a/bundle/libraries/libraries.go b/bundle/libraries/libraries.go index 61160d64de..d26768f955 100644 --- a/bundle/libraries/libraries.go +++ b/bundle/libraries/libraries.go @@ -93,7 +93,7 @@ func findArtifactsAndMarkForUpload(ctx context.Context, lib *compute.Library, b } if len(matches) == 0 && isLocalLibrary(lib) { - return fmt.Errorf("file %s is referenced in libraries section but no such file found on local file system", libPath(lib)) + return fmt.Errorf("file %s is referenced in libraries section but doesn't exist on the local file system", libPath(lib)) } for _, match := range matches { @@ -140,20 +140,36 @@ func isLocalLibrary(library *compute.Library) bool { return false } - url, err := url.Parse(path) - if err != nil { + if isExplicitFileScheme(path) { return true } - // If scheme exists in path and it's "file" than it's a local path - // If scheme exists but it's not in the format of "scheme://" it means it's a Windows full path - if url.Scheme != "" { - return url.Scheme == "file" || !strings.HasPrefix(path, url.Scheme+"://") + if isRemoteStorageScheme(path) { + return false } return !isWorkspacePath(path) } +func isExplicitFileScheme(path string) bool { + return strings.HasPrefix(path, "file://") +} + +func isRemoteStorageScheme(path string) bool { + url, err := url.Parse(path) + if err != nil { + return false + } + + if url.Scheme == "" { + return false + } + + // If the path starts with scheme:// format, it's a correct remote storage scheme + return strings.HasPrefix(path, url.Scheme+"://") + +} + func isWorkspacePath(path string) bool { return strings.HasPrefix(path, "/Workspace/") || strings.HasPrefix(path, "/Users/") || diff --git a/bundle/tests/bundle/wheel_test.go b/bundle/tests/bundle/wheel_test.go index ee7457735c..f34528cbb0 100644 --- a/bundle/tests/bundle/wheel_test.go +++ b/bundle/tests/bundle/wheel_test.go @@ -6,7 +6,6 @@ import ( "testing" "github.com/databricks/cli/bundle" - "github.com/databricks/cli/bundle/libraries" "github.com/databricks/cli/bundle/phases" "github.com/stretchr/testify/require" ) @@ -23,10 +22,6 @@ func TestBundlePythonWheelBuild(t *testing.T) { matches, err := filepath.Glob("python_wheel/my_test_code/dist/my_test_code-*.whl") require.NoError(t, err) require.Equal(t, 1, len(matches)) - - match := libraries.MatchWithArtifacts() - err = match.Apply(ctx, b) - require.NoError(t, err) } func TestBundlePythonWheelBuildAutoDetect(t *testing.T) { @@ -41,10 +36,6 @@ func TestBundlePythonWheelBuildAutoDetect(t *testing.T) { matches, err := filepath.Glob("python_wheel/my_test_code/dist/my_test_code-*.whl") require.NoError(t, err) require.Equal(t, 1, len(matches)) - - match := libraries.MatchWithArtifacts() - err = match.Apply(ctx, b) - require.NoError(t, err) } func TestBundlePythonWheelWithDBFSLib(t *testing.T) { @@ -55,8 +46,4 @@ func TestBundlePythonWheelWithDBFSLib(t *testing.T) { m := phases.Build() err = m.Apply(ctx, b) require.NoError(t, err) - - match := libraries.MatchWithArtifacts() - err = match.Apply(ctx, b) - require.NoError(t, err) } From 771c3093bc29dad16886a5e2e003fc0439b67528 Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Mon, 28 Aug 2023 18:20:36 +0200 Subject: [PATCH 3/3] bring back missing test case --- bundle/tests/bundle/wheel_test.go | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/bundle/tests/bundle/wheel_test.go b/bundle/tests/bundle/wheel_test.go index f34528cbb0..ee7457735c 100644 --- a/bundle/tests/bundle/wheel_test.go +++ b/bundle/tests/bundle/wheel_test.go @@ -6,6 +6,7 @@ import ( "testing" "github.com/databricks/cli/bundle" + "github.com/databricks/cli/bundle/libraries" "github.com/databricks/cli/bundle/phases" "github.com/stretchr/testify/require" ) @@ -22,6 +23,10 @@ func TestBundlePythonWheelBuild(t *testing.T) { matches, err := filepath.Glob("python_wheel/my_test_code/dist/my_test_code-*.whl") require.NoError(t, err) require.Equal(t, 1, len(matches)) + + match := libraries.MatchWithArtifacts() + err = match.Apply(ctx, b) + require.NoError(t, err) } func TestBundlePythonWheelBuildAutoDetect(t *testing.T) { @@ -36,6 +41,10 @@ func TestBundlePythonWheelBuildAutoDetect(t *testing.T) { matches, err := filepath.Glob("python_wheel/my_test_code/dist/my_test_code-*.whl") require.NoError(t, err) require.Equal(t, 1, len(matches)) + + match := libraries.MatchWithArtifacts() + err = match.Apply(ctx, b) + require.NoError(t, err) } func TestBundlePythonWheelWithDBFSLib(t *testing.T) { @@ -46,4 +55,8 @@ func TestBundlePythonWheelWithDBFSLib(t *testing.T) { m := phases.Build() err = m.Apply(ctx, b) require.NoError(t, err) + + match := libraries.MatchWithArtifacts() + err = match.Apply(ctx, b) + require.NoError(t, err) }