From 0da0a37eda201d737895a6a16b862f0f2f7ce3de Mon Sep 17 00:00:00 2001 From: kartikgupta-db Date: Thu, 20 Jul 2023 11:35:26 +0200 Subject: [PATCH 01/12] Add DATABRICKS_BUNDLE_INCLUDE_PATHS to specify include paths through env vars --- bundle/bundle.go | 32 +++++++++++++++++++++++- bundle/config/mutator/default_include.go | 18 +++++++------ 2 files changed, 42 insertions(+), 8 deletions(-) diff --git a/bundle/bundle.go b/bundle/bundle.go index 81fdfd4a85..a6dce3d3b2 100644 --- a/bundle/bundle.go +++ b/bundle/bundle.go @@ -10,6 +10,7 @@ import ( "fmt" "os" "path/filepath" + "strings" "sync" "github.com/databricks/cli/bundle/config" @@ -43,6 +44,29 @@ type Bundle struct { AutoApprove bool } +const extraIncludePathsKey string = "DATABRICKS_BUNDLE_INCLUDE_PATHS" + +// Get extra include paths from environment variable +func GetExtraIncludePaths() []string { + value, exists := os.LookupEnv(extraIncludePathsKey) + if !exists { + return []string{} + } + return strings.Split(os.Getenv(value), ":") +} + +// Try loading bundle config from one of the extra include paths from the environment variable +// If not able to load, do not return an error, just return nil +func tryLoadFromExtraIncludePaths() *Bundle { + for _, extraIncludePath := range GetExtraIncludePaths() { + bundle := &Bundle{} + if err := bundle.Config.Load(extraIncludePath); err == nil { + return bundle + } + } + return nil +} + func Load(path string) (*Bundle, error) { bundle := &Bundle{} configFile, err := config.FileNames.FindInPath(path) @@ -73,14 +97,20 @@ func MustLoad() (*Bundle, error) { func TryLoad() (*Bundle, error) { root, err := tryGetRoot() if err != nil { + if b := tryLoadFromExtraIncludePaths(); b != nil { + return b, nil + } return nil, err } // No root is fine in this function. if root == "" { - return nil, nil + return tryLoadFromExtraIncludePaths(), nil } + // We only fallback to extra include paths if root config is NOT FOUND. + // Therefore, the intended UX for end users is that they always have a VALID root config. + // The loading of extra include paths is only for the convenience of interfacing with internal tools. return Load(root) } diff --git a/bundle/config/mutator/default_include.go b/bundle/config/mutator/default_include.go index baf0529685..d149e49f11 100644 --- a/bundle/config/mutator/default_include.go +++ b/bundle/config/mutator/default_include.go @@ -13,14 +13,18 @@ type defineDefaultInclude struct { // DefineDefaultInclude sets the list of includes to a default if it hasn't been set. func DefineDefaultInclude() bundle.Mutator { - return &defineDefaultInclude{ + var includePaths = []string{ // When we support globstar we can collapse below into a single line. - include: []string{ - // Load YAML files in the same directory. - "*.yml", - // Load YAML files in subdirectories. - "*/*.yml", - }, + // Load YAML files in the same directory. + "*.yml", + // Load YAML files in subdirectories. + "*/*.yml", + } + if extraIncludePaths := bundle.GetExtraIncludePaths(); len(extraIncludePaths) > 0 { + includePaths = append(includePaths, extraIncludePaths...) + } + return &defineDefaultInclude{ + include: includePaths, } } From ef328037491c5afc7d16cddb2124ac026dee2ece Mon Sep 17 00:00:00 2001 From: kartikgupta-db Date: Thu, 20 Jul 2023 11:49:21 +0200 Subject: [PATCH 02/12] Add DATABRICKS_BUNDLE_INCLUDE_PATHS to specify include paths through env vars --- bundle/config/mutator/default_include.go | 5 +---- bundle/config/mutator/process_root_includes.go | 12 ++++++++++++ 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/bundle/config/mutator/default_include.go b/bundle/config/mutator/default_include.go index d149e49f11..fe80d37fd8 100644 --- a/bundle/config/mutator/default_include.go +++ b/bundle/config/mutator/default_include.go @@ -20,11 +20,8 @@ func DefineDefaultInclude() bundle.Mutator { // Load YAML files in subdirectories. "*/*.yml", } - if extraIncludePaths := bundle.GetExtraIncludePaths(); len(extraIncludePaths) > 0 { - includePaths = append(includePaths, extraIncludePaths...) - } return &defineDefaultInclude{ - include: includePaths, + include: append(includePaths, bundle.GetExtraIncludePaths()...), } } diff --git a/bundle/config/mutator/process_root_includes.go b/bundle/config/mutator/process_root_includes.go index f3717ce01e..f5b6b789fb 100644 --- a/bundle/config/mutator/process_root_includes.go +++ b/bundle/config/mutator/process_root_includes.go @@ -80,6 +80,18 @@ func (m *processRootIncludes) Apply(ctx context.Context, b *bundle.Bundle) error } } + extraIncludePaths := bundle.GetExtraIncludePaths() + for _, extraIncludePath := range extraIncludePaths { + if filepath.IsAbs(extraIncludePath) { + extraIncludePath, err := filepath.Rel(b.Config.Path, extraIncludePath) + if err != nil { + return fmt.Errorf("included file '%s' is not inside bundle root %s: %w", extraIncludePath, b.Config.Path, err) + } + } + out = append(out, ProcessInclude(filepath.Join(b.Config.Path, extraIncludePath), extraIncludePath)) + } + files = append(files, bundle.GetExtraIncludePaths()...) + // Swap out the original includes list with the expanded globs. b.Config.Include = files From 658f2d63d3d6e8ebdfe93015345aa19c4adb8336 Mon Sep 17 00:00:00 2001 From: kartikgupta-db Date: Fri, 21 Jul 2023 10:05:16 +0200 Subject: [PATCH 03/12] Unit tests --- bundle/bundle.go | 6 +-- bundle/bundle_test.go | 9 ++++ .../config/mutator/process_root_includes.go | 13 ++++-- .../mutator/process_root_includes_test.go | 42 +++++++++++++++++++ 4 files changed, 63 insertions(+), 7 deletions(-) diff --git a/bundle/bundle.go b/bundle/bundle.go index a6dce3d3b2..09947f6e89 100644 --- a/bundle/bundle.go +++ b/bundle/bundle.go @@ -44,15 +44,15 @@ type Bundle struct { AutoApprove bool } -const extraIncludePathsKey string = "DATABRICKS_BUNDLE_INCLUDE_PATHS" +const ExtraIncludePathsKey string = "DATABRICKS_BUNDLE_INCLUDE_PATHS" // Get extra include paths from environment variable func GetExtraIncludePaths() []string { - value, exists := os.LookupEnv(extraIncludePathsKey) + value, exists := os.LookupEnv(ExtraIncludePathsKey) if !exists { return []string{} } - return strings.Split(os.Getenv(value), ":") + return strings.Split(value, ":") } // Try loading bundle config from one of the extra include paths from the environment variable diff --git a/bundle/bundle_test.go b/bundle/bundle_test.go index 18550f4f2d..922a084240 100644 --- a/bundle/bundle_test.go +++ b/bundle/bundle_test.go @@ -106,3 +106,12 @@ func TestBundleTryLoadOkIfNotFound(t *testing.T) { assert.NoError(t, err) assert.Nil(t, b) } + +func TestBundleTryLoadFallbackToExtraIncludePaths(t *testing.T) { + wd, err := os.Getwd() + assert.NoError(t, err) + t.Setenv(ExtraIncludePathsKey, filepath.Join(wd, "./tests/basic/databricks.yml")) + b, err := TryLoad() + assert.NoError(t, err) + assert.NotNil(t, b) +} diff --git a/bundle/config/mutator/process_root_includes.go b/bundle/config/mutator/process_root_includes.go index f5b6b789fb..50f281dff3 100644 --- a/bundle/config/mutator/process_root_includes.go +++ b/bundle/config/mutator/process_root_includes.go @@ -80,17 +80,22 @@ func (m *processRootIncludes) Apply(ctx context.Context, b *bundle.Bundle) error } } - extraIncludePaths := bundle.GetExtraIncludePaths() + var extraIncludePaths = bundle.GetExtraIncludePaths() for _, extraIncludePath := range extraIncludePaths { if filepath.IsAbs(extraIncludePath) { - extraIncludePath, err := filepath.Rel(b.Config.Path, extraIncludePath) + rel, err := filepath.Rel(b.Config.Path, extraIncludePath) if err != nil { - return fmt.Errorf("included file '%s' is not inside bundle root %s: %w", extraIncludePath, b.Config.Path, err) + return fmt.Errorf("unable to include file '%s': %w", extraIncludePath, err) } + extraIncludePath = rel } + if _, ok := seen[extraIncludePath]; ok { + continue + } + seen[extraIncludePath] = true out = append(out, ProcessInclude(filepath.Join(b.Config.Path, extraIncludePath), extraIncludePath)) + files = append(files, extraIncludePath) } - files = append(files, bundle.GetExtraIncludePaths()...) // Swap out the original includes list with the expanded globs. b.Config.Include = files diff --git a/bundle/config/mutator/process_root_includes_test.go b/bundle/config/mutator/process_root_includes_test.go index 9ca5335ac5..26f7709d66 100644 --- a/bundle/config/mutator/process_root_includes_test.go +++ b/bundle/config/mutator/process_root_includes_test.go @@ -2,7 +2,9 @@ package mutator_test import ( "context" + "fmt" "os" + "path" "path/filepath" "runtime" "testing" @@ -122,3 +124,43 @@ func TestProcessRootIncludesNotExists(t *testing.T) { require.Error(t, err) assert.Contains(t, err.Error(), "notexist.yml defined in 'include' section does not match any files") } + +func TestProcessRootIncludesExtrasFromEnvVar(t *testing.T) { + rootPath := t.TempDir() + testYamlName := "extra_include_path.yml" + touch(t, rootPath, testYamlName) + os.Setenv(bundle.ExtraIncludePathsKey, path.Join(rootPath, testYamlName)) + t.Cleanup(func() { + os.Unsetenv(bundle.ExtraIncludePathsKey) + }) + + bundle := &bundle.Bundle{ + Config: config.Root{ + Path: rootPath, + }, + } + + err := mutator.ProcessRootIncludes().Apply(context.Background(), bundle) + require.NoError(t, err) + assert.Contains(t, bundle.Config.Include, testYamlName) +} + +func TestProcessRootIncludesDedupExtrasFromEnvVar(t *testing.T) { + rootPath := t.TempDir() + testYamlName := "extra_include_path.yml" + touch(t, rootPath, testYamlName) + os.Setenv(bundle.ExtraIncludePathsKey, fmt.Sprintf("%s:%s", path.Join(rootPath, testYamlName), path.Join(rootPath, testYamlName))) + t.Cleanup(func() { + os.Unsetenv(bundle.ExtraIncludePathsKey) + }) + + bundle := &bundle.Bundle{ + Config: config.Root{ + Path: rootPath, + }, + } + + err := mutator.ProcessRootIncludes().Apply(context.Background(), bundle) + require.NoError(t, err) + assert.Equal(t, []string{testYamlName}, bundle.Config.Include) +} From 32fa65995916dd0d5a6f9c5c68886f2fbedca6e6 Mon Sep 17 00:00:00 2001 From: kartikgupta-db Date: Fri, 21 Jul 2023 10:14:35 +0200 Subject: [PATCH 04/12] update unit tests --- bundle/config/mutator/process_root_includes_test.go | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/bundle/config/mutator/process_root_includes_test.go b/bundle/config/mutator/process_root_includes_test.go index 26f7709d66..102d75f0f0 100644 --- a/bundle/config/mutator/process_root_includes_test.go +++ b/bundle/config/mutator/process_root_includes_test.go @@ -149,10 +149,7 @@ func TestProcessRootIncludesDedupExtrasFromEnvVar(t *testing.T) { rootPath := t.TempDir() testYamlName := "extra_include_path.yml" touch(t, rootPath, testYamlName) - os.Setenv(bundle.ExtraIncludePathsKey, fmt.Sprintf("%s:%s", path.Join(rootPath, testYamlName), path.Join(rootPath, testYamlName))) - t.Cleanup(func() { - os.Unsetenv(bundle.ExtraIncludePathsKey) - }) + t.Setenv(bundle.ExtraIncludePathsKey, fmt.Sprintf("%s:%s", path.Join(rootPath, testYamlName), path.Join(rootPath, testYamlName))) bundle := &bundle.Bundle{ Config: config.Root{ From 93100f28b886e1ffd43b12b7b1b7b9ef7842a612 Mon Sep 17 00:00:00 2001 From: kartikgupta-db Date: Fri, 21 Jul 2023 11:16:15 +0200 Subject: [PATCH 05/12] fix windows unit tests --- bundle/bundle.go | 7 ++++++- bundle/config/mutator/process_root_includes_test.go | 6 +++++- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/bundle/bundle.go b/bundle/bundle.go index 09947f6e89..19aa3705f1 100644 --- a/bundle/bundle.go +++ b/bundle/bundle.go @@ -10,6 +10,7 @@ import ( "fmt" "os" "path/filepath" + "runtime" "strings" "sync" @@ -52,7 +53,11 @@ func GetExtraIncludePaths() []string { if !exists { return []string{} } - return strings.Split(value, ":") + var sep = ":" + if runtime.GOOS == "windows" { + sep = ";" + } + return strings.Split(value, sep) } // Try loading bundle config from one of the extra include paths from the environment variable diff --git a/bundle/config/mutator/process_root_includes_test.go b/bundle/config/mutator/process_root_includes_test.go index 102d75f0f0..6b5cfa1b11 100644 --- a/bundle/config/mutator/process_root_includes_test.go +++ b/bundle/config/mutator/process_root_includes_test.go @@ -149,7 +149,11 @@ func TestProcessRootIncludesDedupExtrasFromEnvVar(t *testing.T) { rootPath := t.TempDir() testYamlName := "extra_include_path.yml" touch(t, rootPath, testYamlName) - t.Setenv(bundle.ExtraIncludePathsKey, fmt.Sprintf("%s:%s", path.Join(rootPath, testYamlName), path.Join(rootPath, testYamlName))) + sep := ":" + if runtime.GOOS == "windows" { + sep = ";" + } + t.Setenv(bundle.ExtraIncludePathsKey, fmt.Sprintf("%s%s%s", path.Join(rootPath, testYamlName), sep, path.Join(rootPath, testYamlName))) bundle := &bundle.Bundle{ Config: config.Root{ From 5e74315270b8760afa86a91d88975eca2565bcf1 Mon Sep 17 00:00:00 2001 From: kartikgupta-db Date: Fri, 21 Jul 2023 12:24:14 +0200 Subject: [PATCH 06/12] address feedback --- bundle/bundle.go | 20 ++++++++----------- bundle/bundle_test.go | 1 + .../mutator/process_root_includes_test.go | 6 +----- 3 files changed, 10 insertions(+), 17 deletions(-) diff --git a/bundle/bundle.go b/bundle/bundle.go index 19aa3705f1..6effa46f1a 100644 --- a/bundle/bundle.go +++ b/bundle/bundle.go @@ -10,7 +10,6 @@ import ( "fmt" "os" "path/filepath" - "runtime" "strings" "sync" @@ -51,13 +50,9 @@ const ExtraIncludePathsKey string = "DATABRICKS_BUNDLE_INCLUDE_PATHS" func GetExtraIncludePaths() []string { value, exists := os.LookupEnv(ExtraIncludePathsKey) if !exists { - return []string{} + return nil } - var sep = ":" - if runtime.GOOS == "windows" { - sep = ";" - } - return strings.Split(value, sep) + return strings.Split(value, string(os.PathListSeparator)) } // Try loading bundle config from one of the extra include paths from the environment variable @@ -102,20 +97,21 @@ func MustLoad() (*Bundle, error) { func TryLoad() (*Bundle, error) { root, err := tryGetRoot() if err != nil { - if b := tryLoadFromExtraIncludePaths(); b != nil { - return b, nil - } return nil, err } // No root is fine in this function. if root == "" { - return tryLoadFromExtraIncludePaths(), nil + return nil, nil } - // We only fallback to extra include paths if root config is NOT FOUND. + // We only fallback to extra include paths if bundle root is defined but a root config is NOT FOUND // Therefore, the intended UX for end users is that they always have a VALID root config. // The loading of extra include paths is only for the convenience of interfacing with internal tools. + if _, err := config.FileNames.FindInPath(root); err != nil { + return tryLoadFromExtraIncludePaths(), nil + } + return Load(root) } diff --git a/bundle/bundle_test.go b/bundle/bundle_test.go index 922a084240..520c0ed7d3 100644 --- a/bundle/bundle_test.go +++ b/bundle/bundle_test.go @@ -111,6 +111,7 @@ func TestBundleTryLoadFallbackToExtraIncludePaths(t *testing.T) { wd, err := os.Getwd() assert.NoError(t, err) t.Setenv(ExtraIncludePathsKey, filepath.Join(wd, "./tests/basic/databricks.yml")) + t.Setenv(envBundleRoot, t.TempDir()) b, err := TryLoad() assert.NoError(t, err) assert.NotNil(t, b) diff --git a/bundle/config/mutator/process_root_includes_test.go b/bundle/config/mutator/process_root_includes_test.go index 6b5cfa1b11..1ce094bc3f 100644 --- a/bundle/config/mutator/process_root_includes_test.go +++ b/bundle/config/mutator/process_root_includes_test.go @@ -149,11 +149,7 @@ func TestProcessRootIncludesDedupExtrasFromEnvVar(t *testing.T) { rootPath := t.TempDir() testYamlName := "extra_include_path.yml" touch(t, rootPath, testYamlName) - sep := ":" - if runtime.GOOS == "windows" { - sep = ";" - } - t.Setenv(bundle.ExtraIncludePathsKey, fmt.Sprintf("%s%s%s", path.Join(rootPath, testYamlName), sep, path.Join(rootPath, testYamlName))) + t.Setenv(bundle.ExtraIncludePathsKey, fmt.Sprintf("%s%s%s", path.Join(rootPath, testYamlName), string(os.PathListSeparator), path.Join(rootPath, testYamlName))) bundle := &bundle.Bundle{ Config: config.Root{ From e35f4e72c922845bf8824276c6426555e0c036ee Mon Sep 17 00:00:00 2001 From: kartikgupta-db Date: Fri, 21 Jul 2023 12:28:23 +0200 Subject: [PATCH 07/12] comment --- bundle/bundle.go | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/bundle/bundle.go b/bundle/bundle.go index 6effa46f1a..3f78423f92 100644 --- a/bundle/bundle.go +++ b/bundle/bundle.go @@ -55,9 +55,14 @@ func GetExtraIncludePaths() []string { return strings.Split(value, string(os.PathListSeparator)) } -// Try loading bundle config from one of the extra include paths from the environment variable +// Try loading bundle config from one of the extra include paths +// from the environment variable, if the bundle root is known but no +// bundle config is found in the bundle root. // If not able to load, do not return an error, just return nil -func tryLoadFromExtraIncludePaths() *Bundle { +func tryLoadFromExtraIncludePaths(bundleRoot string) *Bundle { + if _, err := config.FileNames.FindInPath(bundleRoot); err == nil { + return nil + } for _, extraIncludePath := range GetExtraIncludePaths() { bundle := &Bundle{} if err := bundle.Config.Load(extraIncludePath); err == nil { @@ -108,8 +113,8 @@ func TryLoad() (*Bundle, error) { // We only fallback to extra include paths if bundle root is defined but a root config is NOT FOUND // Therefore, the intended UX for end users is that they always have a VALID root config. // The loading of extra include paths is only for the convenience of interfacing with internal tools. - if _, err := config.FileNames.FindInPath(root); err != nil { - return tryLoadFromExtraIncludePaths(), nil + if b := tryLoadFromExtraIncludePaths(root); b != nil { + return b, nil } return Load(root) From 55db33e25a54b22dbf21e73f7bbd4f65c37f8e21 Mon Sep 17 00:00:00 2001 From: kartikgupta-db Date: Tue, 25 Jul 2023 12:06:09 +0200 Subject: [PATCH 08/12] revert changes for bundle root not defined --- bundle/bundle.go | 36 ------------------- bundle/bundle_test.go | 10 ------ bundle/config/mutator/default_include.go | 2 +- .../config/mutator/process_root_includes.go | 14 +++++++- .../mutator/process_root_includes_test.go | 6 ++-- 5 files changed, 17 insertions(+), 51 deletions(-) diff --git a/bundle/bundle.go b/bundle/bundle.go index 3f78423f92..81fdfd4a85 100644 --- a/bundle/bundle.go +++ b/bundle/bundle.go @@ -10,7 +10,6 @@ import ( "fmt" "os" "path/filepath" - "strings" "sync" "github.com/databricks/cli/bundle/config" @@ -44,34 +43,6 @@ type Bundle struct { AutoApprove bool } -const ExtraIncludePathsKey string = "DATABRICKS_BUNDLE_INCLUDE_PATHS" - -// Get extra include paths from environment variable -func GetExtraIncludePaths() []string { - value, exists := os.LookupEnv(ExtraIncludePathsKey) - if !exists { - return nil - } - return strings.Split(value, string(os.PathListSeparator)) -} - -// Try loading bundle config from one of the extra include paths -// from the environment variable, if the bundle root is known but no -// bundle config is found in the bundle root. -// If not able to load, do not return an error, just return nil -func tryLoadFromExtraIncludePaths(bundleRoot string) *Bundle { - if _, err := config.FileNames.FindInPath(bundleRoot); err == nil { - return nil - } - for _, extraIncludePath := range GetExtraIncludePaths() { - bundle := &Bundle{} - if err := bundle.Config.Load(extraIncludePath); err == nil { - return bundle - } - } - return nil -} - func Load(path string) (*Bundle, error) { bundle := &Bundle{} configFile, err := config.FileNames.FindInPath(path) @@ -110,13 +81,6 @@ func TryLoad() (*Bundle, error) { return nil, nil } - // We only fallback to extra include paths if bundle root is defined but a root config is NOT FOUND - // Therefore, the intended UX for end users is that they always have a VALID root config. - // The loading of extra include paths is only for the convenience of interfacing with internal tools. - if b := tryLoadFromExtraIncludePaths(root); b != nil { - return b, nil - } - return Load(root) } diff --git a/bundle/bundle_test.go b/bundle/bundle_test.go index 520c0ed7d3..18550f4f2d 100644 --- a/bundle/bundle_test.go +++ b/bundle/bundle_test.go @@ -106,13 +106,3 @@ func TestBundleTryLoadOkIfNotFound(t *testing.T) { assert.NoError(t, err) assert.Nil(t, b) } - -func TestBundleTryLoadFallbackToExtraIncludePaths(t *testing.T) { - wd, err := os.Getwd() - assert.NoError(t, err) - t.Setenv(ExtraIncludePathsKey, filepath.Join(wd, "./tests/basic/databricks.yml")) - t.Setenv(envBundleRoot, t.TempDir()) - b, err := TryLoad() - assert.NoError(t, err) - assert.NotNil(t, b) -} diff --git a/bundle/config/mutator/default_include.go b/bundle/config/mutator/default_include.go index fe80d37fd8..8d325cea3f 100644 --- a/bundle/config/mutator/default_include.go +++ b/bundle/config/mutator/default_include.go @@ -21,7 +21,7 @@ func DefineDefaultInclude() bundle.Mutator { "*/*.yml", } return &defineDefaultInclude{ - include: append(includePaths, bundle.GetExtraIncludePaths()...), + include: append(includePaths, GetExtraIncludePaths()...), } } diff --git a/bundle/config/mutator/process_root_includes.go b/bundle/config/mutator/process_root_includes.go index 50f281dff3..db9c6a2a14 100644 --- a/bundle/config/mutator/process_root_includes.go +++ b/bundle/config/mutator/process_root_includes.go @@ -3,6 +3,7 @@ package mutator import ( "context" "fmt" + "os" "path/filepath" "strings" @@ -11,6 +12,17 @@ import ( "golang.org/x/exp/slices" ) +const ExtraIncludePathsKey string = "DATABRICKS_BUNDLE_INCLUDE_PATHS" + +// Get extra include paths from environment variable +func GetExtraIncludePaths() []string { + value, exists := os.LookupEnv(ExtraIncludePathsKey) + if !exists { + return nil + } + return strings.Split(value, string(os.PathListSeparator)) +} + type processRootIncludes struct{} // ProcessRootIncludes expands the patterns in the configuration's include list @@ -80,7 +92,7 @@ func (m *processRootIncludes) Apply(ctx context.Context, b *bundle.Bundle) error } } - var extraIncludePaths = bundle.GetExtraIncludePaths() + var extraIncludePaths = GetExtraIncludePaths() for _, extraIncludePath := range extraIncludePaths { if filepath.IsAbs(extraIncludePath) { rel, err := filepath.Rel(b.Config.Path, extraIncludePath) diff --git a/bundle/config/mutator/process_root_includes_test.go b/bundle/config/mutator/process_root_includes_test.go index 1ce094bc3f..449e3a02c7 100644 --- a/bundle/config/mutator/process_root_includes_test.go +++ b/bundle/config/mutator/process_root_includes_test.go @@ -129,9 +129,9 @@ func TestProcessRootIncludesExtrasFromEnvVar(t *testing.T) { rootPath := t.TempDir() testYamlName := "extra_include_path.yml" touch(t, rootPath, testYamlName) - os.Setenv(bundle.ExtraIncludePathsKey, path.Join(rootPath, testYamlName)) + os.Setenv(mutator.ExtraIncludePathsKey, path.Join(rootPath, testYamlName)) t.Cleanup(func() { - os.Unsetenv(bundle.ExtraIncludePathsKey) + os.Unsetenv(mutator.ExtraIncludePathsKey) }) bundle := &bundle.Bundle{ @@ -149,7 +149,7 @@ func TestProcessRootIncludesDedupExtrasFromEnvVar(t *testing.T) { rootPath := t.TempDir() testYamlName := "extra_include_path.yml" touch(t, rootPath, testYamlName) - t.Setenv(bundle.ExtraIncludePathsKey, fmt.Sprintf("%s%s%s", path.Join(rootPath, testYamlName), string(os.PathListSeparator), path.Join(rootPath, testYamlName))) + t.Setenv(mutator.ExtraIncludePathsKey, fmt.Sprintf("%s%s%s", path.Join(rootPath, testYamlName), string(os.PathListSeparator), path.Join(rootPath, testYamlName))) bundle := &bundle.Bundle{ Config: config.Root{ From 7a1ce07ea40c235d1fc6bf0adf761ee7f873fe8d Mon Sep 17 00:00:00 2001 From: kartikgupta-db Date: Tue, 25 Jul 2023 13:14:25 +0200 Subject: [PATCH 09/12] refactor to add extra includes to config.include --- .../config/mutator/process_root_includes.go | 29 ++++++++----------- 1 file changed, 12 insertions(+), 17 deletions(-) diff --git a/bundle/config/mutator/process_root_includes.go b/bundle/config/mutator/process_root_includes.go index db9c6a2a14..d9c225c744 100644 --- a/bundle/config/mutator/process_root_includes.go +++ b/bundle/config/mutator/process_root_includes.go @@ -49,6 +49,18 @@ func (m *processRootIncludes) Apply(ctx context.Context, b *bundle.Bundle) error // This is stored in the bundle configuration for observability. var files []string + // Converts extra include paths from environment variable to relative paths + for _, extraIncludePath := range GetExtraIncludePaths() { + if filepath.IsAbs(extraIncludePath) { + rel, err := filepath.Rel(b.Config.Path, extraIncludePath) + if err != nil { + return fmt.Errorf("unable to include file '%s': %w", extraIncludePath, err) + } + extraIncludePath = rel + } + b.Config.Include = append(b.Config.Include, extraIncludePath) + } + // For each glob, find all files to load. // Ordering of the list of globs is maintained in the output. // For matches that appear in multiple globs, only the first is kept. @@ -92,23 +104,6 @@ func (m *processRootIncludes) Apply(ctx context.Context, b *bundle.Bundle) error } } - var extraIncludePaths = GetExtraIncludePaths() - for _, extraIncludePath := range extraIncludePaths { - if filepath.IsAbs(extraIncludePath) { - rel, err := filepath.Rel(b.Config.Path, extraIncludePath) - if err != nil { - return fmt.Errorf("unable to include file '%s': %w", extraIncludePath, err) - } - extraIncludePath = rel - } - if _, ok := seen[extraIncludePath]; ok { - continue - } - seen[extraIncludePath] = true - out = append(out, ProcessInclude(filepath.Join(b.Config.Path, extraIncludePath), extraIncludePath)) - files = append(files, extraIncludePath) - } - // Swap out the original includes list with the expanded globs. b.Config.Include = files From c0bb634d6a5dc8f49a325d3425ec37bf075641d6 Mon Sep 17 00:00:00 2001 From: kartikgupta-db Date: Tue, 25 Jul 2023 13:18:46 +0200 Subject: [PATCH 10/12] rename env var --- bundle/config/mutator/process_root_includes.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bundle/config/mutator/process_root_includes.go b/bundle/config/mutator/process_root_includes.go index d9c225c744..1b0faa77eb 100644 --- a/bundle/config/mutator/process_root_includes.go +++ b/bundle/config/mutator/process_root_includes.go @@ -12,7 +12,7 @@ import ( "golang.org/x/exp/slices" ) -const ExtraIncludePathsKey string = "DATABRICKS_BUNDLE_INCLUDE_PATHS" +const ExtraIncludePathsKey string = "DATABRICKS_BUNDLE_INCLUDES" // Get extra include paths from environment variable func GetExtraIncludePaths() []string { From 4e2716cd7b8aa28b9f35104461da1ab1f9f126a0 Mon Sep 17 00:00:00 2001 From: kartikgupta-db Date: Tue, 25 Jul 2023 15:40:59 +0200 Subject: [PATCH 11/12] Initialise a empty default bundle if BUNDLE_ROOT and DATABRICKS_BUNDLE_INCLUDES envs are present --- bundle/bundle.go | 16 ++++++++++ .../config/mutator/process_root_includes.go | 4 +-- .../mutator/process_root_includes_test.go | 6 ++-- bundle/root_test.go | 30 +++++++++++++++++++ 4 files changed, 50 insertions(+), 6 deletions(-) diff --git a/bundle/bundle.go b/bundle/bundle.go index 81fdfd4a85..2a5c5c2c31 100644 --- a/bundle/bundle.go +++ b/bundle/bundle.go @@ -43,10 +43,26 @@ type Bundle struct { AutoApprove bool } +const ExtraIncludePathsKey string = "DATABRICKS_BUNDLE_INCLUDES" + func Load(path string) (*Bundle, error) { bundle := &Bundle{} + stat, err := os.Stat(path) + if err != nil { + return nil, err + } configFile, err := config.FileNames.FindInPath(path) if err != nil { + _, hasIncludePathEnv := os.LookupEnv(ExtraIncludePathsKey) + if hasIncludePathEnv && stat.IsDir() { + bundle.Config = config.Root{ + Path: path, + Bundle: config.Bundle{ + Name: filepath.Base(path), + }, + } + return bundle, nil + } return nil, err } err = bundle.Config.Load(configFile) diff --git a/bundle/config/mutator/process_root_includes.go b/bundle/config/mutator/process_root_includes.go index 1b0faa77eb..c2dffc6eed 100644 --- a/bundle/config/mutator/process_root_includes.go +++ b/bundle/config/mutator/process_root_includes.go @@ -12,11 +12,9 @@ import ( "golang.org/x/exp/slices" ) -const ExtraIncludePathsKey string = "DATABRICKS_BUNDLE_INCLUDES" - // Get extra include paths from environment variable func GetExtraIncludePaths() []string { - value, exists := os.LookupEnv(ExtraIncludePathsKey) + value, exists := os.LookupEnv(bundle.ExtraIncludePathsKey) if !exists { return nil } diff --git a/bundle/config/mutator/process_root_includes_test.go b/bundle/config/mutator/process_root_includes_test.go index 449e3a02c7..1ce094bc3f 100644 --- a/bundle/config/mutator/process_root_includes_test.go +++ b/bundle/config/mutator/process_root_includes_test.go @@ -129,9 +129,9 @@ func TestProcessRootIncludesExtrasFromEnvVar(t *testing.T) { rootPath := t.TempDir() testYamlName := "extra_include_path.yml" touch(t, rootPath, testYamlName) - os.Setenv(mutator.ExtraIncludePathsKey, path.Join(rootPath, testYamlName)) + os.Setenv(bundle.ExtraIncludePathsKey, path.Join(rootPath, testYamlName)) t.Cleanup(func() { - os.Unsetenv(mutator.ExtraIncludePathsKey) + os.Unsetenv(bundle.ExtraIncludePathsKey) }) bundle := &bundle.Bundle{ @@ -149,7 +149,7 @@ func TestProcessRootIncludesDedupExtrasFromEnvVar(t *testing.T) { rootPath := t.TempDir() testYamlName := "extra_include_path.yml" touch(t, rootPath, testYamlName) - t.Setenv(mutator.ExtraIncludePathsKey, fmt.Sprintf("%s%s%s", path.Join(rootPath, testYamlName), string(os.PathListSeparator), path.Join(rootPath, testYamlName))) + t.Setenv(bundle.ExtraIncludePathsKey, fmt.Sprintf("%s%s%s", path.Join(rootPath, testYamlName), string(os.PathListSeparator), path.Join(rootPath, testYamlName))) bundle := &bundle.Bundle{ Config: config.Root{ diff --git a/bundle/root_test.go b/bundle/root_test.go index 2f8304921e..174ba1d917 100644 --- a/bundle/root_test.go +++ b/bundle/root_test.go @@ -6,6 +6,7 @@ import ( "testing" "github.com/databricks/cli/bundle/config" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -102,3 +103,32 @@ func TestRootLookupError(t *testing.T) { _, err := mustGetRoot() require.ErrorContains(t, err, "unable to locate bundle root") } + +func TestLoadDefautlBundleWhenRootAndIncludesEnvPresent(t *testing.T) { + dir := t.TempDir() + chdir(t, dir) + t.Setenv(envBundleRoot, dir) + t.Setenv(ExtraIncludePathsKey, "test") + + bundle, err := MustLoad() + assert.NoError(t, err) + assert.Equal(t, dir, bundle.Config.Path) +} + +func TestErrorIfNoBundleAndNoRootEnv(t *testing.T) { + dir := t.TempDir() + chdir(t, dir) + t.Setenv(ExtraIncludePathsKey, "test") + + _, err := MustLoad() + assert.Error(t, err) +} + +func TestErrorIfNoBundleAndNoIncludesEnv(t *testing.T) { + dir := t.TempDir() + chdir(t, dir) + t.Setenv(envBundleRoot, dir) + + _, err := MustLoad() + assert.Error(t, err) +} From 6a5c376894c205044522bf8060f45d5b2ebbc1a0 Mon Sep 17 00:00:00 2001 From: kartikgupta-db Date: Wed, 2 Aug 2023 14:06:39 +0200 Subject: [PATCH 12/12] adress feedback --- bundle/bundle.go | 3 ++- bundle/root_test.go | 19 ++++++++++++++++--- 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/bundle/bundle.go b/bundle/bundle.go index 2a5c5c2c31..0147883ca7 100644 --- a/bundle/bundle.go +++ b/bundle/bundle.go @@ -54,7 +54,8 @@ func Load(path string) (*Bundle, error) { configFile, err := config.FileNames.FindInPath(path) if err != nil { _, hasIncludePathEnv := os.LookupEnv(ExtraIncludePathsKey) - if hasIncludePathEnv && stat.IsDir() { + _, hasBundleRootEnv := os.LookupEnv(envBundleRoot) + if hasIncludePathEnv && hasBundleRootEnv && stat.IsDir() { bundle.Config = config.Root{ Path: path, Bundle: config.Bundle{ diff --git a/bundle/root_test.go b/bundle/root_test.go index 174ba1d917..e85c4fdcb1 100644 --- a/bundle/root_test.go +++ b/bundle/root_test.go @@ -104,7 +104,20 @@ func TestRootLookupError(t *testing.T) { require.ErrorContains(t, err, "unable to locate bundle root") } -func TestLoadDefautlBundleWhenRootAndIncludesEnvPresent(t *testing.T) { +func TestLoadYamlWhenIncludesEnvPresent(t *testing.T) { + chdir(t, filepath.Join(".", "tests", "basic")) + t.Setenv(ExtraIncludePathsKey, "test") + + bundle, err := MustLoad() + assert.NoError(t, err) + assert.Equal(t, "basic", bundle.Config.Bundle.Name) + + cwd, err := os.Getwd() + assert.NoError(t, err) + assert.Equal(t, cwd, bundle.Config.Path) +} + +func TestLoadDefautlBundleWhenNoYamlAndRootAndIncludesEnvPresent(t *testing.T) { dir := t.TempDir() chdir(t, dir) t.Setenv(envBundleRoot, dir) @@ -115,7 +128,7 @@ func TestLoadDefautlBundleWhenRootAndIncludesEnvPresent(t *testing.T) { assert.Equal(t, dir, bundle.Config.Path) } -func TestErrorIfNoBundleAndNoRootEnv(t *testing.T) { +func TestErrorIfNoYamlNoRootEnvAndIncludesEnvPresent(t *testing.T) { dir := t.TempDir() chdir(t, dir) t.Setenv(ExtraIncludePathsKey, "test") @@ -124,7 +137,7 @@ func TestErrorIfNoBundleAndNoRootEnv(t *testing.T) { assert.Error(t, err) } -func TestErrorIfNoBundleAndNoIncludesEnv(t *testing.T) { +func TestErrorIfNoYamlNoIncludesEnvAndRootEnvPresent(t *testing.T) { dir := t.TempDir() chdir(t, dir) t.Setenv(envBundleRoot, dir)