-
Notifications
You must be signed in to change notification settings - Fork 154
Added support for job environments #1379
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
9e580b7
549d22d
8ca8233
d6c7739
8d830a3
9f19ddc
b6b025b
eec9b9b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,7 +12,6 @@ import ( | |
| "github.com/databricks/cli/bundle" | ||
| "github.com/databricks/cli/bundle/artifacts/whl" | ||
| "github.com/databricks/cli/bundle/config" | ||
| "github.com/databricks/cli/bundle/libraries" | ||
| "github.com/databricks/cli/libs/cmdio" | ||
| "github.com/databricks/cli/libs/diag" | ||
| "github.com/databricks/cli/libs/filer" | ||
|
|
@@ -117,8 +116,6 @@ func (m *basicUpload) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnost | |
| } | ||
|
|
||
| func uploadArtifact(ctx context.Context, b *bundle.Bundle, a *config.Artifact, uploadPath string, client filer.Filer) error { | ||
| filesToLibraries := libraries.MapFilesToTaskLibraries(ctx, b) | ||
|
|
||
| for i := range a.Files { | ||
| f := &a.Files[i] | ||
|
|
||
|
|
@@ -133,31 +130,59 @@ func uploadArtifact(ctx context.Context, b *bundle.Bundle, a *config.Artifact, u | |
| log.Infof(ctx, "Upload succeeded") | ||
| f.RemotePath = path.Join(uploadPath, filepath.Base(f.Source)) | ||
|
|
||
| // Lookup all tasks that reference this file. | ||
| libs, ok := filesToLibraries[f.Source] | ||
| if !ok { | ||
| log.Debugf(ctx, "No tasks reference %s", f.Source) | ||
| continue | ||
| } | ||
|
|
||
| // Update all tasks that reference this file. | ||
| for _, lib := range libs { | ||
| wsfsBase := "/Workspace" | ||
| remotePath := path.Join(wsfsBase, f.RemotePath) | ||
| if lib.Whl != "" { | ||
| lib.Whl = remotePath | ||
| continue | ||
| // TODO: confirm if we still need to update the remote path to start with /Workspace | ||
| wsfsBase := "/Workspace" | ||
| remotePath := path.Join(wsfsBase, f.RemotePath) | ||
|
|
||
| for _, job := range b.Config.Resources.Jobs { | ||
| for i := range job.Tasks { | ||
| task := &job.Tasks[i] | ||
| for j := range task.Libraries { | ||
| lib := &task.Libraries[j] | ||
| if lib.Whl != "" && isArtifactMatchLibrary(f, lib.Whl, b) { | ||
| lib.Whl = remotePath | ||
| } | ||
| if lib.Jar != "" && isArtifactMatchLibrary(f, lib.Jar, b) { | ||
| lib.Jar = remotePath | ||
| } | ||
| } | ||
| } | ||
| if lib.Jar != "" { | ||
| lib.Jar = remotePath | ||
| continue | ||
|
|
||
| for i := range job.Environments { | ||
| env := &job.Environments[i] | ||
| for j := range env.Spec.Dependencies { | ||
| lib := env.Spec.Dependencies[j] | ||
| if isArtifactMatchLibrary(f, lib, b) { | ||
| env.Spec.Dependencies[j] = remotePath | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| func isArtifactMatchLibrary(f *config.ArtifactFile, libPath string, b *bundle.Bundle) bool { | ||
| if !filepath.IsAbs(libPath) { | ||
| libPath = filepath.Join(b.RootPath, libPath) | ||
| } | ||
|
|
||
| // libPath can be a glob pattern, so do the match first | ||
| matches, err := filepath.Glob(libPath) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't the globs have been expanded already at this point?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @pietern I don't think we do this in translate paths now but we should but can be a follow up |
||
| if err != nil { | ||
| return false | ||
| } | ||
|
|
||
| for _, m := range matches { | ||
| if m == f.Source { | ||
| return true | ||
| } | ||
| } | ||
|
|
||
| return false | ||
| } | ||
|
|
||
| // Function to upload artifact file to Workspace | ||
| func uploadArtifactFile(ctx context.Context, file string, client filer.Filer) error { | ||
| raw, err := os.ReadFile(file) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,91 @@ | ||
| package artifacts | ||
|
|
||
| import ( | ||
| "context" | ||
| "path/filepath" | ||
| "testing" | ||
|
|
||
| "github.com/databricks/cli/bundle" | ||
| "github.com/databricks/cli/bundle/config" | ||
| "github.com/databricks/cli/bundle/config/resources" | ||
| mockfiler "github.com/databricks/cli/internal/mocks/libs/filer" | ||
| "github.com/databricks/cli/internal/testutil" | ||
| "github.com/databricks/cli/libs/filer" | ||
| "github.com/databricks/databricks-sdk-go/service/compute" | ||
| "github.com/databricks/databricks-sdk-go/service/jobs" | ||
| "github.com/stretchr/testify/mock" | ||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| func TestArtifactUpload(t *testing.T) { | ||
| tmpDir := t.TempDir() | ||
| whlFolder := filepath.Join(tmpDir, "whl") | ||
| testutil.Touch(t, whlFolder, "source.whl") | ||
| whlLocalPath := filepath.Join(whlFolder, "source.whl") | ||
|
|
||
| b := &bundle.Bundle{ | ||
| RootPath: tmpDir, | ||
| Config: config.Root{ | ||
| Workspace: config.Workspace{ | ||
| ArtifactPath: "/foo/bar/artifacts", | ||
| }, | ||
| Artifacts: config.Artifacts{ | ||
| "whl": { | ||
| Type: config.ArtifactPythonWheel, | ||
| Files: []config.ArtifactFile{ | ||
| {Source: whlLocalPath}, | ||
| }, | ||
| }, | ||
| }, | ||
| Resources: config.Resources{ | ||
| Jobs: map[string]*resources.Job{ | ||
| "job": { | ||
| JobSettings: &jobs.JobSettings{ | ||
| Tasks: []jobs.Task{ | ||
| { | ||
| Libraries: []compute.Library{ | ||
| { | ||
| Whl: filepath.Join("whl", "*.whl"), | ||
| }, | ||
| { | ||
| Whl: "/Workspace/Users/foo@bar.com/mywheel.whl", | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| Environments: []jobs.JobEnvironment{ | ||
| { | ||
| Spec: &compute.Environment{ | ||
| Dependencies: []string{ | ||
| filepath.Join("whl", "source.whl"), | ||
| "/Workspace/Users/foo@bar.com/mywheel.whl", | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| artifact := b.Config.Artifacts["whl"] | ||
| mockFiler := mockfiler.NewMockFiler(t) | ||
| mockFiler.EXPECT().Write( | ||
| mock.Anything, | ||
| filepath.Join("source.whl"), | ||
| mock.AnythingOfType("*bytes.Reader"), | ||
| filer.OverwriteIfExists, | ||
| filer.CreateParentDirectories, | ||
| ).Return(nil) | ||
|
|
||
| err := uploadArtifact(context.Background(), b, artifact, "/foo/bar/artifacts", mockFiler) | ||
| require.NoError(t, err) | ||
|
|
||
| // Test that libraries path is updated | ||
| require.Equal(t, "/Workspace/foo/bar/artifacts/source.whl", b.Config.Resources.Jobs["job"].JobSettings.Tasks[0].Libraries[0].Whl) | ||
| require.Equal(t, "/Workspace/Users/foo@bar.com/mywheel.whl", b.Config.Resources.Jobs["job"].JobSettings.Tasks[0].Libraries[1].Whl) | ||
| require.Equal(t, "/Workspace/foo/bar/artifacts/source.whl", b.Config.Resources.Jobs["job"].JobSettings.Environments[0].Spec.Dependencies[0]) | ||
| require.Equal(t, "/Workspace/Users/foo@bar.com/mywheel.whl", b.Config.Resources.Jobs["job"].JobSettings.Environments[0].Spec.Dependencies[1]) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you file an issue for this and link it here? Worth checking indeed (can be async), but we should track it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added here #1388