Skip to content

Initialise a empty default bundle if BUNDLE_ROOT and DATABRICKS_BUNDLE_INCLUDES env vars are present#604

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
kartikgupta-db merged 16 commits into from
Aug 2, 2023
Merged

Initialise a empty default bundle if BUNDLE_ROOT and DATABRICKS_BUNDLE_INCLUDES env vars are present #604

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
0da0a37
Add DATABRICKS_BUNDLE_INCLUDE_PATHS to specify include paths through …
kartikgupta-db Jul 20, 2023
ef32803
Add DATABRICKS_BUNDLE_INCLUDE_PATHS to specify include paths through …
kartikgupta-db Jul 20, 2023
658f2d6
Unit tests
kartikgupta-db Jul 21, 2023
32fa659
update unit tests
kartikgupta-db Jul 21, 2023
93100f2
fix windows unit tests
kartikgupta-db Jul 21, 2023
5e74315
address feedback
kartikgupta-db Jul 21, 2023
e35f4e7
comment
kartikgupta-db Jul 21, 2023
7c78b41
Merge branch 'main' into include-paths-env-var
kartikgupta-db Jul 21, 2023
55db33e
revert changes for bundle root not defined
kartikgupta-db Jul 25, 2023
5e094a6
Merge branch 'include-paths-env-var' of github.com:databricks/cli int…
kartikgupta-db Jul 25, 2023
73a867d
merge
kartikgupta-db Jul 25, 2023
7a1ce07
refactor to add extra includes to config.include
kartikgupta-db Jul 25, 2023
c0bb634
rename env var
kartikgupta-db Jul 25, 2023
4e2716c
Initialise a empty default bundle if BUNDLE_ROOT and DATABRICKS_BUNDL…
kartikgupta-db Jul 25, 2023
638b503
Merge branch 'main' of github.com:databricks/cli into empty-bundle
kartikgupta-db Aug 2, 2023
6a5c376
adress feedback
kartikgupta-db Aug 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions bundle/bundle.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -43,10 +43,27 @@ type Bundle struct {
AutoApprove bool
}

const ExtraIncludePathsKey string = "DATABRICKS_BUNDLE_INCLUDES"

func Load(path string) (*Bundle, error) {

@andrewnesterandrewnesterAug 2, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand correctly the behaviour is different from what is intended based on PR description

This will initialise it not only when BUNDLE_ROOT is set but also if there any bundle/databricks.yml in current or child dirs (https://github.com/databricks/cli/blob/main/bundle/root.go#L51-L56C9).

Is this correct?

@kartikgupta-dbkartikgupta-dbAug 2, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The assumption here is that configFile, err := config.FileNames.FindInPath(path) is reached only when we have established a bundle root (either through BUNDLE_ROOT or by finding a *.yml).
This functions returns an error only when there is no config file found (no *.yml). Which means BUNDLE_ROOT is set but there is no config file.

I will make it an explicit test though to make reasoning easier.

@andrewnesterandrewnesterAug 2, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we be more explicit here? Our condition to have an empty config is:

  1. We have BUNDLE_ROOT env variable
  2. There is no config file in this BUNDLE_ROOT

Could you codify this condition? It would be somethin like

_, hasBundleRootEnv:=os.LookupEnv(envBundleRoot)
configFile, noConfigFoundErr:=config.FileNames.FindInPath(path)
ifhasBundleRootEnv&&noConfigFoundErr!=nil {
...
}

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)
_, hasBundleRootEnv := os.LookupEnv(envBundleRoot)
if hasIncludePathEnv && hasBundleRootEnv && 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)
Expand Down
4 changes: 1 addition & 3 deletions bundle/config/mutator/process_root_includes.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -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
}
Expand Down
6 changes: 3 additions & 3 deletions bundle/config/mutator/process_root_includes_test.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -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{
Expand All@@ -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{
Expand Down
43 changes: 43 additions & 0 deletions bundle/root_test.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -6,6 +6,7 @@ import (
"testing"

"github.com/databricks/cli/bundle/config"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

Expand DownExpand Up@@ -102,3 +103,45 @@ func TestRootLookupError(t *testing.T) {
_, err := mustGetRoot()
require.ErrorContains(t, err, "unable to locate bundle root")
}

@andrewnesterandrewnesterAug 2, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a test case where you have bundle.yml in the temp dir and also have ExtraInclude set and make sure it loads the config correctly?

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)
t.Setenv(ExtraIncludePathsKey, "test")

bundle, err := MustLoad()
assert.NoError(t, err)
assert.Equal(t, dir, bundle.Config.Path)
}

func TestErrorIfNoYamlNoRootEnvAndIncludesEnvPresent(t *testing.T) {
dir := t.TempDir()
chdir(t, dir)
t.Setenv(ExtraIncludePathsKey, "test")

_, err := MustLoad()
assert.Error(t, err)
}

func TestErrorIfNoYamlNoIncludesEnvAndRootEnvPresent(t *testing.T) {
dir := t.TempDir()
chdir(t, dir)
t.Setenv(envBundleRoot, dir)

_, err := MustLoad()
assert.Error(t, err)
}