From 5875db48338b4a9ebfd8cb75059f136ef2d2c4a7 Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Tue, 29 Aug 2023 16:14:26 +0200 Subject: [PATCH 01/11] Make resource and artifact paths in bundle config relative to config folder --- bundle/config/artifact.go | 12 ++ bundle/config/mutator/translate_paths.go | 164 ++++++++++-------- .../mutator/translate_paths_artifacts.go | 36 ++++ bundle/config/mutator/translate_paths_jobs.go | 88 ++++++++++ .../mutator/translate_paths_pipelines.go | 52 ++++++ bundle/config/mutator/translate_paths_test.go | 50 ++++++ bundle/config/root.go | 17 +- bundle/config/sync.go | 18 ++ bundle/config/target.go | 2 +- .../relative_path_with_includes/bundle.yml | 25 +++ .../subfolder/include.yml | 20 +++ .../tests/relative_path_with_includes_test.go | 27 +++ 12 files changed, 437 insertions(+), 74 deletions(-) create mode 100644 bundle/config/mutator/translate_paths_artifacts.go create mode 100644 bundle/config/mutator/translate_paths_jobs.go create mode 100644 bundle/config/mutator/translate_paths_pipelines.go create mode 100644 bundle/tests/relative_path_with_includes/bundle.yml create mode 100644 bundle/tests/relative_path_with_includes/subfolder/include.yml create mode 100644 bundle/tests/relative_path_with_includes_test.go diff --git a/bundle/config/artifact.go b/bundle/config/artifact.go index 60331eb134..6e34bf9814 100644 --- a/bundle/config/artifact.go +++ b/bundle/config/artifact.go @@ -8,9 +8,19 @@ import ( "path" "strings" + "github.com/databricks/cli/bundle/config/resources" "github.com/databricks/databricks-sdk-go/service/compute" ) +type Artifacts map[string]*Artifact + +func (artifacts Artifacts) SetConfigFilePath(path string) { + for k := range artifacts { + artifact := artifacts[k] + artifact.ConfigFilePath = path + } +} + type ArtifactType string const ArtifactPythonWheel ArtifactType = `whl` @@ -34,6 +44,8 @@ type Artifact struct { // (Python wheel, Java jar and etc) itself Files []ArtifactFile `json:"files"` BuildCommand string `json:"build"` + + resources.Paths } func (a *Artifact) Build(ctx context.Context) ([]byte, error) { diff --git a/bundle/config/mutator/translate_paths.go b/bundle/config/mutator/translate_paths.go index 08f8398613..dfacad6908 100644 --- a/bundle/config/mutator/translate_paths.go +++ b/bundle/config/mutator/translate_paths.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "net/url" "os" "path" "path/filepath" @@ -11,8 +12,6 @@ import ( "github.com/databricks/cli/bundle" "github.com/databricks/cli/libs/notebook" - "github.com/databricks/databricks-sdk-go/service/jobs" - "github.com/databricks/databricks-sdk-go/service/pipelines" ) type ErrIsNotebook struct { @@ -44,7 +43,9 @@ func (m *translatePaths) Name() string { return "TranslatePaths" } -// rewritePath converts a given relative path to a stable remote workspace path. +type rewriteFunc func(b *bundle.Bundle, literal, localPath, remotePath string) (string, error) + +// rewritePath converts a given relative path from the loaded config to a new path based on the passed rewriting function // // It takes these arguments: // - The argument `dir` is the directory relative to which the given relative path is. @@ -57,13 +58,23 @@ func (m *translatePaths) rewritePath( dir string, b *bundle.Bundle, p *string, - fn func(literal, localPath, remotePath string) (string, error), + fn rewriteFunc, ) error { // We assume absolute paths point to a location in the workspace if path.IsAbs(filepath.ToSlash(*p)) { return nil } + url, err := url.Parse(*p) + if err != nil { + return err + } + + // If the file path has scheme, it's a full path and we don't need to transform it + if url.Scheme != "" { + return nil + } + // Local path is relative to the directory the resource was defined in. localPath := filepath.Join(dir, filepath.FromSlash(*p)) if interp, ok := m.seen[localPath]; ok { @@ -84,7 +95,7 @@ func (m *translatePaths) rewritePath( remotePath = path.Join(b.Config.Workspace.FilesPath, filepath.ToSlash(remotePath)) // Convert local path into workspace path via specified function. - interp, err := fn(*p, localPath, filepath.ToSlash(remotePath)) + interp, err := fn(b, *p, localPath, filepath.ToSlash(remotePath)) if err != nil { return err } @@ -94,7 +105,7 @@ func (m *translatePaths) rewritePath( return nil } -func (m *translatePaths) translateNotebookPath(literal, localPath, remotePath string) (string, error) { +func (m *translatePaths) translateNotebookPath(b *bundle.Bundle, literal, localPath, remotePath string) (string, error) { nb, _, err := notebook.Detect(localPath) if os.IsNotExist(err) { return "", fmt.Errorf("notebook %s not found", literal) @@ -110,7 +121,7 @@ func (m *translatePaths) translateNotebookPath(literal, localPath, remotePath st return strings.TrimSuffix(remotePath, filepath.Ext(localPath)), nil } -func (m *translatePaths) translateFilePath(literal, localPath, remotePath string) (string, error) { +func (m *translatePaths) translateFilePath(b *bundle.Bundle, literal, localPath, remotePath string) (string, error) { nb, _, err := notebook.Detect(localPath) if os.IsNotExist(err) { return "", fmt.Errorf("file %s not found", literal) @@ -124,91 +135,102 @@ func (m *translatePaths) translateFilePath(literal, localPath, remotePath string return remotePath, nil } -func (m *translatePaths) translateJobTask(dir string, b *bundle.Bundle, task *jobs.Task) error { - var err error +func (m *translatePaths) translateToBundleRootRelativePath(b *bundle.Bundle, literal, localPath, remotePath string) (string, error) { + return filepath.Rel(b.Config.Path, localPath) +} - if task.NotebookTask != nil { - err = m.rewritePath(dir, b, &task.NotebookTask.NotebookPath, m.translateNotebookPath) - if target := (&ErrIsNotNotebook{}); errors.As(err, target) { - return fmt.Errorf(`expected a notebook for "tasks.notebook_task.notebook_path" but got a file: %w`, target) - } - if err != nil { - return err - } - } +type transformer struct { + // A directory path relative to which `path` will be transformed + dir string - if task.SparkPythonTask != nil { - err = m.rewritePath(dir, b, &task.SparkPythonTask.PythonFile, m.translateFilePath) - if target := (&ErrIsNotebook{}); errors.As(err, target) { - return fmt.Errorf(`expected a file for "tasks.spark_python_task.python_file" but got a notebook: %w`, target) - } - if err != nil { - return err - } - } + // A path to transform + path *string - return nil + // Name of the config property where the path string is coming from + configPath string + + // A function that performs the actual rewriting logic. + fn rewriteFunc } +type transformerFactory func(*translatePaths, *bundle.Bundle) ([]*transformer, error) +type selector struct { + // A path to transform + path *string -func (m *translatePaths) translatePipelineLibrary(dir string, b *bundle.Bundle, library *pipelines.PipelineLibrary) error { - var err error + // Name of the config property where the path string is coming from + configPath string - if library.Notebook != nil { - err = m.rewritePath(dir, b, &library.Notebook.Path, m.translateNotebookPath) - if target := (&ErrIsNotNotebook{}); errors.As(err, target) { - return fmt.Errorf(`expected a notebook for "libraries.notebook.path" but got a file: %w`, target) - } - if err != nil { - return err + // A function that performs the actual rewriting logic. + fn rewriteFunc +} +type selectorFunc func(resource interface{}, m *translatePaths) *selector + +// List of available transformer factories. All of them will be called to get the list of transformers to execute. +var transformerFactories []transformerFactory = []transformerFactory{ + getJobsTransformers, + getPipelineTransformers, + getArtifactsTransformers, +} + +// List of selector functions which are used to identify if the resource passed +// Should have it's path transformed. +// If it needs to be transformed, selector returns a reference to string to be transformed +// And a function to apply for this string +var selectors []selectorFunc = []selectorFunc{ + selectNotebookTask, + selectSparkTask, + selectLibraryNotebook, + selectLibraryFile, + selectArtifactPath, + selectWhlLibrary, + selectJarLibrary, +} + +// Appends the first matched transfomer for the given resource if it matches any if selectors defined +func addTransformerForResource(transformers []*transformer, m *translatePaths, resource interface{}, dir string) []*transformer { + for _, selector := range selectors { + s := selector(resource, m) + if s != nil { + transformers = append(transformers, &transformer{dir, s.path, s.configPath, s.fn}) + break } } - if library.File != nil { - err = m.rewritePath(dir, b, &library.File.Path, m.translateFilePath) - if target := (&ErrIsNotebook{}); errors.As(err, target) { - return fmt.Errorf(`expected a file for "libraries.file.path" but got a notebook: %w`, target) - } + return transformers +} + +// Returns a list of path transformers which are applied to specific configuration section based on +// defined selectors +func (m *translatePaths) getTransformers(b *bundle.Bundle) ([]*transformer, error) { + var transformers []*transformer = make([]*transformer, 0) + for _, get := range transformerFactories { + toAdd, err := get(m, b) if err != nil { - return err + return nil, err } + transformers = append(transformers, toAdd...) } - - return nil + return transformers, nil } func (m *translatePaths) Apply(_ context.Context, b *bundle.Bundle) error { m.seen = make(map[string]string) - for key, job := range b.Config.Resources.Jobs { - dir, err := job.ConfigFileDirectory() - if err != nil { - return fmt.Errorf("unable to determine directory for job %s: %w", key, err) - } - - // Do not translate job task paths if using git source - if job.GitSource != nil { - continue - } - - for i := 0; i < len(job.Tasks); i++ { - err := m.translateJobTask(dir, b, &job.Tasks[i]) - if err != nil { - return err - } - } + transfomers, err := m.getTransformers(b) + if err != nil { + return err } - for key, pipeline := range b.Config.Resources.Pipelines { - dir, err := pipeline.ConfigFileDirectory() + for _, transformer := range transfomers { + err := m.rewritePath(transformer.dir, b, transformer.path, transformer.fn) if err != nil { - return fmt.Errorf("unable to determine directory for pipeline %s: %w", key, err) - } - - for i := 0; i < len(pipeline.Libraries); i++ { - err := m.translatePipelineLibrary(dir, b, &pipeline.Libraries[i]) - if err != nil { - return err + if target := (&ErrIsNotebook{}); errors.As(err, target) { + return fmt.Errorf(`expected a file for "%s" but got a notebook: %w`, transformer.configPath, target) + } + if target := (&ErrIsNotNotebook{}); errors.As(err, target) { + return fmt.Errorf(`expected a notebook for "%s" but got a file: %w`, transformer.configPath, target) } + return err } } diff --git a/bundle/config/mutator/translate_paths_artifacts.go b/bundle/config/mutator/translate_paths_artifacts.go new file mode 100644 index 0000000000..5743bbacac --- /dev/null +++ b/bundle/config/mutator/translate_paths_artifacts.go @@ -0,0 +1,36 @@ +package mutator + +import ( + "fmt" + + "github.com/databricks/cli/bundle" + "github.com/databricks/cli/bundle/config" +) + +func selectArtifactPath(resource interface{}, m *translatePaths) *selector { + artifact, ok := resource.(*config.Artifact) + if !ok { + return nil + } + + return &selector{ + &artifact.Path, + "artifacts.path", + m.translateToBundleRootRelativePath, + } +} + +func getArtifactsTransformers(m *translatePaths, b *bundle.Bundle) ([]*transformer, error) { + var transformers []*transformer = make([]*transformer, 0) + + for key, artifact := range b.Config.Artifacts { + dir, err := artifact.ConfigFileDirectory() + if err != nil { + return nil, fmt.Errorf("unable to determine directory for artifact %s: %w", key, err) + } + + transformers = addTransformerForResource(transformers, m, artifact, dir) + } + + return transformers, nil +} diff --git a/bundle/config/mutator/translate_paths_jobs.go b/bundle/config/mutator/translate_paths_jobs.go new file mode 100644 index 0000000000..933e10310e --- /dev/null +++ b/bundle/config/mutator/translate_paths_jobs.go @@ -0,0 +1,88 @@ +package mutator + +import ( + "fmt" + + "github.com/databricks/cli/bundle" + "github.com/databricks/databricks-sdk-go/service/compute" + "github.com/databricks/databricks-sdk-go/service/jobs" +) + +func selectNotebookTask(resource interface{}, m *translatePaths) *selector { + task, ok := resource.(*jobs.Task) + if !ok || task.NotebookTask == nil { + return nil + } + + return &selector{ + &task.NotebookTask.NotebookPath, + "tasks.notebook_task.notebook_path", + m.translateNotebookPath, + } +} + +func selectSparkTask(resource interface{}, m *translatePaths) *selector { + task, ok := resource.(*jobs.Task) + if !ok || task.SparkPythonTask == nil { + return nil + } + + return &selector{ + &task.SparkPythonTask.PythonFile, + "tasks.spark_python_task.python_file", + m.translateFilePath, + } +} + +func selectWhlLibrary(resource interface{}, m *translatePaths) *selector { + library, ok := resource.(*compute.Library) + if !ok || library.Whl == "" { + return nil + } + + return &selector{ + &library.Whl, + "libraries.whl", + m.translateToBundleRootRelativePath, + } +} + +func selectJarLibrary(resource interface{}, m *translatePaths) *selector { + library, ok := resource.(*compute.Library) + if !ok || library.Jar == "" { + return nil + } + + return &selector{ + &library.Jar, + "libraries.jar", + m.translateFilePath, + } +} + +func getJobsTransformers(m *translatePaths, b *bundle.Bundle) ([]*transformer, error) { + var transformers []*transformer = make([]*transformer, 0) + + for key, job := range b.Config.Resources.Jobs { + dir, err := job.ConfigFileDirectory() + if err != nil { + return nil, fmt.Errorf("unable to determine directory for job %s: %w", key, err) + } + + // Do not translate job task paths if using git source + if job.GitSource != nil { + continue + } + + for i := 0; i < len(job.Tasks); i++ { + task := &job.Tasks[i] + transformers = addTransformerForResource(transformers, m, task, dir) + for j := 0; j < len(task.Libraries); j++ { + library := &task.Libraries[j] + transformers = addTransformerForResource(transformers, m, library, dir) + } + } + } + + return transformers, nil +} diff --git a/bundle/config/mutator/translate_paths_pipelines.go b/bundle/config/mutator/translate_paths_pipelines.go new file mode 100644 index 0000000000..65faba019e --- /dev/null +++ b/bundle/config/mutator/translate_paths_pipelines.go @@ -0,0 +1,52 @@ +package mutator + +import ( + "fmt" + + "github.com/databricks/cli/bundle" + "github.com/databricks/databricks-sdk-go/service/pipelines" +) + +func getPipelineTransformers(m *translatePaths, b *bundle.Bundle) ([]*transformer, error) { + var transformers []*transformer = make([]*transformer, 0) + + for key, pipeline := range b.Config.Resources.Pipelines { + dir, err := pipeline.ConfigFileDirectory() + if err != nil { + return nil, fmt.Errorf("unable to determine directory for pipeline %s: %w", key, err) + } + + for i := 0; i < len(pipeline.Libraries); i++ { + library := &pipeline.Libraries[i] + transformers = addTransformerForResource(transformers, m, library, dir) + } + } + + return transformers, nil +} + +func selectLibraryNotebook(resource interface{}, m *translatePaths) *selector { + library, ok := resource.(*pipelines.PipelineLibrary) + if !ok || library.Notebook == nil { + return nil + } + + return &selector{ + &library.Notebook.Path, + "libraries.notebook.path", + m.translateNotebookPath, + } +} + +func selectLibraryFile(resource interface{}, m *translatePaths) *selector { + library, ok := resource.(*pipelines.PipelineLibrary) + if !ok || library.File == nil { + return nil + } + + return &selector{ + &library.File.Path, + "libraries.file.path", + m.translateFilePath, + } +} diff --git a/bundle/config/mutator/translate_paths_test.go b/bundle/config/mutator/translate_paths_test.go index b87f4f6768..c11e847ddb 100644 --- a/bundle/config/mutator/translate_paths_test.go +++ b/bundle/config/mutator/translate_paths_test.go @@ -10,6 +10,7 @@ import ( "github.com/databricks/cli/bundle/config" "github.com/databricks/cli/bundle/config/mutator" "github.com/databricks/cli/bundle/config/resources" + "github.com/databricks/databricks-sdk-go/service/compute" "github.com/databricks/databricks-sdk-go/service/jobs" "github.com/databricks/databricks-sdk-go/service/pipelines" "github.com/stretchr/testify/assert" @@ -103,6 +104,7 @@ func TestTranslatePaths(t *testing.T) { touchNotebookFile(t, filepath.Join(dir, "my_job_notebook.py")) touchNotebookFile(t, filepath.Join(dir, "my_pipeline_notebook.py")) touchEmptyFile(t, filepath.Join(dir, "my_python_file.py")) + touchEmptyFile(t, filepath.Join(dir, "dist", "task.jar")) bundle := &bundle.Bundle{ Config: config.Root{ @@ -122,6 +124,9 @@ func TestTranslatePaths(t *testing.T) { NotebookTask: &jobs.NotebookTask{ NotebookPath: "./my_job_notebook.py", }, + Libraries: []compute.Library{ + {Whl: "./dist/task.whl"}, + }, }, { NotebookTask: &jobs.NotebookTask{ @@ -143,6 +148,22 @@ func TestTranslatePaths(t *testing.T) { PythonFile: "./my_python_file.py", }, }, + { + SparkJarTask: &jobs.SparkJarTask{ + MainClassName: "HelloWorld", + }, + Libraries: []compute.Library{ + {Jar: "./dist/task.jar"}, + }, + }, + { + SparkJarTask: &jobs.SparkJarTask{ + MainClassName: "HelloWorldRemote", + }, + Libraries: []compute.Library{ + {Jar: "dbfs:///bundle/dist/task_remote.jar"}, + }, + }, }, }, }, @@ -194,6 +215,11 @@ func TestTranslatePaths(t *testing.T) { "/bundle/my_job_notebook", bundle.Config.Resources.Jobs["job"].Tasks[0].NotebookTask.NotebookPath, ) + assert.Equal( + t, + "dist/task.whl", + bundle.Config.Resources.Jobs["job"].Tasks[0].Libraries[0].Whl, + ) assert.Equal( t, "/Users/jane.doe@databricks.com/doesnt_exist.py", @@ -209,6 +235,16 @@ func TestTranslatePaths(t *testing.T) { "/bundle/my_python_file.py", bundle.Config.Resources.Jobs["job"].Tasks[4].SparkPythonTask.PythonFile, ) + assert.Equal( + t, + "/bundle/dist/task.jar", + bundle.Config.Resources.Jobs["job"].Tasks[5].Libraries[0].Jar, + ) + assert.Equal( + t, + "dbfs:///bundle/dist/task_remote.jar", + bundle.Config.Resources.Jobs["job"].Tasks[6].Libraries[0].Jar, + ) // Assert that the path in the libraries now refer to the artifact. assert.Equal( @@ -236,6 +272,7 @@ func TestTranslatePaths(t *testing.T) { func TestTranslatePathsInSubdirectories(t *testing.T) { dir := t.TempDir() touchEmptyFile(t, filepath.Join(dir, "job", "my_python_file.py")) + touchEmptyFile(t, filepath.Join(dir, "job", "dist", "task.jar")) touchEmptyFile(t, filepath.Join(dir, "pipeline", "my_python_file.py")) bundle := &bundle.Bundle{ @@ -257,6 +294,14 @@ func TestTranslatePathsInSubdirectories(t *testing.T) { PythonFile: "./my_python_file.py", }, }, + { + SparkJarTask: &jobs.SparkJarTask{ + MainClassName: "HelloWorld", + }, + Libraries: []compute.Library{ + {Jar: "./dist/task.jar"}, + }, + }, }, }, }, @@ -290,6 +335,11 @@ func TestTranslatePathsInSubdirectories(t *testing.T) { "/bundle/job/my_python_file.py", bundle.Config.Resources.Jobs["job"].Tasks[0].SparkPythonTask.PythonFile, ) + assert.Equal( + t, + "/bundle/job/dist/task.jar", + bundle.Config.Resources.Jobs["job"].Tasks[1].Libraries[0].Jar, + ) assert.Equal( t, diff --git a/bundle/config/root.go b/bundle/config/root.go index 1275dab481..99ea33ad6b 100644 --- a/bundle/config/root.go +++ b/bundle/config/root.go @@ -64,7 +64,7 @@ type Root struct { Workspace Workspace `json:"workspace,omitempty"` // Artifacts contains a description of all code artifacts in this bundle. - Artifacts map[string]*Artifact `json:"artifacts,omitempty"` + Artifacts Artifacts `json:"artifacts,omitempty"` // Resources contains a description of all Databricks resources // to deploy in this bundle (e.g. jobs, pipelines, etc.). @@ -113,6 +113,10 @@ func Load(path string) (*Root, error) { // was loaded from in configuration leafs that require it. func (r *Root) SetConfigFilePath(path string) { r.Resources.SetConfigFilePath(path) + if r.Artifacts != nil { + r.Artifacts.SetConfigFilePath(path) + } + if r.Targets != nil { for _, env := range r.Targets { if env == nil { @@ -121,6 +125,9 @@ func (r *Root) SetConfigFilePath(path string) { if env.Resources != nil { env.Resources.SetConfigFilePath(path) } + if env.Artifacts != nil { + env.Artifacts.SetConfigFilePath(path) + } } } } @@ -175,11 +182,17 @@ func (r *Root) Load(path string) error { } func (r *Root) Merge(other *Root) error { + err := r.Sync.Merge(r, other) + if err != nil { + return err + } + other.Sync = Sync{} + // TODO: when hooking into merge semantics, disallow setting path on the target instance. other.Path = "" // Check for safe merge, protecting against duplicate resource identifiers - err := r.Resources.VerifySafeMerge(&other.Resources) + err = r.Resources.VerifySafeMerge(&other.Resources) if err != nil { return err } diff --git a/bundle/config/sync.go b/bundle/config/sync.go index 0580e4c4ff..6ba2603c41 100644 --- a/bundle/config/sync.go +++ b/bundle/config/sync.go @@ -1,5 +1,7 @@ package config +import "path/filepath" + type Sync struct { // Include contains a list of globs evaluated relative to the bundle root path // to explicitly include files that were excluded by the user's gitignore. @@ -11,3 +13,19 @@ type Sync struct { // 2) the `Include` field above. Exclude []string `json:"exclude,omitempty"` } + +func (s *Sync) Merge(root *Root, other *Root) error { + path, err := filepath.Rel(root.Path, other.Path) + if err != nil { + return err + } + for _, include := range other.Sync.Include { + s.Include = append(s.Include, filepath.Join(path, include)) + } + + for _, exclude := range other.Sync.Exclude { + s.Exclude = append(s.Exclude, filepath.Join(path, exclude)) + } + + return nil +} diff --git a/bundle/config/target.go b/bundle/config/target.go index 6a45fdb851..2489efc33d 100644 --- a/bundle/config/target.go +++ b/bundle/config/target.go @@ -23,7 +23,7 @@ type Target struct { Workspace *Workspace `json:"workspace,omitempty"` - Artifacts map[string]*Artifact `json:"artifacts,omitempty"` + Artifacts Artifacts `json:"artifacts,omitempty"` Resources *Resources `json:"resources,omitempty"` diff --git a/bundle/tests/relative_path_with_includes/bundle.yml b/bundle/tests/relative_path_with_includes/bundle.yml new file mode 100644 index 0000000000..36474c7546 --- /dev/null +++ b/bundle/tests/relative_path_with_includes/bundle.yml @@ -0,0 +1,25 @@ +bundle: + name: sync_include + +include: + - "*/*.yml" + +sync: + include: + - ./folder_a/*.* + exclude: + - ./folder_b/*.* + +artifacts: + test_a: + type: whl + path: ./artifact_a + +resources: + jobs: + job_a: + name: "job_a" + tasks: + - task_key: "task_a" + libraries: + - whl: ./dist/job_a.whl diff --git a/bundle/tests/relative_path_with_includes/subfolder/include.yml b/bundle/tests/relative_path_with_includes/subfolder/include.yml new file mode 100644 index 0000000000..597abe3bf1 --- /dev/null +++ b/bundle/tests/relative_path_with_includes/subfolder/include.yml @@ -0,0 +1,20 @@ +sync: + include: + - ./folder_c/*.* + exclude: + - ./folder_d/*.* + +artifacts: + test_b: + type: whl + path: ./artifact_b + + +resources: + jobs: + job_b: + name: "job_b" + tasks: + - task_key: "task_a" + libraries: + - whl: ./dist/job_b.whl diff --git a/bundle/tests/relative_path_with_includes_test.go b/bundle/tests/relative_path_with_includes_test.go new file mode 100644 index 0000000000..c8218062c4 --- /dev/null +++ b/bundle/tests/relative_path_with_includes_test.go @@ -0,0 +1,27 @@ +package config_tests + +import ( + "context" + "testing" + + "github.com/databricks/cli/bundle" + "github.com/databricks/cli/bundle/config/mutator" + "github.com/stretchr/testify/assert" +) + +func TestRelativePathsWithIncludes(t *testing.T) { + b := load(t, "./relative_path_with_includes") + + m := mutator.TranslatePaths() + err := bundle.Apply(context.Background(), b, m) + assert.NoError(t, err) + + assert.Equal(t, "artifact_a", b.Config.Artifacts["test_a"].Path) + assert.Equal(t, "subfolder/artifact_b", b.Config.Artifacts["test_b"].Path) + + assert.ElementsMatch(t, []string{"./folder_a/*.*", "subfolder/folder_c/*.*"}, b.Config.Sync.Include) + assert.ElementsMatch(t, []string{"./folder_b/*.*", "subfolder/folder_d/*.*"}, b.Config.Sync.Exclude) + + assert.Equal(t, "dist/job_a.whl", b.Config.Resources.Jobs["job_a"].Tasks[0].Libraries[0].Whl) + assert.Equal(t, "subfolder/dist/job_b.whl", b.Config.Resources.Jobs["job_b"].Tasks[0].Libraries[0].Whl) +} From 899de692198bccd8d9efa4995241bd38792f75e5 Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Tue, 29 Aug 2023 16:44:19 +0200 Subject: [PATCH 02/11] fixed tests --- bundle/tests/relative_path_with_includes_test.go | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/bundle/tests/relative_path_with_includes_test.go b/bundle/tests/relative_path_with_includes_test.go index c8218062c4..92249c412c 100644 --- a/bundle/tests/relative_path_with_includes_test.go +++ b/bundle/tests/relative_path_with_includes_test.go @@ -2,6 +2,7 @@ package config_tests import ( "context" + "path/filepath" "testing" "github.com/databricks/cli/bundle" @@ -17,11 +18,11 @@ func TestRelativePathsWithIncludes(t *testing.T) { assert.NoError(t, err) assert.Equal(t, "artifact_a", b.Config.Artifacts["test_a"].Path) - assert.Equal(t, "subfolder/artifact_b", b.Config.Artifacts["test_b"].Path) + assert.Equal(t, filepath.Join("subfolder", "artifact_b"), b.Config.Artifacts["test_b"].Path) - assert.ElementsMatch(t, []string{"./folder_a/*.*", "subfolder/folder_c/*.*"}, b.Config.Sync.Include) - assert.ElementsMatch(t, []string{"./folder_b/*.*", "subfolder/folder_d/*.*"}, b.Config.Sync.Exclude) + assert.ElementsMatch(t, []string{"./folder_a/*.*", filepath.Join("subfolder", "folder_c", "*.*")}, b.Config.Sync.Include) + assert.ElementsMatch(t, []string{"./folder_b/*.*", filepath.Join("subfolder", "folder_d", "*.*")}, b.Config.Sync.Exclude) - assert.Equal(t, "dist/job_a.whl", b.Config.Resources.Jobs["job_a"].Tasks[0].Libraries[0].Whl) - assert.Equal(t, "subfolder/dist/job_b.whl", b.Config.Resources.Jobs["job_b"].Tasks[0].Libraries[0].Whl) + assert.Equal(t, filepath.Join("dist", "job_a.whl"), b.Config.Resources.Jobs["job_a"].Tasks[0].Libraries[0].Whl) + assert.Equal(t, filepath.Join("subfolder", "dist", "job_b.whl"), b.Config.Resources.Jobs["job_b"].Tasks[0].Libraries[0].Whl) } From fcf8d160f77475e2dca0d39ee41453f17f1ab87f Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Tue, 29 Aug 2023 16:57:10 +0200 Subject: [PATCH 03/11] fix tests --- bundle/config/mutator/translate_paths_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bundle/config/mutator/translate_paths_test.go b/bundle/config/mutator/translate_paths_test.go index c11e847ddb..dee3a737c2 100644 --- a/bundle/config/mutator/translate_paths_test.go +++ b/bundle/config/mutator/translate_paths_test.go @@ -217,7 +217,7 @@ func TestTranslatePaths(t *testing.T) { ) assert.Equal( t, - "dist/task.whl", + filepath.Join("dist", "task.whl"), bundle.Config.Resources.Jobs["job"].Tasks[0].Libraries[0].Whl, ) assert.Equal( From bab847bdb8ed132d169885d6f068058fc1c41da9 Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Thu, 31 Aug 2023 14:51:05 +0200 Subject: [PATCH 04/11] removed selector logic --- bundle/config/mutator/translate_paths.go | 121 ++++++------------ .../mutator/translate_paths_artifacts.go | 22 ++-- bundle/config/mutator/translate_paths_jobs.go | 51 +++++--- .../mutator/translate_paths_pipelines.go | 30 +++-- 4 files changed, 105 insertions(+), 119 deletions(-) diff --git a/bundle/config/mutator/translate_paths.go b/bundle/config/mutator/translate_paths.go index dfacad6908..e5adf1e02b 100644 --- a/bundle/config/mutator/translate_paths.go +++ b/bundle/config/mutator/translate_paths.go @@ -43,7 +43,7 @@ func (m *translatePaths) Name() string { return "TranslatePaths" } -type rewriteFunc func(b *bundle.Bundle, literal, localPath, remotePath string) (string, error) +type rewriteFunc func(literal, localFullPath, localRelPath, remotePath string) (string, error) // rewritePath converts a given relative path from the loaded config to a new path based on the passed rewriting function // @@ -83,19 +83,19 @@ func (m *translatePaths) rewritePath( } // Remote path must be relative to the bundle root. - remotePath, err := filepath.Rel(b.Config.Path, localPath) + localRelPath, err := filepath.Rel(b.Config.Path, localPath) if err != nil { return err } - if strings.HasPrefix(remotePath, "..") { + if strings.HasPrefix(localRelPath, "..") { return fmt.Errorf("path %s is not contained in bundle root path", localPath) } // Prefix remote path with its remote root path. - remotePath = path.Join(b.Config.Workspace.FilesPath, filepath.ToSlash(remotePath)) + remotePath := path.Join(b.Config.Workspace.FilesPath, filepath.ToSlash(localRelPath)) // Convert local path into workspace path via specified function. - interp, err := fn(b, *p, localPath, filepath.ToSlash(remotePath)) + interp, err := fn(*p, localPath, localRelPath, filepath.ToSlash(remotePath)) if err != nil { return err } @@ -105,131 +105,88 @@ func (m *translatePaths) rewritePath( return nil } -func (m *translatePaths) translateNotebookPath(b *bundle.Bundle, literal, localPath, remotePath string) (string, error) { - nb, _, err := notebook.Detect(localPath) +func translateNotebookPath(literal, localFullPath, localRelPath, remotePath string) (string, error) { + nb, _, err := notebook.Detect(localFullPath) if os.IsNotExist(err) { return "", fmt.Errorf("notebook %s not found", literal) } if err != nil { - return "", fmt.Errorf("unable to determine if %s is a notebook: %w", localPath, err) + return "", fmt.Errorf("unable to determine if %s is a notebook: %w", localFullPath, err) } if !nb { - return "", ErrIsNotNotebook{localPath} + return "", ErrIsNotNotebook{localFullPath} } // Upon import, notebooks are stripped of their extension. - return strings.TrimSuffix(remotePath, filepath.Ext(localPath)), nil + return strings.TrimSuffix(remotePath, filepath.Ext(localFullPath)), nil } -func (m *translatePaths) translateFilePath(b *bundle.Bundle, literal, localPath, remotePath string) (string, error) { - nb, _, err := notebook.Detect(localPath) +func translateFilePath(literal, localFullPath, localRelPath, remotePath string) (string, error) { + nb, _, err := notebook.Detect(localFullPath) if os.IsNotExist(err) { return "", fmt.Errorf("file %s not found", literal) } if err != nil { - return "", fmt.Errorf("unable to determine if %s is not a notebook: %w", localPath, err) + return "", fmt.Errorf("unable to determine if %s is not a notebook: %w", localFullPath, err) } if nb { - return "", ErrIsNotebook{localPath} + return "", ErrIsNotebook{localFullPath} } return remotePath, nil } -func (m *translatePaths) translateToBundleRootRelativePath(b *bundle.Bundle, literal, localPath, remotePath string) (string, error) { - return filepath.Rel(b.Config.Path, localPath) +func translateNoOp(literal, localPath, localFullPath, remotePath string) (string, error) { + return localFullPath, nil } type transformer struct { // A directory path relative to which `path` will be transformed dir string - // A path to transform path *string - // Name of the config property where the path string is coming from configPath string - // A function that performs the actual rewriting logic. fn rewriteFunc } -type transformerFactory func(*translatePaths, *bundle.Bundle) ([]*transformer, error) -type selector struct { - // A path to transform - path *string - // Name of the config property where the path string is coming from - configPath string +type transformFunc func(resource any, dir string) *transformer - // A function that performs the actual rewriting logic. - fn rewriteFunc +var transformers []func(*translatePaths, *bundle.Bundle) error = []func(*translatePaths, *bundle.Bundle) error{ + applyJobsTransformers, + applyPipelineTransformers, + applyArtifactTransformers, } -type selectorFunc func(resource interface{}, m *translatePaths) *selector -// List of available transformer factories. All of them will be called to get the list of transformers to execute. -var transformerFactories []transformerFactory = []transformerFactory{ - getJobsTransformers, - getPipelineTransformers, - getArtifactsTransformers, -} - -// List of selector functions which are used to identify if the resource passed -// Should have it's path transformed. -// If it needs to be transformed, selector returns a reference to string to be transformed -// And a function to apply for this string -var selectors []selectorFunc = []selectorFunc{ - selectNotebookTask, - selectSparkTask, - selectLibraryNotebook, - selectLibraryFile, - selectArtifactPath, - selectWhlLibrary, - selectJarLibrary, -} - -// Appends the first matched transfomer for the given resource if it matches any if selectors defined -func addTransformerForResource(transformers []*transformer, m *translatePaths, resource interface{}, dir string) []*transformer { - for _, selector := range selectors { - s := selector(resource, m) - if s != nil { - transformers = append(transformers, &transformer{dir, s.path, s.configPath, s.fn}) - break +// Apply all matches transformers for the given resource +func applyTransformers(funcs []transformFunc, m *translatePaths, b *bundle.Bundle, resource any, dir string) error { + for _, transformFn := range funcs { + transformer := transformFn(resource, dir) + if transformer == nil { + continue } - } - return transformers -} - -// Returns a list of path transformers which are applied to specific configuration section based on -// defined selectors -func (m *translatePaths) getTransformers(b *bundle.Bundle) ([]*transformer, error) { - var transformers []*transformer = make([]*transformer, 0) - for _, get := range transformerFactories { - toAdd, err := get(m, b) + err := m.rewritePath(transformer.dir, b, transformer.path, transformer.fn) if err != nil { - return nil, err + if target := (&ErrIsNotebook{}); errors.As(err, target) { + return fmt.Errorf(`expected a file for "%s" but got a notebook: %w`, transformer.configPath, target) + } + if target := (&ErrIsNotNotebook{}); errors.As(err, target) { + return fmt.Errorf(`expected a notebook for "%s" but got a file: %w`, transformer.configPath, target) + } + return err } - transformers = append(transformers, toAdd...) } - return transformers, nil + + return nil } func (m *translatePaths) Apply(_ context.Context, b *bundle.Bundle) error { m.seen = make(map[string]string) - transfomers, err := m.getTransformers(b) - if err != nil { - return err - } - - for _, transformer := range transfomers { - err := m.rewritePath(transformer.dir, b, transformer.path, transformer.fn) + for _, apply := range transformers { + err := apply(m, b) if err != nil { - if target := (&ErrIsNotebook{}); errors.As(err, target) { - return fmt.Errorf(`expected a file for "%s" but got a notebook: %w`, transformer.configPath, target) - } - if target := (&ErrIsNotNotebook{}); errors.As(err, target) { - return fmt.Errorf(`expected a notebook for "%s" but got a file: %w`, transformer.configPath, target) - } return err } } diff --git a/bundle/config/mutator/translate_paths_artifacts.go b/bundle/config/mutator/translate_paths_artifacts.go index 5743bbacac..1a3e5c3949 100644 --- a/bundle/config/mutator/translate_paths_artifacts.go +++ b/bundle/config/mutator/translate_paths_artifacts.go @@ -7,30 +7,36 @@ import ( "github.com/databricks/cli/bundle/config" ) -func selectArtifactPath(resource interface{}, m *translatePaths) *selector { +func transformArtifactPath(resource any, dir string) *transformer { artifact, ok := resource.(*config.Artifact) if !ok { return nil } - return &selector{ + return &transformer{ + dir, &artifact.Path, "artifacts.path", - m.translateToBundleRootRelativePath, + translateNoOp, } } -func getArtifactsTransformers(m *translatePaths, b *bundle.Bundle) ([]*transformer, error) { - var transformers []*transformer = make([]*transformer, 0) +var artifactTransformers []transformFunc = []transformFunc{ + transformArtifactPath, +} +func applyArtifactTransformers(m *translatePaths, b *bundle.Bundle) error { for key, artifact := range b.Config.Artifacts { dir, err := artifact.ConfigFileDirectory() if err != nil { - return nil, fmt.Errorf("unable to determine directory for artifact %s: %w", key, err) + return fmt.Errorf("unable to determine directory for artifact %s: %w", key, err) } - transformers = addTransformerForResource(transformers, m, artifact, dir) + err = applyTransformers(artifactTransformers, m, b, artifact, dir) + if err != nil { + return err + } } - return transformers, nil + return nil } diff --git a/bundle/config/mutator/translate_paths_jobs.go b/bundle/config/mutator/translate_paths_jobs.go index 933e10310e..324febdded 100644 --- a/bundle/config/mutator/translate_paths_jobs.go +++ b/bundle/config/mutator/translate_paths_jobs.go @@ -8,65 +8,74 @@ import ( "github.com/databricks/databricks-sdk-go/service/jobs" ) -func selectNotebookTask(resource interface{}, m *translatePaths) *selector { +func transformNotebookTask(resource any, dir string) *transformer { task, ok := resource.(*jobs.Task) if !ok || task.NotebookTask == nil { return nil } - return &selector{ + return &transformer{ + dir, &task.NotebookTask.NotebookPath, "tasks.notebook_task.notebook_path", - m.translateNotebookPath, + translateNotebookPath, } } -func selectSparkTask(resource interface{}, m *translatePaths) *selector { +func transformSparkTask(resource any, dir string) *transformer { task, ok := resource.(*jobs.Task) if !ok || task.SparkPythonTask == nil { return nil } - return &selector{ + return &transformer{ + dir, &task.SparkPythonTask.PythonFile, "tasks.spark_python_task.python_file", - m.translateFilePath, + translateFilePath, } } -func selectWhlLibrary(resource interface{}, m *translatePaths) *selector { +func transformWhlLibrary(resource any, dir string) *transformer { library, ok := resource.(*compute.Library) if !ok || library.Whl == "" { return nil } - return &selector{ + return &transformer{ + dir, &library.Whl, "libraries.whl", - m.translateToBundleRootRelativePath, + translateNoOp, } } -func selectJarLibrary(resource interface{}, m *translatePaths) *selector { +func transformJarLibrary(resource any, dir string) *transformer { library, ok := resource.(*compute.Library) if !ok || library.Jar == "" { return nil } - return &selector{ + return &transformer{ + dir, &library.Jar, "libraries.jar", - m.translateFilePath, + translateFilePath, } } -func getJobsTransformers(m *translatePaths, b *bundle.Bundle) ([]*transformer, error) { - var transformers []*transformer = make([]*transformer, 0) +var jobTransformers []transformFunc = []transformFunc{ + transformNotebookTask, + transformSparkTask, + transformWhlLibrary, + transformJarLibrary, +} +func applyJobsTransformers(m *translatePaths, b *bundle.Bundle) error { for key, job := range b.Config.Resources.Jobs { dir, err := job.ConfigFileDirectory() if err != nil { - return nil, fmt.Errorf("unable to determine directory for job %s: %w", key, err) + return fmt.Errorf("unable to determine directory for job %s: %w", key, err) } // Do not translate job task paths if using git source @@ -76,13 +85,19 @@ func getJobsTransformers(m *translatePaths, b *bundle.Bundle) ([]*transformer, e for i := 0; i < len(job.Tasks); i++ { task := &job.Tasks[i] - transformers = addTransformerForResource(transformers, m, task, dir) + err := applyTransformers(jobTransformers, m, b, task, dir) + if err != nil { + return err + } for j := 0; j < len(task.Libraries); j++ { library := &task.Libraries[j] - transformers = addTransformerForResource(transformers, m, library, dir) + err := applyTransformers(jobTransformers, m, b, library, dir) + if err != nil { + return err + } } } } - return transformers, nil + return nil } diff --git a/bundle/config/mutator/translate_paths_pipelines.go b/bundle/config/mutator/translate_paths_pipelines.go index 65faba019e..8ce2a70d64 100644 --- a/bundle/config/mutator/translate_paths_pipelines.go +++ b/bundle/config/mutator/translate_paths_pipelines.go @@ -7,46 +7,54 @@ import ( "github.com/databricks/databricks-sdk-go/service/pipelines" ) -func getPipelineTransformers(m *translatePaths, b *bundle.Bundle) ([]*transformer, error) { - var transformers []*transformer = make([]*transformer, 0) +var pipelineTransformers []transformFunc = []transformFunc{ + transformLibraryNotebook, + transformLibraryFile, +} +func applyPipelineTransformers(m *translatePaths, b *bundle.Bundle) error { for key, pipeline := range b.Config.Resources.Pipelines { dir, err := pipeline.ConfigFileDirectory() if err != nil { - return nil, fmt.Errorf("unable to determine directory for pipeline %s: %w", key, err) + return fmt.Errorf("unable to determine directory for pipeline %s: %w", key, err) } for i := 0; i < len(pipeline.Libraries); i++ { library := &pipeline.Libraries[i] - transformers = addTransformerForResource(transformers, m, library, dir) + err := applyTransformers(pipelineTransformers, m, b, library, dir) + if err != nil { + return err + } } } - return transformers, nil + return nil } -func selectLibraryNotebook(resource interface{}, m *translatePaths) *selector { +func transformLibraryNotebook(resource any, dir string) *transformer { library, ok := resource.(*pipelines.PipelineLibrary) if !ok || library.Notebook == nil { return nil } - return &selector{ + return &transformer{ + dir, &library.Notebook.Path, "libraries.notebook.path", - m.translateNotebookPath, + translateNotebookPath, } } -func selectLibraryFile(resource interface{}, m *translatePaths) *selector { +func transformLibraryFile(resource any, dir string) *transformer { library, ok := resource.(*pipelines.PipelineLibrary) if !ok || library.File == nil { return nil } - return &selector{ + return &transformer{ + dir, &library.File.Path, "libraries.file.path", - m.translateFilePath, + translateFilePath, } } From ab1348ac7f77ceb664aeba7005e1c5b411395911 Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Thu, 31 Aug 2023 14:52:57 +0200 Subject: [PATCH 05/11] small cleanup --- bundle/config/artifact.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/bundle/config/artifact.go b/bundle/config/artifact.go index 6e34bf9814..32837a74f0 100644 --- a/bundle/config/artifact.go +++ b/bundle/config/artifact.go @@ -15,8 +15,7 @@ import ( type Artifacts map[string]*Artifact func (artifacts Artifacts) SetConfigFilePath(path string) { - for k := range artifacts { - artifact := artifacts[k] + for _, artifact := range artifacts { artifact.ConfigFilePath = path } } From 6b18793f59599f1ca4b0107d6c6a6808dcc4b6e2 Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Thu, 31 Aug 2023 15:04:26 +0200 Subject: [PATCH 06/11] moved paths struct --- bundle/config/artifact.go | 4 +-- bundle/config/mutator/trampoline_test.go | 3 +- bundle/config/mutator/translate_paths_test.go | 29 ++++++++++--------- .../{resources/pkg.go => paths/paths.go} | 2 +- bundle/config/resources/job.go | 3 +- bundle/config/resources/mlflow_experiment.go | 7 +++-- bundle/config/resources/mlflow_model.go | 7 +++-- bundle/config/resources/pipeline.go | 7 +++-- bundle/config/resources_test.go | 19 ++++++------ bundle/python/transform_test.go | 3 +- 10 files changed, 49 insertions(+), 35 deletions(-) rename bundle/config/{resources/pkg.go => paths/paths.go} (95%) diff --git a/bundle/config/artifact.go b/bundle/config/artifact.go index 32837a74f0..1955e265db 100644 --- a/bundle/config/artifact.go +++ b/bundle/config/artifact.go @@ -8,7 +8,7 @@ import ( "path" "strings" - "github.com/databricks/cli/bundle/config/resources" + "github.com/databricks/cli/bundle/config/paths" "github.com/databricks/databricks-sdk-go/service/compute" ) @@ -44,7 +44,7 @@ type Artifact struct { Files []ArtifactFile `json:"files"` BuildCommand string `json:"build"` - resources.Paths + paths.Paths } func (a *Artifact) Build(ctx context.Context) ([]byte, error) { diff --git a/bundle/config/mutator/trampoline_test.go b/bundle/config/mutator/trampoline_test.go index e523250e09..aec58618cb 100644 --- a/bundle/config/mutator/trampoline_test.go +++ b/bundle/config/mutator/trampoline_test.go @@ -9,6 +9,7 @@ import ( "github.com/databricks/cli/bundle" "github.com/databricks/cli/bundle/config" + "github.com/databricks/cli/bundle/config/paths" "github.com/databricks/cli/bundle/config/resources" "github.com/databricks/databricks-sdk-go/service/jobs" "github.com/stretchr/testify/require" @@ -64,7 +65,7 @@ func TestGenerateTrampoline(t *testing.T) { Resources: config.Resources{ Jobs: map[string]*resources.Job{ "test": { - Paths: resources.Paths{ + Paths: paths.Paths{ ConfigFilePath: tmpDir, }, JobSettings: &jobs.JobSettings{ diff --git a/bundle/config/mutator/translate_paths_test.go b/bundle/config/mutator/translate_paths_test.go index dee3a737c2..e7ac5e8afd 100644 --- a/bundle/config/mutator/translate_paths_test.go +++ b/bundle/config/mutator/translate_paths_test.go @@ -9,6 +9,7 @@ import ( "github.com/databricks/cli/bundle" "github.com/databricks/cli/bundle/config" "github.com/databricks/cli/bundle/config/mutator" + "github.com/databricks/cli/bundle/config/paths" "github.com/databricks/cli/bundle/config/resources" "github.com/databricks/databricks-sdk-go/service/compute" "github.com/databricks/databricks-sdk-go/service/jobs" @@ -44,7 +45,7 @@ func TestTranslatePathsSkippedWithGitSource(t *testing.T) { Jobs: map[string]*resources.Job{ "job": { - Paths: resources.Paths{ + Paths: paths.Paths{ ConfigFilePath: filepath.Join(dir, "resource.yml"), }, JobSettings: &jobs.JobSettings{ @@ -115,7 +116,7 @@ func TestTranslatePaths(t *testing.T) { Resources: config.Resources{ Jobs: map[string]*resources.Job{ "job": { - Paths: resources.Paths{ + Paths: paths.Paths{ ConfigFilePath: filepath.Join(dir, "resource.yml"), }, JobSettings: &jobs.JobSettings{ @@ -170,7 +171,7 @@ func TestTranslatePaths(t *testing.T) { }, Pipelines: map[string]*resources.Pipeline{ "pipeline": { - Paths: resources.Paths{ + Paths: paths.Paths{ ConfigFilePath: filepath.Join(dir, "resource.yml"), }, PipelineSpec: &pipelines.PipelineSpec{ @@ -284,7 +285,7 @@ func TestTranslatePathsInSubdirectories(t *testing.T) { Resources: config.Resources{ Jobs: map[string]*resources.Job{ "job": { - Paths: resources.Paths{ + Paths: paths.Paths{ ConfigFilePath: filepath.Join(dir, "job/resource.yml"), }, JobSettings: &jobs.JobSettings{ @@ -308,7 +309,7 @@ func TestTranslatePathsInSubdirectories(t *testing.T) { }, Pipelines: map[string]*resources.Pipeline{ "pipeline": { - Paths: resources.Paths{ + Paths: paths.Paths{ ConfigFilePath: filepath.Join(dir, "pipeline/resource.yml"), }, @@ -360,7 +361,7 @@ func TestTranslatePathsOutsideBundleRoot(t *testing.T) { Resources: config.Resources{ Jobs: map[string]*resources.Job{ "job": { - Paths: resources.Paths{ + Paths: paths.Paths{ ConfigFilePath: filepath.Join(dir, "../resource.yml"), }, JobSettings: &jobs.JobSettings{ @@ -391,7 +392,7 @@ func TestJobNotebookDoesNotExistError(t *testing.T) { Resources: config.Resources{ Jobs: map[string]*resources.Job{ "job": { - Paths: resources.Paths{ + Paths: paths.Paths{ ConfigFilePath: filepath.Join(dir, "fake.yml"), }, JobSettings: &jobs.JobSettings{ @@ -422,7 +423,7 @@ func TestJobFileDoesNotExistError(t *testing.T) { Resources: config.Resources{ Jobs: map[string]*resources.Job{ "job": { - Paths: resources.Paths{ + Paths: paths.Paths{ ConfigFilePath: filepath.Join(dir, "fake.yml"), }, JobSettings: &jobs.JobSettings{ @@ -453,7 +454,7 @@ func TestPipelineNotebookDoesNotExistError(t *testing.T) { Resources: config.Resources{ Pipelines: map[string]*resources.Pipeline{ "pipeline": { - Paths: resources.Paths{ + Paths: paths.Paths{ ConfigFilePath: filepath.Join(dir, "fake.yml"), }, PipelineSpec: &pipelines.PipelineSpec{ @@ -484,7 +485,7 @@ func TestPipelineFileDoesNotExistError(t *testing.T) { Resources: config.Resources{ Pipelines: map[string]*resources.Pipeline{ "pipeline": { - Paths: resources.Paths{ + Paths: paths.Paths{ ConfigFilePath: filepath.Join(dir, "fake.yml"), }, PipelineSpec: &pipelines.PipelineSpec{ @@ -519,7 +520,7 @@ func TestJobSparkPythonTaskWithNotebookSourceError(t *testing.T) { Resources: config.Resources{ Jobs: map[string]*resources.Job{ "job": { - Paths: resources.Paths{ + Paths: paths.Paths{ ConfigFilePath: filepath.Join(dir, "resource.yml"), }, JobSettings: &jobs.JobSettings{ @@ -554,7 +555,7 @@ func TestJobNotebookTaskWithFileSourceError(t *testing.T) { Resources: config.Resources{ Jobs: map[string]*resources.Job{ "job": { - Paths: resources.Paths{ + Paths: paths.Paths{ ConfigFilePath: filepath.Join(dir, "resource.yml"), }, JobSettings: &jobs.JobSettings{ @@ -589,7 +590,7 @@ func TestPipelineNotebookLibraryWithFileSourceError(t *testing.T) { Resources: config.Resources{ Pipelines: map[string]*resources.Pipeline{ "pipeline": { - Paths: resources.Paths{ + Paths: paths.Paths{ ConfigFilePath: filepath.Join(dir, "resource.yml"), }, PipelineSpec: &pipelines.PipelineSpec{ @@ -624,7 +625,7 @@ func TestPipelineFileLibraryWithNotebookSourceError(t *testing.T) { Resources: config.Resources{ Pipelines: map[string]*resources.Pipeline{ "pipeline": { - Paths: resources.Paths{ + Paths: paths.Paths{ ConfigFilePath: filepath.Join(dir, "resource.yml"), }, PipelineSpec: &pipelines.PipelineSpec{ diff --git a/bundle/config/resources/pkg.go b/bundle/config/paths/paths.go similarity index 95% rename from bundle/config/resources/pkg.go rename to bundle/config/paths/paths.go index 5cf54a06ba..c2cbcb7dd9 100644 --- a/bundle/config/resources/pkg.go +++ b/bundle/config/paths/paths.go @@ -1,4 +1,4 @@ -package resources +package paths import ( "fmt" diff --git a/bundle/config/resources/job.go b/bundle/config/resources/job.go index 6200062a80..66705afb2f 100644 --- a/bundle/config/resources/job.go +++ b/bundle/config/resources/job.go @@ -1,6 +1,7 @@ package resources import ( + "github.com/databricks/cli/bundle/config/paths" "github.com/databricks/databricks-sdk-go/service/jobs" "github.com/imdario/mergo" ) @@ -9,7 +10,7 @@ type Job struct { ID string `json:"id,omitempty" bundle:"readonly"` Permissions []Permission `json:"permissions,omitempty"` - Paths + paths.Paths *jobs.JobSettings } diff --git a/bundle/config/resources/mlflow_experiment.go b/bundle/config/resources/mlflow_experiment.go index ebef039a8f..d843cf226b 100644 --- a/bundle/config/resources/mlflow_experiment.go +++ b/bundle/config/resources/mlflow_experiment.go @@ -1,11 +1,14 @@ package resources -import "github.com/databricks/databricks-sdk-go/service/ml" +import ( + "github.com/databricks/cli/bundle/config/paths" + "github.com/databricks/databricks-sdk-go/service/ml" +) type MlflowExperiment struct { Permissions []Permission `json:"permissions,omitempty"` - Paths + paths.Paths *ml.Experiment } diff --git a/bundle/config/resources/mlflow_model.go b/bundle/config/resources/mlflow_model.go index 31c72f6b02..92617c95a3 100644 --- a/bundle/config/resources/mlflow_model.go +++ b/bundle/config/resources/mlflow_model.go @@ -1,11 +1,14 @@ package resources -import "github.com/databricks/databricks-sdk-go/service/ml" +import ( + "github.com/databricks/cli/bundle/config/paths" + "github.com/databricks/databricks-sdk-go/service/ml" +) type MlflowModel struct { Permissions []Permission `json:"permissions,omitempty"` - Paths + paths.Paths *ml.Model } diff --git a/bundle/config/resources/pipeline.go b/bundle/config/resources/pipeline.go index 96efc2c4fd..d3a51c5754 100644 --- a/bundle/config/resources/pipeline.go +++ b/bundle/config/resources/pipeline.go @@ -1,12 +1,15 @@ package resources -import "github.com/databricks/databricks-sdk-go/service/pipelines" +import ( + "github.com/databricks/cli/bundle/config/paths" + "github.com/databricks/databricks-sdk-go/service/pipelines" +) type Pipeline struct { ID string `json:"id,omitempty" bundle:"readonly"` Permissions []Permission `json:"permissions,omitempty"` - Paths + paths.Paths *pipelines.PipelineSpec } diff --git a/bundle/config/resources_test.go b/bundle/config/resources_test.go index 63285bf94b..82cb9f4549 100644 --- a/bundle/config/resources_test.go +++ b/bundle/config/resources_test.go @@ -3,6 +3,7 @@ package config import ( "testing" + "github.com/databricks/cli/bundle/config/paths" "github.com/databricks/cli/bundle/config/resources" "github.com/stretchr/testify/assert" ) @@ -11,21 +12,21 @@ func TestVerifyUniqueResourceIdentifiers(t *testing.T) { r := Resources{ Jobs: map[string]*resources.Job{ "foo": { - Paths: resources.Paths{ + Paths: paths.Paths{ ConfigFilePath: "foo.yml", }, }, }, Models: map[string]*resources.MlflowModel{ "bar": { - Paths: resources.Paths{ + Paths: paths.Paths{ ConfigFilePath: "bar.yml", }, }, }, Experiments: map[string]*resources.MlflowExperiment{ "foo": { - Paths: resources.Paths{ + Paths: paths.Paths{ ConfigFilePath: "foo2.yml", }, }, @@ -39,14 +40,14 @@ func TestVerifySafeMerge(t *testing.T) { r := Resources{ Jobs: map[string]*resources.Job{ "foo": { - Paths: resources.Paths{ + Paths: paths.Paths{ ConfigFilePath: "foo.yml", }, }, }, Models: map[string]*resources.MlflowModel{ "bar": { - Paths: resources.Paths{ + Paths: paths.Paths{ ConfigFilePath: "bar.yml", }, }, @@ -55,7 +56,7 @@ func TestVerifySafeMerge(t *testing.T) { other := Resources{ Pipelines: map[string]*resources.Pipeline{ "foo": { - Paths: resources.Paths{ + Paths: paths.Paths{ ConfigFilePath: "foo2.yml", }, }, @@ -69,14 +70,14 @@ func TestVerifySafeMergeForSameResourceType(t *testing.T) { r := Resources{ Jobs: map[string]*resources.Job{ "foo": { - Paths: resources.Paths{ + Paths: paths.Paths{ ConfigFilePath: "foo.yml", }, }, }, Models: map[string]*resources.MlflowModel{ "bar": { - Paths: resources.Paths{ + Paths: paths.Paths{ ConfigFilePath: "bar.yml", }, }, @@ -85,7 +86,7 @@ func TestVerifySafeMergeForSameResourceType(t *testing.T) { other := Resources{ Jobs: map[string]*resources.Job{ "foo": { - Paths: resources.Paths{ + Paths: paths.Paths{ ConfigFilePath: "foo2.yml", }, }, diff --git a/bundle/python/transform_test.go b/bundle/python/transform_test.go index fb2c23e42d..a9f57db8ef 100644 --- a/bundle/python/transform_test.go +++ b/bundle/python/transform_test.go @@ -7,6 +7,7 @@ import ( "github.com/databricks/cli/bundle" "github.com/databricks/cli/bundle/config" + "github.com/databricks/cli/bundle/config/paths" "github.com/databricks/cli/bundle/config/resources" "github.com/databricks/databricks-sdk-go/service/jobs" "github.com/stretchr/testify/require" @@ -112,7 +113,7 @@ func TestNoPanicWithNoPythonWheelTasks(t *testing.T) { Resources: config.Resources{ Jobs: map[string]*resources.Job{ "test": { - Paths: resources.Paths{ + Paths: paths.Paths{ ConfigFilePath: tmpDir, }, JobSettings: &jobs.JobSettings{ From 04a4804476c01e537ef7fa9db9833e3876f6d2fa Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Mon, 4 Sep 2023 11:37:23 +0200 Subject: [PATCH 07/11] Make applyTransformers a member function --- bundle/config/mutator/translate_paths.go | 2 +- bundle/config/mutator/translate_paths_artifacts.go | 2 +- bundle/config/mutator/translate_paths_jobs.go | 4 ++-- bundle/config/mutator/translate_paths_pipelines.go | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/bundle/config/mutator/translate_paths.go b/bundle/config/mutator/translate_paths.go index e5adf1e02b..732e5fac38 100644 --- a/bundle/config/mutator/translate_paths.go +++ b/bundle/config/mutator/translate_paths.go @@ -159,7 +159,7 @@ var transformers []func(*translatePaths, *bundle.Bundle) error = []func(*transla } // Apply all matches transformers for the given resource -func applyTransformers(funcs []transformFunc, m *translatePaths, b *bundle.Bundle, resource any, dir string) error { +func (m *translatePaths) applyTransformers(funcs []transformFunc, b *bundle.Bundle, resource any, dir string) error { for _, transformFn := range funcs { transformer := transformFn(resource, dir) if transformer == nil { diff --git a/bundle/config/mutator/translate_paths_artifacts.go b/bundle/config/mutator/translate_paths_artifacts.go index 1a3e5c3949..c31b95192c 100644 --- a/bundle/config/mutator/translate_paths_artifacts.go +++ b/bundle/config/mutator/translate_paths_artifacts.go @@ -32,7 +32,7 @@ func applyArtifactTransformers(m *translatePaths, b *bundle.Bundle) error { return fmt.Errorf("unable to determine directory for artifact %s: %w", key, err) } - err = applyTransformers(artifactTransformers, m, b, artifact, dir) + err = m.applyTransformers(artifactTransformers, b, artifact, dir) if err != nil { return err } diff --git a/bundle/config/mutator/translate_paths_jobs.go b/bundle/config/mutator/translate_paths_jobs.go index 324febdded..a069dfcba4 100644 --- a/bundle/config/mutator/translate_paths_jobs.go +++ b/bundle/config/mutator/translate_paths_jobs.go @@ -85,13 +85,13 @@ func applyJobsTransformers(m *translatePaths, b *bundle.Bundle) error { for i := 0; i < len(job.Tasks); i++ { task := &job.Tasks[i] - err := applyTransformers(jobTransformers, m, b, task, dir) + err := m.applyTransformers(jobTransformers, b, task, dir) if err != nil { return err } for j := 0; j < len(task.Libraries); j++ { library := &task.Libraries[j] - err := applyTransformers(jobTransformers, m, b, library, dir) + err := m.applyTransformers(jobTransformers, b, library, dir) if err != nil { return err } diff --git a/bundle/config/mutator/translate_paths_pipelines.go b/bundle/config/mutator/translate_paths_pipelines.go index 8ce2a70d64..6d5569d70d 100644 --- a/bundle/config/mutator/translate_paths_pipelines.go +++ b/bundle/config/mutator/translate_paths_pipelines.go @@ -21,7 +21,7 @@ func applyPipelineTransformers(m *translatePaths, b *bundle.Bundle) error { for i := 0; i < len(pipeline.Libraries); i++ { library := &pipeline.Libraries[i] - err := applyTransformers(pipelineTransformers, m, b, library, dir) + err := m.applyTransformers(pipelineTransformers, b, library, dir) if err != nil { return err } From 20c79bf6329d12baefafd782c7f8bc7293a56920 Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Mon, 4 Sep 2023 11:40:21 +0200 Subject: [PATCH 08/11] Inline --- bundle/config/mutator/translate_paths.go | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/bundle/config/mutator/translate_paths.go b/bundle/config/mutator/translate_paths.go index 732e5fac38..b324e8e96d 100644 --- a/bundle/config/mutator/translate_paths.go +++ b/bundle/config/mutator/translate_paths.go @@ -152,12 +152,6 @@ type transformer struct { type transformFunc func(resource any, dir string) *transformer -var transformers []func(*translatePaths, *bundle.Bundle) error = []func(*translatePaths, *bundle.Bundle) error{ - applyJobsTransformers, - applyPipelineTransformers, - applyArtifactTransformers, -} - // Apply all matches transformers for the given resource func (m *translatePaths) applyTransformers(funcs []transformFunc, b *bundle.Bundle, resource any, dir string) error { for _, transformFn := range funcs { @@ -184,8 +178,12 @@ func (m *translatePaths) applyTransformers(funcs []transformFunc, b *bundle.Bund func (m *translatePaths) Apply(_ context.Context, b *bundle.Bundle) error { m.seen = make(map[string]string) - for _, apply := range transformers { - err := apply(m, b) + for _, fn := range []func(*translatePaths, *bundle.Bundle) error{ + applyJobsTransformers, + applyPipelineTransformers, + applyArtifactTransformers, + } { + err := fn(m, b) if err != nil { return err } From 57ffb07db4267529d2bcf2ba30c5827b5473fef8 Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Mon, 4 Sep 2023 11:40:33 +0200 Subject: [PATCH 09/11] Singular --- bundle/config/mutator/translate_paths.go | 2 +- bundle/config/mutator/translate_paths_jobs.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/bundle/config/mutator/translate_paths.go b/bundle/config/mutator/translate_paths.go index b324e8e96d..bdd5ce8e40 100644 --- a/bundle/config/mutator/translate_paths.go +++ b/bundle/config/mutator/translate_paths.go @@ -179,7 +179,7 @@ func (m *translatePaths) Apply(_ context.Context, b *bundle.Bundle) error { m.seen = make(map[string]string) for _, fn := range []func(*translatePaths, *bundle.Bundle) error{ - applyJobsTransformers, + applyJobTransformers, applyPipelineTransformers, applyArtifactTransformers, } { diff --git a/bundle/config/mutator/translate_paths_jobs.go b/bundle/config/mutator/translate_paths_jobs.go index a069dfcba4..a5c78126b9 100644 --- a/bundle/config/mutator/translate_paths_jobs.go +++ b/bundle/config/mutator/translate_paths_jobs.go @@ -71,7 +71,7 @@ var jobTransformers []transformFunc = []transformFunc{ transformJarLibrary, } -func applyJobsTransformers(m *translatePaths, b *bundle.Bundle) error { +func applyJobTransformers(m *translatePaths, b *bundle.Bundle) error { for key, job := range b.Config.Resources.Jobs { dir, err := job.ConfigFileDirectory() if err != nil { From b540cb81c4e137f2bc7b75cea4e5ba1af5dd73b5 Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Mon, 4 Sep 2023 11:43:48 +0200 Subject: [PATCH 10/11] More inline --- .../mutator/translate_paths_artifacts.go | 8 ++-- bundle/config/mutator/translate_paths_jobs.go | 14 +++--- .../mutator/translate_paths_pipelines.go | 48 +++++++++---------- 3 files changed, 35 insertions(+), 35 deletions(-) diff --git a/bundle/config/mutator/translate_paths_artifacts.go b/bundle/config/mutator/translate_paths_artifacts.go index c31b95192c..91e8397cb7 100644 --- a/bundle/config/mutator/translate_paths_artifacts.go +++ b/bundle/config/mutator/translate_paths_artifacts.go @@ -21,11 +21,11 @@ func transformArtifactPath(resource any, dir string) *transformer { } } -var artifactTransformers []transformFunc = []transformFunc{ - transformArtifactPath, -} - func applyArtifactTransformers(m *translatePaths, b *bundle.Bundle) error { + artifactTransformers := []transformFunc{ + transformArtifactPath, + } + for key, artifact := range b.Config.Artifacts { dir, err := artifact.ConfigFileDirectory() if err != nil { diff --git a/bundle/config/mutator/translate_paths_jobs.go b/bundle/config/mutator/translate_paths_jobs.go index a5c78126b9..b94df5e2e6 100644 --- a/bundle/config/mutator/translate_paths_jobs.go +++ b/bundle/config/mutator/translate_paths_jobs.go @@ -64,14 +64,14 @@ func transformJarLibrary(resource any, dir string) *transformer { } } -var jobTransformers []transformFunc = []transformFunc{ - transformNotebookTask, - transformSparkTask, - transformWhlLibrary, - transformJarLibrary, -} - func applyJobTransformers(m *translatePaths, b *bundle.Bundle) error { + jobTransformers := []transformFunc{ + transformNotebookTask, + transformSparkTask, + transformWhlLibrary, + transformJarLibrary, + } + for key, job := range b.Config.Resources.Jobs { dir, err := job.ConfigFileDirectory() if err != nil { diff --git a/bundle/config/mutator/translate_paths_pipelines.go b/bundle/config/mutator/translate_paths_pipelines.go index 6d5569d70d..1afdb9d51b 100644 --- a/bundle/config/mutator/translate_paths_pipelines.go +++ b/bundle/config/mutator/translate_paths_pipelines.go @@ -7,30 +7,6 @@ import ( "github.com/databricks/databricks-sdk-go/service/pipelines" ) -var pipelineTransformers []transformFunc = []transformFunc{ - transformLibraryNotebook, - transformLibraryFile, -} - -func applyPipelineTransformers(m *translatePaths, b *bundle.Bundle) error { - for key, pipeline := range b.Config.Resources.Pipelines { - dir, err := pipeline.ConfigFileDirectory() - if err != nil { - return fmt.Errorf("unable to determine directory for pipeline %s: %w", key, err) - } - - for i := 0; i < len(pipeline.Libraries); i++ { - library := &pipeline.Libraries[i] - err := m.applyTransformers(pipelineTransformers, b, library, dir) - if err != nil { - return err - } - } - } - - return nil -} - func transformLibraryNotebook(resource any, dir string) *transformer { library, ok := resource.(*pipelines.PipelineLibrary) if !ok || library.Notebook == nil { @@ -58,3 +34,27 @@ func transformLibraryFile(resource any, dir string) *transformer { translateFilePath, } } + +func applyPipelineTransformers(m *translatePaths, b *bundle.Bundle) error { + pipelineTransformers := []transformFunc{ + transformLibraryNotebook, + transformLibraryFile, + } + + for key, pipeline := range b.Config.Resources.Pipelines { + dir, err := pipeline.ConfigFileDirectory() + if err != nil { + return fmt.Errorf("unable to determine directory for pipeline %s: %w", key, err) + } + + for i := 0; i < len(pipeline.Libraries); i++ { + library := &pipeline.Libraries[i] + err := m.applyTransformers(pipelineTransformers, b, library, dir) + if err != nil { + return err + } + } + } + + return nil +} From 5056c78bae993a622c79727641759430d9a171cc Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Mon, 4 Sep 2023 11:45:48 +0200 Subject: [PATCH 11/11] Fix function signature to match the others --- bundle/config/mutator/translate_paths.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bundle/config/mutator/translate_paths.go b/bundle/config/mutator/translate_paths.go index bdd5ce8e40..acfd55258b 100644 --- a/bundle/config/mutator/translate_paths.go +++ b/bundle/config/mutator/translate_paths.go @@ -135,8 +135,8 @@ func translateFilePath(literal, localFullPath, localRelPath, remotePath string) return remotePath, nil } -func translateNoOp(literal, localPath, localFullPath, remotePath string) (string, error) { - return localFullPath, nil +func translateNoOp(literal, localFullPath, localRelPath, remotePath string) (string, error) { + return localRelPath, nil } type transformer struct {