From b451905b6ea3091fac34775e3a14a5af9f649d53 Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Mon, 9 Sep 2024 11:56:16 +0200 Subject: [PATCH] Expand library globs relative to the sync root (#1756) ## Changes Library glob expansion happens during deployment. Before that, all entries that refer to local paths in resource definitions are made relative to the _sync root_. Before #1694, they were made relative to the _bundle root_. This PR didn't update the library glob expansion code to use the sync root path. If you were using the sync paths setting with library globs, the CLI would fail to expand the globs because the code was using the wrong path to anchor those globs. This change fixes the issue. ## Tests Manually confirmed that this fixes the issue reported in #1755. --- bundle/libraries/expand_glob_references.go | 10 +-- .../libraries/expand_glob_references_test.go | 6 +- bundle/libraries/match_test.go | 8 +-- bundle/libraries/upload.go | 2 +- bundle/libraries/upload_test.go | 8 +-- .../bundle.yml | 3 + bundle/tests/python_wheel_test.go | 64 +++++++++---------- 7 files changed, 49 insertions(+), 52 deletions(-) diff --git a/bundle/libraries/expand_glob_references.go b/bundle/libraries/expand_glob_references.go index 9322a06b83..c71615e0e2 100644 --- a/bundle/libraries/expand_glob_references.go +++ b/bundle/libraries/expand_glob_references.go @@ -39,7 +39,7 @@ func getLibDetails(v dyn.Value) (string, string, bool) { } func findMatches(b *bundle.Bundle, path string) ([]string, error) { - matches, err := filepath.Glob(filepath.Join(b.RootPath, path)) + matches, err := filepath.Glob(filepath.Join(b.SyncRootPath, path)) if err != nil { return nil, err } @@ -52,10 +52,10 @@ func findMatches(b *bundle.Bundle, path string) ([]string, error) { } } - // We make the matched path relative to the root path before storing it + // We make the matched path relative to the sync root path before storing it // to allow upload mutator to distinguish between local and remote paths for i, match := range matches { - matches[i], err = filepath.Rel(b.RootPath, match) + matches[i], err = filepath.Rel(b.SyncRootPath, match) if err != nil { return nil, err } @@ -211,8 +211,8 @@ func (e *expand) Name() string { // ExpandGlobReferences expands any glob references in the libraries or environments section // to corresponding local paths. -// We only expand local paths (i.e. paths that are relative to the root path). -// After expanding we make the paths relative to the root path to allow upload mutator later in the chain to +// We only expand local paths (i.e. paths that are relative to the sync root path). +// After expanding we make the paths relative to the sync root path to allow upload mutator later in the chain to // distinguish between local and remote paths. func ExpandGlobReferences() bundle.Mutator { return &expand{} diff --git a/bundle/libraries/expand_glob_references_test.go b/bundle/libraries/expand_glob_references_test.go index 34855b539d..e7f2e16935 100644 --- a/bundle/libraries/expand_glob_references_test.go +++ b/bundle/libraries/expand_glob_references_test.go @@ -23,7 +23,7 @@ func TestGlobReferencesExpandedForTaskLibraries(t *testing.T) { testutil.Touch(t, dir, "jar", "my2.jar") b := &bundle.Bundle{ - RootPath: dir, + SyncRootPath: dir, Config: config.Root{ Resources: config.Resources{ Jobs: map[string]*resources.Job{ @@ -104,7 +104,7 @@ func TestGlobReferencesExpandedForForeachTaskLibraries(t *testing.T) { testutil.Touch(t, dir, "jar", "my2.jar") b := &bundle.Bundle{ - RootPath: dir, + SyncRootPath: dir, Config: config.Root{ Resources: config.Resources{ Jobs: map[string]*resources.Job{ @@ -189,7 +189,7 @@ func TestGlobReferencesExpandedForEnvironmentsDeps(t *testing.T) { testutil.Touch(t, dir, "jar", "my2.jar") b := &bundle.Bundle{ - RootPath: dir, + SyncRootPath: dir, Config: config.Root{ Resources: config.Resources{ Jobs: map[string]*resources.Job{ diff --git a/bundle/libraries/match_test.go b/bundle/libraries/match_test.go index e60504c844..78765cbd4a 100644 --- a/bundle/libraries/match_test.go +++ b/bundle/libraries/match_test.go @@ -18,7 +18,7 @@ func TestValidateEnvironments(t *testing.T) { testutil.Touch(t, tmpDir, "wheel.whl") b := &bundle.Bundle{ - RootPath: tmpDir, + SyncRootPath: tmpDir, Config: config.Root{ Resources: config.Resources{ Jobs: map[string]*resources.Job{ @@ -50,7 +50,7 @@ func TestValidateEnvironmentsNoFile(t *testing.T) { tmpDir := t.TempDir() b := &bundle.Bundle{ - RootPath: tmpDir, + SyncRootPath: tmpDir, Config: config.Root{ Resources: config.Resources{ Jobs: map[string]*resources.Job{ @@ -84,7 +84,7 @@ func TestValidateTaskLibraries(t *testing.T) { testutil.Touch(t, tmpDir, "wheel.whl") b := &bundle.Bundle{ - RootPath: tmpDir, + SyncRootPath: tmpDir, Config: config.Root{ Resources: config.Resources{ Jobs: map[string]*resources.Job{ @@ -117,7 +117,7 @@ func TestValidateTaskLibrariesNoFile(t *testing.T) { tmpDir := t.TempDir() b := &bundle.Bundle{ - RootPath: tmpDir, + SyncRootPath: tmpDir, Config: config.Root{ Resources: config.Resources{ Jobs: map[string]*resources.Job{ diff --git a/bundle/libraries/upload.go b/bundle/libraries/upload.go index 224e7ab2d9..90a1a21fc4 100644 --- a/bundle/libraries/upload.go +++ b/bundle/libraries/upload.go @@ -74,7 +74,7 @@ func collectLocalLibraries(b *bundle.Bundle) (map[string][]configLocation, error return v, nil } - source = filepath.Join(b.RootPath, source) + source = filepath.Join(b.SyncRootPath, source) libs[source] = append(libs[source], configLocation{ configPath: p, location: v.Location(), diff --git a/bundle/libraries/upload_test.go b/bundle/libraries/upload_test.go index 82fe6e7c7c..44b194c56a 100644 --- a/bundle/libraries/upload_test.go +++ b/bundle/libraries/upload_test.go @@ -24,7 +24,7 @@ func TestArtifactUploadForWorkspace(t *testing.T) { whlLocalPath := filepath.Join(whlFolder, "source.whl") b := &bundle.Bundle{ - RootPath: tmpDir, + SyncRootPath: tmpDir, Config: config.Root{ Workspace: config.Workspace{ ArtifactPath: "/foo/bar/artifacts", @@ -112,7 +112,7 @@ func TestArtifactUploadForVolumes(t *testing.T) { whlLocalPath := filepath.Join(whlFolder, "source.whl") b := &bundle.Bundle{ - RootPath: tmpDir, + SyncRootPath: tmpDir, Config: config.Root{ Workspace: config.Workspace{ ArtifactPath: "/Volumes/foo/bar/artifacts", @@ -200,7 +200,7 @@ func TestArtifactUploadWithNoLibraryReference(t *testing.T) { whlLocalPath := filepath.Join(whlFolder, "source.whl") b := &bundle.Bundle{ - RootPath: tmpDir, + SyncRootPath: tmpDir, Config: config.Root{ Workspace: config.Workspace{ ArtifactPath: "/Workspace/foo/bar/artifacts", @@ -240,7 +240,7 @@ func TestUploadMultipleLibraries(t *testing.T) { testutil.Touch(t, whlFolder, "source4.whl") b := &bundle.Bundle{ - RootPath: tmpDir, + SyncRootPath: tmpDir, Config: config.Root{ Workspace: config.Workspace{ ArtifactPath: "/foo/bar/artifacts", diff --git a/bundle/tests/python_wheel/python_wheel_no_artifact_no_setup/bundle.yml b/bundle/tests/python_wheel/python_wheel_no_artifact_no_setup/bundle.yml index 492861969d..d030843033 100644 --- a/bundle/tests/python_wheel/python_wheel_no_artifact_no_setup/bundle.yml +++ b/bundle/tests/python_wheel/python_wheel_no_artifact_no_setup/bundle.yml @@ -1,6 +1,9 @@ bundle: name: python-wheel-local +workspace: + artifact_path: /foo/bar + resources: jobs: test_job: diff --git a/bundle/tests/python_wheel_test.go b/bundle/tests/python_wheel_test.go index c4d85703cc..c982c09d68 100644 --- a/bundle/tests/python_wheel_test.go +++ b/bundle/tests/python_wheel_test.go @@ -15,11 +15,10 @@ import ( ) func TestPythonWheelBuild(t *testing.T) { - ctx := context.Background() - b, err := bundle.Load(ctx, "./python_wheel/python_wheel") - require.NoError(t, err) + b := loadTarget(t, "./python_wheel/python_wheel", "default") - diags := bundle.Apply(ctx, b, bundle.Seq(phases.Load(), phases.Build())) + ctx := context.Background() + diags := bundle.Apply(ctx, b, phases.Build()) require.NoError(t, diags.Error()) matches, err := filepath.Glob("./python_wheel/python_wheel/my_test_code/dist/my_test_code-*.whl") @@ -32,11 +31,10 @@ func TestPythonWheelBuild(t *testing.T) { } func TestPythonWheelBuildAutoDetect(t *testing.T) { - ctx := context.Background() - b, err := bundle.Load(ctx, "./python_wheel/python_wheel_no_artifact") - require.NoError(t, err) + b := loadTarget(t, "./python_wheel/python_wheel_no_artifact", "default") - diags := bundle.Apply(ctx, b, bundle.Seq(phases.Load(), phases.Build())) + ctx := context.Background() + diags := bundle.Apply(ctx, b, phases.Build()) require.NoError(t, diags.Error()) matches, err := filepath.Glob("./python_wheel/python_wheel_no_artifact/dist/my_test_code-*.whl") @@ -49,11 +47,10 @@ func TestPythonWheelBuildAutoDetect(t *testing.T) { } func TestPythonWheelBuildAutoDetectWithNotebookTask(t *testing.T) { - ctx := context.Background() - b, err := bundle.Load(ctx, "./python_wheel/python_wheel_no_artifact_notebook") - require.NoError(t, err) + b := loadTarget(t, "./python_wheel/python_wheel_no_artifact_notebook", "default") - diags := bundle.Apply(ctx, b, bundle.Seq(phases.Load(), phases.Build())) + ctx := context.Background() + diags := bundle.Apply(ctx, b, phases.Build()) require.NoError(t, diags.Error()) matches, err := filepath.Glob("./python_wheel/python_wheel_no_artifact_notebook/dist/my_test_code-*.whl") @@ -66,11 +63,10 @@ func TestPythonWheelBuildAutoDetectWithNotebookTask(t *testing.T) { } func TestPythonWheelWithDBFSLib(t *testing.T) { - ctx := context.Background() - b, err := bundle.Load(ctx, "./python_wheel/python_wheel_dbfs_lib") - require.NoError(t, err) + b := loadTarget(t, "./python_wheel/python_wheel_dbfs_lib", "default") - diags := bundle.Apply(ctx, b, bundle.Seq(phases.Load(), phases.Build())) + ctx := context.Background() + diags := bundle.Apply(ctx, b, phases.Build()) require.NoError(t, diags.Error()) match := libraries.ExpandGlobReferences() @@ -79,11 +75,11 @@ func TestPythonWheelWithDBFSLib(t *testing.T) { } func TestPythonWheelBuildNoBuildJustUpload(t *testing.T) { - ctx := context.Background() - b, err := bundle.Load(ctx, "./python_wheel/python_wheel_no_artifact_no_setup") - require.NoError(t, err) + b := loadTarget(t, "./python_wheel/python_wheel_no_artifact_no_setup", "default") - b.Config.Workspace.ArtifactPath = "/foo/bar" + ctx := context.Background() + diags := bundle.Apply(ctx, b, phases.Build()) + require.NoError(t, diags.Error()) mockFiler := mockfiler.NewMockFiler(t) mockFiler.EXPECT().Write( @@ -94,20 +90,20 @@ func TestPythonWheelBuildNoBuildJustUpload(t *testing.T) { filer.CreateParentDirectories, ).Return(nil) - u := libraries.UploadWithClient(mockFiler) - diags := bundle.Apply(ctx, b, bundle.Seq(phases.Load(), phases.Build(), libraries.ExpandGlobReferences(), u)) + diags = bundle.Apply(ctx, b, bundle.Seq( + libraries.ExpandGlobReferences(), + libraries.UploadWithClient(mockFiler), + )) require.NoError(t, diags.Error()) require.Empty(t, diags) - require.Equal(t, "/Workspace/foo/bar/.internal/my_test_code-0.0.1-py3-none-any.whl", b.Config.Resources.Jobs["test_job"].JobSettings.Tasks[0].Libraries[0].Whl) } func TestPythonWheelBuildWithEnvironmentKey(t *testing.T) { - ctx := context.Background() - b, err := bundle.Load(ctx, "./python_wheel/environment_key") - require.NoError(t, err) + b := loadTarget(t, "./python_wheel/environment_key", "default") - diags := bundle.Apply(ctx, b, bundle.Seq(phases.Load(), phases.Build())) + ctx := context.Background() + diags := bundle.Apply(ctx, b, phases.Build()) require.NoError(t, diags.Error()) matches, err := filepath.Glob("./python_wheel/environment_key/my_test_code/dist/my_test_code-*.whl") @@ -120,11 +116,10 @@ func TestPythonWheelBuildWithEnvironmentKey(t *testing.T) { } func TestPythonWheelBuildMultiple(t *testing.T) { - ctx := context.Background() - b, err := bundle.Load(ctx, "./python_wheel/python_wheel_multiple") - require.NoError(t, err) + b := loadTarget(t, "./python_wheel/python_wheel_multiple", "default") - diags := bundle.Apply(ctx, b, bundle.Seq(phases.Load(), phases.Build())) + ctx := context.Background() + diags := bundle.Apply(ctx, b, phases.Build()) require.NoError(t, diags.Error()) matches, err := filepath.Glob("./python_wheel/python_wheel_multiple/my_test_code/dist/my_test_code*.whl") @@ -137,11 +132,10 @@ func TestPythonWheelBuildMultiple(t *testing.T) { } func TestPythonWheelNoBuild(t *testing.T) { - ctx := context.Background() - b, err := bundle.Load(ctx, "./python_wheel/python_wheel_no_build") - require.NoError(t, err) + b := loadTarget(t, "./python_wheel/python_wheel_no_build", "default") - diags := bundle.Apply(ctx, b, bundle.Seq(phases.Load(), phases.Build())) + ctx := context.Background() + diags := bundle.Apply(ctx, b, phases.Build()) require.NoError(t, diags.Error()) match := libraries.ExpandGlobReferences()