mirror of https://github.com/databricks/cli.git
Do not treat empty path as a local path (#1717)
## Changes Fixes issue introduced here https://github.com/databricks/cli/pull/1699 where PyPi packages were treated as local library. The reason is that `libraryPath` returns an empty string as a path for PyPi packages and then `IsLibraryLocal` treated empty string as local path. Both of these functions are fixed in this PR. ## Tests Added regression test
This commit is contained in:
parent
84b47745e4
commit
783e05c939
|
@ -1,19 +1,24 @@
|
|||
package libraries
|
||||
|
||||
import "github.com/databricks/databricks-sdk-go/service/compute"
|
||||
import (
|
||||
"fmt"
|
||||
|
||||
func libraryPath(library *compute.Library) string {
|
||||
"github.com/databricks/databricks-sdk-go/service/compute"
|
||||
)
|
||||
|
||||
func libraryPath(library *compute.Library) (string, error) {
|
||||
if library.Whl != "" {
|
||||
return library.Whl
|
||||
return library.Whl, nil
|
||||
}
|
||||
if library.Jar != "" {
|
||||
return library.Jar
|
||||
return library.Jar, nil
|
||||
}
|
||||
if library.Egg != "" {
|
||||
return library.Egg
|
||||
return library.Egg, nil
|
||||
}
|
||||
if library.Requirements != "" {
|
||||
return library.Requirements
|
||||
return library.Requirements, nil
|
||||
}
|
||||
return ""
|
||||
|
||||
return "", fmt.Errorf("not supported library type")
|
||||
}
|
||||
|
|
|
@ -10,9 +10,27 @@ import (
|
|||
func TestLibraryPath(t *testing.T) {
|
||||
path := "/some/path"
|
||||
|
||||
assert.Equal(t, path, libraryPath(&compute.Library{Whl: path}))
|
||||
assert.Equal(t, path, libraryPath(&compute.Library{Jar: path}))
|
||||
assert.Equal(t, path, libraryPath(&compute.Library{Egg: path}))
|
||||
assert.Equal(t, path, libraryPath(&compute.Library{Requirements: path}))
|
||||
assert.Equal(t, "", libraryPath(&compute.Library{}))
|
||||
p, err := libraryPath(&compute.Library{Whl: path})
|
||||
assert.Equal(t, path, p)
|
||||
assert.Nil(t, err)
|
||||
|
||||
p, err = libraryPath(&compute.Library{Jar: path})
|
||||
assert.Equal(t, path, p)
|
||||
assert.Nil(t, err)
|
||||
|
||||
p, err = libraryPath(&compute.Library{Egg: path})
|
||||
assert.Equal(t, path, p)
|
||||
assert.Nil(t, err)
|
||||
|
||||
p, err = libraryPath(&compute.Library{Requirements: path})
|
||||
assert.Equal(t, path, p)
|
||||
assert.Nil(t, err)
|
||||
|
||||
p, err = libraryPath(&compute.Library{})
|
||||
assert.Equal(t, "", p)
|
||||
assert.NotNil(t, err)
|
||||
|
||||
p, err = libraryPath(&compute.Library{Pypi: &compute.PythonPyPiLibrary{Package: "pypipackage"}})
|
||||
assert.Equal(t, "", p)
|
||||
assert.NotNil(t, err)
|
||||
}
|
||||
|
|
|
@ -67,7 +67,12 @@ func FindTasksWithLocalLibraries(b *bundle.Bundle) []jobs.Task {
|
|||
|
||||
func isTaskWithLocalLibraries(task jobs.Task) bool {
|
||||
for _, l := range task.Libraries {
|
||||
if IsLibraryLocal(libraryPath(&l)) {
|
||||
p, err := libraryPath(&l)
|
||||
// If there's an error, skip the library because it's not of supported type
|
||||
if err != nil {
|
||||
continue
|
||||
}
|
||||
if IsLibraryLocal(p) {
|
||||
return true
|
||||
}
|
||||
}
|
||||
|
|
|
@ -43,6 +43,10 @@ func IsLocalPath(p string) bool {
|
|||
// We can't use IsLocalPath beacuse environment dependencies can be
|
||||
// a pypi package name which can be misinterpreted as a local path by IsLocalPath.
|
||||
func IsLibraryLocal(dep string) bool {
|
||||
if dep == "" {
|
||||
return false
|
||||
}
|
||||
|
||||
possiblePrefixes := []string{
|
||||
".",
|
||||
}
|
||||
|
|
|
@ -48,6 +48,7 @@ func TestIsLibraryLocal(t *testing.T) {
|
|||
{path: "../../local/*.whl", expected: true},
|
||||
{path: "..\\..\\local\\*.whl", expected: true},
|
||||
{path: "file://path/to/package/whl.whl", expected: true},
|
||||
{path: "", expected: false},
|
||||
{path: "pypipackage", expected: false},
|
||||
{path: "/Volumes/catalog/schema/volume/path.whl", expected: false},
|
||||
{path: "/Workspace/my_project/dist.whl", expected: false},
|
||||
|
|
|
@ -29,8 +29,8 @@ func IsWorkspacePath(path string) bool {
|
|||
|
||||
// IsWorkspaceLibrary returns true if the specified library refers to a workspace path.
|
||||
func IsWorkspaceLibrary(library *compute.Library) bool {
|
||||
path := libraryPath(library)
|
||||
if path == "" {
|
||||
path, err := libraryPath(library)
|
||||
if err != nil {
|
||||
return false
|
||||
}
|
||||
|
||||
|
|
|
@ -223,6 +223,17 @@ func TestNoIncompatibleWheelTasks(t *testing.T) {
|
|||
{Whl: "./dist/test.whl"},
|
||||
},
|
||||
},
|
||||
{
|
||||
TaskKey: "key7",
|
||||
PythonWheelTask: &jobs.PythonWheelTask{},
|
||||
ExistingClusterId: "test-key-2",
|
||||
Libraries: []compute.Library{
|
||||
{Whl: "signol_lib-0.4.4-20240822+prod-py3-none-any.whl"},
|
||||
{Pypi: &compute.PythonPyPiLibrary{
|
||||
Package: "requests==2.25.1",
|
||||
}},
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
|
@ -241,6 +252,46 @@ func TestNoIncompatibleWheelTasks(t *testing.T) {
|
|||
require.False(t, hasIncompatibleWheelTasks(context.Background(), b))
|
||||
}
|
||||
|
||||
func TestTasksWithPyPiPackageAreCompatible(t *testing.T) {
|
||||
b := &bundle.Bundle{
|
||||
Config: config.Root{
|
||||
Resources: config.Resources{
|
||||
Jobs: map[string]*resources.Job{
|
||||
"job1": {
|
||||
JobSettings: &jobs.JobSettings{
|
||||
JobClusters: []jobs.JobCluster{
|
||||
{
|
||||
JobClusterKey: "cluster1",
|
||||
NewCluster: compute.ClusterSpec{
|
||||
SparkVersion: "12.2.x-scala2.12",
|
||||
},
|
||||
},
|
||||
},
|
||||
Tasks: []jobs.Task{
|
||||
{
|
||||
TaskKey: "key1",
|
||||
PythonWheelTask: &jobs.PythonWheelTask{},
|
||||
ExistingClusterId: "test-key-2",
|
||||
Libraries: []compute.Library{
|
||||
{Pypi: &compute.PythonPyPiLibrary{
|
||||
Package: "requests==2.25.1",
|
||||
}},
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
}
|
||||
|
||||
m := mocks.NewMockWorkspaceClient(t)
|
||||
b.SetWorkpaceClient(m.WorkspaceClient)
|
||||
|
||||
require.False(t, hasIncompatibleWheelTasks(context.Background(), b))
|
||||
}
|
||||
|
||||
func TestNoWarningWhenPythonWheelWrapperIsOn(t *testing.T) {
|
||||
b := &bundle.Bundle{
|
||||
Config: config.Root{
|
||||
|
|
Loading…
Reference in New Issue