diff --git a/NOTICE b/NOTICE index 71ba7fbcc4..fdc2a88cf9 100644 --- a/NOTICE +++ b/NOTICE @@ -57,11 +57,6 @@ google/uuid - https://github.com/google/uuid Copyright (c) 2009,2014 Google Inc. All rights reserved. License - https://github.com/google/uuid/blob/master/LICENSE -imdario/mergo - https://github.com/imdario/mergo -Copyright (c) 2013 Dario Castañé. All rights reserved. -Copyright (c) 2012 The Go Authors. All rights reserved. -License - https://github.com/imdario/mergo/blob/master/LICENSE - manifoldco/promptui - https://github.com/manifoldco/promptui Copyright (c) 2017, Arigato Machine Inc. All rights reserved. License - https://github.com/manifoldco/promptui/blob/master/LICENSE.md diff --git a/bundle/config/artifact.go b/bundle/config/artifact.go index dbf327fa05..219def5714 100644 --- a/bundle/config/artifact.go +++ b/bundle/config/artifact.go @@ -10,9 +10,9 @@ import ( type Artifacts map[string]*Artifact -func (artifacts Artifacts) SetConfigFilePath(path string) { +func (artifacts Artifacts) ConfigureConfigFilePath() { for _, artifact := range artifacts { - artifact.ConfigFilePath = path + artifact.ConfigureConfigFilePath() } } diff --git a/bundle/config/git.go b/bundle/config/git.go index 58a5d54d2b..f9f2f83e52 100644 --- a/bundle/config/git.go +++ b/bundle/config/git.go @@ -9,8 +9,8 @@ type Git struct { BundleRootPath string `json:"bundle_root_path,omitempty" bundle:"readonly"` // Inferred is set to true if the Git details were inferred and weren't set explicitly - Inferred bool `json:"-" bundle:"readonly"` + Inferred bool `json:"inferred,omitempty" bundle:"readonly"` // The actual branch according to Git (may be different from the configured branch) - ActualBranch string `json:"-" bundle:"readonly"` + ActualBranch string `json:"actual_branch,omitempty" bundle:"readonly"` } diff --git a/bundle/config/interpolation/interpolation.go b/bundle/config/interpolation/interpolation.go deleted file mode 100644 index 8ba0b8b1ff..0000000000 --- a/bundle/config/interpolation/interpolation.go +++ /dev/null @@ -1,254 +0,0 @@ -package interpolation - -import ( - "context" - "errors" - "fmt" - "reflect" - "regexp" - "sort" - "strings" - - "slices" - - "github.com/databricks/cli/bundle" - "github.com/databricks/cli/bundle/config/variable" - "golang.org/x/exp/maps" -) - -const Delimiter = "." - -// must start with alphabet, support hyphens and underscores in middle but must end with character -var re = regexp.MustCompile(`\$\{([a-zA-Z]+([-_]?[a-zA-Z0-9]+)*(\.[a-zA-Z]+([-_]?[a-zA-Z0-9]+)*)*)\}`) - -type stringField struct { - path string - - getter - setter -} - -func newStringField(path string, g getter, s setter) *stringField { - return &stringField{ - path: path, - - getter: g, - setter: s, - } -} - -func (s *stringField) dependsOn() []string { - var out []string - m := re.FindAllStringSubmatch(s.Get(), -1) - for i := range m { - out = append(out, m[i][1]) - } - return out -} - -func (s *stringField) interpolate(fns []LookupFunction, lookup map[string]string) { - out := re.ReplaceAllStringFunc(s.Get(), func(s string) string { - // Turn the whole match into the submatch. - match := re.FindStringSubmatch(s) - for _, fn := range fns { - v, err := fn(match[1], lookup) - if errors.Is(err, ErrSkipInterpolation) { - continue - } - if err != nil { - panic(err) - } - return v - } - - // No substitution. - return s - }) - - s.Set(out) -} - -type accumulator struct { - // all string fields in the bundle config - strings map[string]*stringField - - // contains path -> resolved_string mapping for string fields in the config - // The resolved strings will NOT contain any variable references that could - // have been resolved, however there might still be references that cannot - // be resolved - memo map[string]string -} - -// jsonFieldName returns the name in a field's `json` tag. -// Returns the empty string if it isn't set. -func jsonFieldName(sf reflect.StructField) string { - tag, ok := sf.Tag.Lookup("json") - if !ok { - return "" - } - parts := strings.Split(tag, ",") - if parts[0] == "-" { - return "" - } - return parts[0] -} - -func (a *accumulator) walkStruct(scope []string, rv reflect.Value) { - num := rv.NumField() - for i := 0; i < num; i++ { - sf := rv.Type().Field(i) - f := rv.Field(i) - - // Walk field with the same scope for anonymous (embedded) fields. - if sf.Anonymous { - a.walk(scope, f, anySetter{f}) - continue - } - - // Skip unnamed fields. - fieldName := jsonFieldName(rv.Type().Field(i)) - if fieldName == "" { - continue - } - - a.walk(append(scope, fieldName), f, anySetter{f}) - } -} - -func (a *accumulator) walk(scope []string, rv reflect.Value, s setter) { - // Dereference pointer. - if rv.Type().Kind() == reflect.Pointer { - // Skip nil pointers. - if rv.IsNil() { - return - } - rv = rv.Elem() - s = anySetter{rv} - } - - switch rv.Type().Kind() { - case reflect.String: - path := strings.Join(scope, Delimiter) - a.strings[path] = newStringField(path, anyGetter{rv}, s) - - // register alias for variable value. `var.foo` would be the alias for - // `variables.foo.value` - if len(scope) == 3 && scope[0] == "variables" && scope[2] == "value" { - aliasPath := strings.Join([]string{variable.VariableReferencePrefix, scope[1]}, Delimiter) - a.strings[aliasPath] = a.strings[path] - } - case reflect.Struct: - a.walkStruct(scope, rv) - case reflect.Map: - if rv.Type().Key().Kind() != reflect.String { - panic("only support string keys in map") - } - keys := rv.MapKeys() - for _, key := range keys { - a.walk(append(scope, key.String()), rv.MapIndex(key), mapSetter{rv, key}) - } - case reflect.Slice: - n := rv.Len() - name := scope[len(scope)-1] - base := scope[:len(scope)-1] - for i := 0; i < n; i++ { - element := rv.Index(i) - a.walk(append(base, fmt.Sprintf("%s[%d]", name, i)), element, anySetter{element}) - } - } -} - -// walk and gather all string fields in the config -func (a *accumulator) start(v any) { - rv := reflect.ValueOf(v) - if rv.Type().Kind() != reflect.Pointer { - panic("expect pointer") - } - rv = rv.Elem() - if rv.Type().Kind() != reflect.Struct { - panic("expect struct") - } - - a.strings = make(map[string]*stringField) - a.memo = make(map[string]string) - a.walk([]string{}, rv, nilSetter{}) -} - -// recursively interpolate variables in a depth first manner -func (a *accumulator) Resolve(path string, seenPaths []string, fns ...LookupFunction) error { - // return early if the path is already resolved - if _, ok := a.memo[path]; ok { - return nil - } - - // fetch the string node to resolve - field, ok := a.strings[path] - if !ok { - return fmt.Errorf("no value found for interpolation reference: ${%s}", path) - } - - // return early if the string field has no variables to interpolate - if len(field.dependsOn()) == 0 { - a.memo[path] = field.Get() - return nil - } - - // resolve all variables refered in the root string field - for _, childFieldPath := range field.dependsOn() { - // error if there is a loop in variable interpolation - if slices.Contains(seenPaths, childFieldPath) { - return fmt.Errorf("cycle detected in field resolution: %s", strings.Join(append(seenPaths, childFieldPath), " -> ")) - } - - // recursive resolve variables in the child fields - err := a.Resolve(childFieldPath, append(seenPaths, childFieldPath), fns...) - if err != nil { - return err - } - } - - // interpolate root string once all variable references in it have been resolved - field.interpolate(fns, a.memo) - - // record interpolated string in memo - a.memo[path] = field.Get() - return nil -} - -// Interpolate all string fields in the config -func (a *accumulator) expand(fns ...LookupFunction) error { - // sorting paths for stable order of iteration - paths := maps.Keys(a.strings) - sort.Strings(paths) - - // iterate over paths for all strings fields in the config - for _, path := range paths { - err := a.Resolve(path, []string{path}, fns...) - if err != nil { - return err - } - } - return nil -} - -type interpolate struct { - fns []LookupFunction -} - -func (m *interpolate) expand(v any) error { - a := accumulator{} - a.start(v) - return a.expand(m.fns...) -} - -func Interpolate(fns ...LookupFunction) bundle.Mutator { - return &interpolate{fns: fns} -} - -func (m *interpolate) Name() string { - return "Interpolate" -} - -func (m *interpolate) Apply(_ context.Context, b *bundle.Bundle) error { - return m.expand(&b.Config) -} diff --git a/bundle/config/interpolation/interpolation_test.go b/bundle/config/interpolation/interpolation_test.go deleted file mode 100644 index cccb6dc718..0000000000 --- a/bundle/config/interpolation/interpolation_test.go +++ /dev/null @@ -1,251 +0,0 @@ -package interpolation - -import ( - "testing" - - "github.com/databricks/cli/bundle/config" - "github.com/databricks/cli/bundle/config/variable" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -type nest struct { - X string `json:"x"` - Y *string `json:"y"` - Z map[string]string `json:"z"` -} - -type foo struct { - A string `json:"a"` - B string `json:"b"` - C string `json:"c"` - - // Pointer field - D *string `json:"d"` - - // Struct field - E nest `json:"e"` - - // Map field - F map[string]string `json:"f"` -} - -func expand(v any) error { - a := accumulator{} - a.start(v) - return a.expand(DefaultLookup) -} - -func TestInterpolationVariables(t *testing.T) { - f := foo{ - A: "a", - B: "${a}", - C: "${a}", - } - - err := expand(&f) - require.NoError(t, err) - - assert.Equal(t, "a", f.A) - assert.Equal(t, "a", f.B) - assert.Equal(t, "a", f.C) -} - -func TestInterpolationVariablesSpecialChars(t *testing.T) { - type bar struct { - A string `json:"a-b"` - B string `json:"b_c"` - C string `json:"c-_a"` - } - f := bar{ - A: "a", - B: "${a-b}", - C: "${a-b}", - } - - err := expand(&f) - require.NoError(t, err) - - assert.Equal(t, "a", f.A) - assert.Equal(t, "a", f.B) - assert.Equal(t, "a", f.C) -} - -func TestInterpolationValidMatches(t *testing.T) { - expectedMatches := map[string]string{ - "${hello_world.world_world}": "hello_world.world_world", - "${helloworld.world-world}": "helloworld.world-world", - "${hello-world.world-world}": "hello-world.world-world", - } - for interpolationStr, expectedMatch := range expectedMatches { - match := re.FindStringSubmatch(interpolationStr) - assert.True(t, len(match) > 0, - "Failed to match %s and find %s", interpolationStr, expectedMatch) - assert.Equal(t, expectedMatch, match[1], - "Failed to match the exact pattern %s and find %s", interpolationStr, expectedMatch) - } -} - -func TestInterpolationInvalidMatches(t *testing.T) { - invalidMatches := []string{ - "${hello_world-.world_world}", // the first segment ending must not end with hyphen (-) - "${hello_world-_.world_world}", // the first segment ending must not end with underscore (_) - "${helloworld.world-world-}", // second segment must not end with hyphen (-) - "${helloworld-.world-world}", // first segment must not end with hyphen (-) - "${helloworld.-world-world}", // second segment must not start with hyphen (-) - "${-hello-world.-world-world-}", // must not start or end with hyphen (-) - "${_-_._-_.id}", // cannot use _- in sequence - "${0helloworld.world-world}", // interpolated first section shouldn't start with number - "${helloworld.9world-world}", // interpolated second section shouldn't start with number - "${a-a.a-_a-a.id}", // fails because of -_ in the second segment - "${a-a.a--a-a.id}", // fails because of -- in the second segment - } - for _, invalidMatch := range invalidMatches { - match := re.FindStringSubmatch(invalidMatch) - assert.True(t, len(match) == 0, "Should be invalid interpolation: %s", invalidMatch) - } -} - -func TestInterpolationWithPointers(t *testing.T) { - fd := "${a}" - f := foo{ - A: "a", - D: &fd, - } - - err := expand(&f) - require.NoError(t, err) - - assert.Equal(t, "a", f.A) - assert.Equal(t, "a", *f.D) -} - -func TestInterpolationWithStruct(t *testing.T) { - fy := "${e.x}" - f := foo{ - A: "${e.x}", - E: nest{ - X: "x", - Y: &fy, - }, - } - - err := expand(&f) - require.NoError(t, err) - - assert.Equal(t, "x", f.A) - assert.Equal(t, "x", f.E.X) - assert.Equal(t, "x", *f.E.Y) -} - -func TestInterpolationWithMap(t *testing.T) { - f := foo{ - A: "${f.a}", - F: map[string]string{ - "a": "a", - "b": "${f.a}", - }, - } - - err := expand(&f) - require.NoError(t, err) - - assert.Equal(t, "a", f.A) - assert.Equal(t, "a", f.F["a"]) - assert.Equal(t, "a", f.F["b"]) -} - -func TestInterpolationWithResursiveVariableReferences(t *testing.T) { - f := foo{ - A: "a", - B: "(${a})", - C: "${a} ${b}", - } - - err := expand(&f) - require.NoError(t, err) - - assert.Equal(t, "a", f.A) - assert.Equal(t, "(a)", f.B) - assert.Equal(t, "a (a)", f.C) -} - -func TestInterpolationVariableLoopError(t *testing.T) { - d := "${b}" - f := foo{ - A: "a", - B: "${c}", - C: "${d}", - D: &d, - } - - err := expand(&f) - assert.ErrorContains(t, err, "cycle detected in field resolution: b -> c -> d -> b") -} - -func TestInterpolationForVariables(t *testing.T) { - foo := "abc" - bar := "${var.foo} def" - apple := "${var.foo} ${var.bar}" - config := config.Root{ - Variables: map[string]*variable.Variable{ - "foo": { - Value: &foo, - }, - "bar": { - Value: &bar, - }, - "apple": { - Value: &apple, - }, - }, - Bundle: config.Bundle{ - Name: "${var.apple} ${var.foo}", - }, - } - - err := expand(&config) - assert.NoError(t, err) - assert.Equal(t, "abc", *(config.Variables["foo"].Value)) - assert.Equal(t, "abc def", *(config.Variables["bar"].Value)) - assert.Equal(t, "abc abc def", *(config.Variables["apple"].Value)) - assert.Equal(t, "abc abc def abc", config.Bundle.Name) -} - -func TestInterpolationLoopForVariables(t *testing.T) { - foo := "${var.bar}" - bar := "${var.foo}" - config := config.Root{ - Variables: map[string]*variable.Variable{ - "foo": { - Value: &foo, - }, - "bar": { - Value: &bar, - }, - }, - Bundle: config.Bundle{ - Name: "${var.foo}", - }, - } - - err := expand(&config) - assert.ErrorContains(t, err, "cycle detected in field resolution: bundle.name -> var.foo -> var.bar -> var.foo") -} - -func TestInterpolationInvalidVariableReference(t *testing.T) { - foo := "abc" - config := config.Root{ - Variables: map[string]*variable.Variable{ - "foo": { - Value: &foo, - }, - }, - Bundle: config.Bundle{ - Name: "${vars.foo}", - }, - } - - err := expand(&config) - assert.ErrorContains(t, err, "no value found for interpolation reference: ${vars.foo}") -} diff --git a/bundle/config/interpolation/lookup.go b/bundle/config/interpolation/lookup.go deleted file mode 100644 index 3dc5047a75..0000000000 --- a/bundle/config/interpolation/lookup.go +++ /dev/null @@ -1,51 +0,0 @@ -package interpolation - -import ( - "errors" - "fmt" - "slices" - "strings" -) - -// LookupFunction returns the value to rewrite a path expression to. -type LookupFunction func(path string, depends map[string]string) (string, error) - -// ErrSkipInterpolation can be used to fall through from [LookupFunction]. -var ErrSkipInterpolation = errors.New("skip interpolation") - -// DefaultLookup looks up the specified path in the map. -// It returns an error if it doesn't exist. -func DefaultLookup(path string, lookup map[string]string) (string, error) { - v, ok := lookup[path] - if !ok { - return "", fmt.Errorf("expected to find value for path: %s", path) - } - return v, nil -} - -func pathPrefixMatches(prefix []string, path string) bool { - parts := strings.Split(path, Delimiter) - return len(parts) >= len(prefix) && slices.Compare(prefix, parts[0:len(prefix)]) == 0 -} - -// ExcludeLookupsInPath is a lookup function that skips lookups for the specified path. -func ExcludeLookupsInPath(exclude ...string) LookupFunction { - return func(path string, lookup map[string]string) (string, error) { - if pathPrefixMatches(exclude, path) { - return "", ErrSkipInterpolation - } - - return DefaultLookup(path, lookup) - } -} - -// IncludeLookupsInPath is a lookup function that limits lookups to the specified path. -func IncludeLookupsInPath(include ...string) LookupFunction { - return func(path string, lookup map[string]string) (string, error) { - if !pathPrefixMatches(include, path) { - return "", ErrSkipInterpolation - } - - return DefaultLookup(path, lookup) - } -} diff --git a/bundle/config/interpolation/lookup_test.go b/bundle/config/interpolation/lookup_test.go deleted file mode 100644 index 61628bf042..0000000000 --- a/bundle/config/interpolation/lookup_test.go +++ /dev/null @@ -1,81 +0,0 @@ -package interpolation - -import ( - "testing" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -type interpolationFixture struct { - A map[string]string `json:"a"` - B map[string]string `json:"b"` - C map[string]string `json:"c"` -} - -func fixture() interpolationFixture { - return interpolationFixture{ - A: map[string]string{ - "x": "1", - }, - B: map[string]string{ - "x": "2", - }, - C: map[string]string{ - "ax": "${a.x}", - "bx": "${b.x}", - }, - } -} - -func TestExcludePath(t *testing.T) { - tmp := fixture() - m := interpolate{ - fns: []LookupFunction{ - ExcludeLookupsInPath("a"), - }, - } - - err := m.expand(&tmp) - require.NoError(t, err) - - assert.Equal(t, "1", tmp.A["x"]) - assert.Equal(t, "2", tmp.B["x"]) - assert.Equal(t, "${a.x}", tmp.C["ax"]) - assert.Equal(t, "2", tmp.C["bx"]) -} - -func TestIncludePath(t *testing.T) { - tmp := fixture() - m := interpolate{ - fns: []LookupFunction{ - IncludeLookupsInPath("a"), - }, - } - - err := m.expand(&tmp) - require.NoError(t, err) - - assert.Equal(t, "1", tmp.A["x"]) - assert.Equal(t, "2", tmp.B["x"]) - assert.Equal(t, "1", tmp.C["ax"]) - assert.Equal(t, "${b.x}", tmp.C["bx"]) -} - -func TestIncludePathMultiple(t *testing.T) { - tmp := fixture() - m := interpolate{ - fns: []LookupFunction{ - IncludeLookupsInPath("a"), - IncludeLookupsInPath("b"), - }, - } - - err := m.expand(&tmp) - require.NoError(t, err) - - assert.Equal(t, "1", tmp.A["x"]) - assert.Equal(t, "2", tmp.B["x"]) - assert.Equal(t, "1", tmp.C["ax"]) - assert.Equal(t, "2", tmp.C["bx"]) -} diff --git a/bundle/config/interpolation/setter.go b/bundle/config/interpolation/setter.go deleted file mode 100644 index cce39c6111..0000000000 --- a/bundle/config/interpolation/setter.go +++ /dev/null @@ -1,48 +0,0 @@ -package interpolation - -import "reflect" - -// String values in maps are not addressable and therefore not settable -// through Go's reflection mechanism. This interface solves this limitation -// by wrapping the setter differently for addressable values and map values. -type setter interface { - Set(string) -} - -type nilSetter struct{} - -func (nilSetter) Set(_ string) { - panic("nil setter") -} - -type anySetter struct { - rv reflect.Value -} - -func (s anySetter) Set(str string) { - s.rv.SetString(str) -} - -type mapSetter struct { - // map[string]string - m reflect.Value - - // key - k reflect.Value -} - -func (s mapSetter) Set(str string) { - s.m.SetMapIndex(s.k, reflect.ValueOf(str)) -} - -type getter interface { - Get() string -} - -type anyGetter struct { - rv reflect.Value -} - -func (g anyGetter) Get() string { - return g.rv.String() -} diff --git a/bundle/config/mutator/environments_compat.go b/bundle/config/mutator/environments_compat.go new file mode 100644 index 0000000000..0eb996b14c --- /dev/null +++ b/bundle/config/mutator/environments_compat.go @@ -0,0 +1,63 @@ +package mutator + +import ( + "context" + "fmt" + + "github.com/databricks/cli/bundle" + "github.com/databricks/cli/libs/dyn" +) + +type environmentsToTargets struct{} + +func EnvironmentsToTargets() bundle.Mutator { + return &environmentsToTargets{} +} + +func (m *environmentsToTargets) Name() string { + return "EnvironmentsToTargets" +} + +func (m *environmentsToTargets) Apply(ctx context.Context, b *bundle.Bundle) error { + // Short circuit if the "environments" key is not set. + // This is the common case. + if b.Config.Environments == nil { + return nil + } + + // The "environments" key is set; validate and rewrite it to "targets". + return b.Config.Mutate(func(v dyn.Value) (dyn.Value, error) { + environments := v.Get("environments") + targets := v.Get("targets") + + // Return an error if both "environments" and "targets" are set. + if environments != dyn.NilValue && targets != dyn.NilValue { + return dyn.NilValue, fmt.Errorf( + "both 'environments' and 'targets' are specified; only 'targets' should be used: %s", + environments.Location().String(), + ) + } + + // Rewrite "environments" to "targets". + if environments != dyn.NilValue && targets == dyn.NilValue { + nv, err := dyn.Set(v, "targets", environments) + if err != nil { + return dyn.NilValue, err + } + // Drop the "environments" key. + return dyn.Walk(nv, func(p dyn.Path, v dyn.Value) (dyn.Value, error) { + switch len(p) { + case 0: + return v, nil + case 1: + if p[0] == dyn.Key("environments") { + return v, dyn.ErrDrop + } + } + return v, dyn.ErrSkip + }) + } + + return v, nil + }) +} diff --git a/bundle/config/mutator/environments_compat_test.go b/bundle/config/mutator/environments_compat_test.go new file mode 100644 index 0000000000..f7045b3df2 --- /dev/null +++ b/bundle/config/mutator/environments_compat_test.go @@ -0,0 +1,65 @@ +package mutator_test + +import ( + "context" + "testing" + + "github.com/databricks/cli/bundle" + "github.com/databricks/cli/bundle/config" + "github.com/databricks/cli/bundle/config/mutator" + "github.com/stretchr/testify/assert" +) + +func TestEnvironmentsToTargetsWithBothDefined(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Environments: map[string]*config.Target{ + "name": { + Mode: config.Development, + }, + }, + Targets: map[string]*config.Target{ + "name": { + Mode: config.Development, + }, + }, + }, + } + + err := bundle.Apply(context.Background(), b, mutator.EnvironmentsToTargets()) + assert.ErrorContains(t, err, `both 'environments' and 'targets' are specified;`) +} + +func TestEnvironmentsToTargetsWithEnvironmentsDefined(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Environments: map[string]*config.Target{ + "name": { + Mode: config.Development, + }, + }, + }, + } + + err := bundle.Apply(context.Background(), b, mutator.EnvironmentsToTargets()) + assert.NoError(t, err) + assert.Len(t, b.Config.Environments, 0) + assert.Len(t, b.Config.Targets, 1) +} + +func TestEnvironmentsToTargetsWithTargetsDefined(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Targets: map[string]*config.Target{ + "name": { + Mode: config.Development, + }, + }, + }, + } + + err := bundle.Apply(context.Background(), b, mutator.EnvironmentsToTargets()) + assert.NoError(t, err) + assert.Len(t, b.Config.Environments, 0) + assert.Len(t, b.Config.Targets, 1) +} diff --git a/bundle/config/mutator/expand_pipeline_glob_paths_test.go b/bundle/config/mutator/expand_pipeline_glob_paths_test.go index ad86865af7..e2cba80e25 100644 --- a/bundle/config/mutator/expand_pipeline_glob_paths_test.go +++ b/bundle/config/mutator/expand_pipeline_glob_paths_test.go @@ -8,8 +8,8 @@ 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/cli/bundle/internal/bundletest" "github.com/databricks/databricks-sdk-go/service/compute" "github.com/databricks/databricks-sdk-go/service/pipelines" "github.com/stretchr/testify/require" @@ -42,9 +42,6 @@ func TestExpandGlobPathsInPipelines(t *testing.T) { Resources: config.Resources{ Pipelines: map[string]*resources.Pipeline{ "pipeline": { - Paths: paths.Paths{ - ConfigFilePath: filepath.Join(dir, "resource.yml"), - }, PipelineSpec: &pipelines.PipelineSpec{ Libraries: []pipelines.PipelineLibrary{ { @@ -98,6 +95,8 @@ func TestExpandGlobPathsInPipelines(t *testing.T) { }, } + bundletest.SetLocation(b, ".", filepath.Join(dir, "resource.yml")) + m := ExpandPipelineGlobPaths() err := bundle.Apply(context.Background(), b, m) require.NoError(t, err) diff --git a/bundle/config/mutator/merge_job_clusters.go b/bundle/config/mutator/merge_job_clusters.go new file mode 100644 index 0000000000..e8378f4801 --- /dev/null +++ b/bundle/config/mutator/merge_job_clusters.go @@ -0,0 +1,42 @@ +package mutator + +import ( + "context" + + "github.com/databricks/cli/bundle" + "github.com/databricks/cli/libs/dyn" + "github.com/databricks/cli/libs/dyn/merge" +) + +type mergeJobClusters struct{} + +func MergeJobClusters() bundle.Mutator { + return &mergeJobClusters{} +} + +func (m *mergeJobClusters) Name() string { + return "MergeJobClusters" +} + +func (m *mergeJobClusters) jobClusterKey(v dyn.Value) string { + switch v.Kind() { + case dyn.KindNil: + return "" + case dyn.KindString: + return v.MustString() + default: + panic("job cluster key must be a string") + } +} + +func (m *mergeJobClusters) Apply(ctx context.Context, b *bundle.Bundle) error { + return b.Config.Mutate(func(v dyn.Value) (dyn.Value, error) { + if v == dyn.NilValue { + return v, nil + } + + return dyn.Map(v, "resources.jobs", dyn.Foreach(func(job dyn.Value) (dyn.Value, error) { + return dyn.Map(job, "job_clusters", merge.ElementsByKey("job_cluster_key", m.jobClusterKey)) + })) + }) +} diff --git a/bundle/config/mutator/merge_job_clusters_test.go b/bundle/config/mutator/merge_job_clusters_test.go new file mode 100644 index 0000000000..a32b70281f --- /dev/null +++ b/bundle/config/mutator/merge_job_clusters_test.go @@ -0,0 +1,105 @@ +package mutator_test + +import ( + "context" + "testing" + + "github.com/databricks/cli/bundle" + "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/stretchr/testify/assert" +) + +func TestMergeJobClusters(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Resources: config.Resources{ + Jobs: map[string]*resources.Job{ + "foo": { + JobSettings: &jobs.JobSettings{ + JobClusters: []jobs.JobCluster{ + { + JobClusterKey: "foo", + NewCluster: &compute.ClusterSpec{ + SparkVersion: "13.3.x-scala2.12", + NodeTypeId: "i3.xlarge", + NumWorkers: 2, + }, + }, + { + JobClusterKey: "bar", + NewCluster: &compute.ClusterSpec{ + SparkVersion: "10.4.x-scala2.12", + }, + }, + { + JobClusterKey: "foo", + NewCluster: &compute.ClusterSpec{ + NodeTypeId: "i3.2xlarge", + NumWorkers: 4, + }, + }, + }, + }, + }, + }, + }, + }, + } + + err := bundle.Apply(context.Background(), b, mutator.MergeJobClusters()) + assert.NoError(t, err) + + j := b.Config.Resources.Jobs["foo"] + + assert.Len(t, j.JobClusters, 2) + assert.Equal(t, "foo", j.JobClusters[0].JobClusterKey) + assert.Equal(t, "bar", j.JobClusters[1].JobClusterKey) + + // This job cluster was merged with a subsequent one. + jc0 := j.JobClusters[0].NewCluster + assert.Equal(t, "13.3.x-scala2.12", jc0.SparkVersion) + assert.Equal(t, "i3.2xlarge", jc0.NodeTypeId) + assert.Equal(t, 4, jc0.NumWorkers) + + // This job cluster was left untouched. + jc1 := j.JobClusters[1].NewCluster + assert.Equal(t, "10.4.x-scala2.12", jc1.SparkVersion) +} + +func TestMergeJobClustersWithNilKey(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Resources: config.Resources{ + Jobs: map[string]*resources.Job{ + "foo": { + JobSettings: &jobs.JobSettings{ + JobClusters: []jobs.JobCluster{ + { + NewCluster: &compute.ClusterSpec{ + SparkVersion: "13.3.x-scala2.12", + NodeTypeId: "i3.xlarge", + NumWorkers: 2, + }, + }, + { + NewCluster: &compute.ClusterSpec{ + NodeTypeId: "i3.2xlarge", + NumWorkers: 4, + }, + }, + }, + }, + }, + }, + }, + }, + } + + err := bundle.Apply(context.Background(), b, mutator.MergeJobClusters()) + assert.NoError(t, err) + assert.Len(t, b.Config.Resources.Jobs["foo"].JobClusters, 1) +} diff --git a/bundle/config/mutator/merge_job_tasks.go b/bundle/config/mutator/merge_job_tasks.go new file mode 100644 index 0000000000..7394368ab8 --- /dev/null +++ b/bundle/config/mutator/merge_job_tasks.go @@ -0,0 +1,42 @@ +package mutator + +import ( + "context" + + "github.com/databricks/cli/bundle" + "github.com/databricks/cli/libs/dyn" + "github.com/databricks/cli/libs/dyn/merge" +) + +type mergeJobTasks struct{} + +func MergeJobTasks() bundle.Mutator { + return &mergeJobTasks{} +} + +func (m *mergeJobTasks) Name() string { + return "MergeJobTasks" +} + +func (m *mergeJobTasks) taskKeyString(v dyn.Value) string { + switch v.Kind() { + case dyn.KindNil: + return "" + case dyn.KindString: + return v.MustString() + default: + panic("task key must be a string") + } +} + +func (m *mergeJobTasks) Apply(ctx context.Context, b *bundle.Bundle) error { + return b.Config.Mutate(func(v dyn.Value) (dyn.Value, error) { + if v == dyn.NilValue { + return v, nil + } + + return dyn.Map(v, "resources.jobs", dyn.Foreach(func(job dyn.Value) (dyn.Value, error) { + return dyn.Map(job, "tasks", merge.ElementsByKey("task_key", m.taskKeyString)) + })) + }) +} diff --git a/bundle/config/mutator/merge_job_tasks_test.go b/bundle/config/mutator/merge_job_tasks_test.go new file mode 100644 index 0000000000..b3fb357e0b --- /dev/null +++ b/bundle/config/mutator/merge_job_tasks_test.go @@ -0,0 +1,117 @@ +package mutator_test + +import ( + "context" + "testing" + + "github.com/databricks/cli/bundle" + "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/stretchr/testify/assert" +) + +func TestMergeJobTasks(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Resources: config.Resources{ + Jobs: map[string]*resources.Job{ + "foo": { + JobSettings: &jobs.JobSettings{ + Tasks: []jobs.Task{ + { + TaskKey: "foo", + NewCluster: &compute.ClusterSpec{ + SparkVersion: "13.3.x-scala2.12", + NodeTypeId: "i3.xlarge", + NumWorkers: 2, + }, + Libraries: []compute.Library{ + {Whl: "package1"}, + }, + }, + { + TaskKey: "bar", + NewCluster: &compute.ClusterSpec{ + SparkVersion: "10.4.x-scala2.12", + }, + }, + { + TaskKey: "foo", + NewCluster: &compute.ClusterSpec{ + NodeTypeId: "i3.2xlarge", + NumWorkers: 4, + }, + Libraries: []compute.Library{ + {Pypi: &compute.PythonPyPiLibrary{ + Package: "package2", + }}, + }, + }, + }, + }, + }, + }, + }, + }, + } + + err := bundle.Apply(context.Background(), b, mutator.MergeJobTasks()) + assert.NoError(t, err) + + j := b.Config.Resources.Jobs["foo"] + + assert.Len(t, j.Tasks, 2) + assert.Equal(t, "foo", j.Tasks[0].TaskKey) + assert.Equal(t, "bar", j.Tasks[1].TaskKey) + + // This task was merged with a subsequent one. + task0 := j.Tasks[0] + cluster := task0.NewCluster + assert.Equal(t, "13.3.x-scala2.12", cluster.SparkVersion) + assert.Equal(t, "i3.2xlarge", cluster.NodeTypeId) + assert.Equal(t, 4, cluster.NumWorkers) + assert.Len(t, task0.Libraries, 2) + assert.Equal(t, task0.Libraries[0].Whl, "package1") + assert.Equal(t, task0.Libraries[1].Pypi.Package, "package2") + + // This task was left untouched. + task1 := j.Tasks[1].NewCluster + assert.Equal(t, "10.4.x-scala2.12", task1.SparkVersion) +} + +func TestMergeJobTasksWithNilKey(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Resources: config.Resources{ + Jobs: map[string]*resources.Job{ + "foo": { + JobSettings: &jobs.JobSettings{ + Tasks: []jobs.Task{ + { + NewCluster: &compute.ClusterSpec{ + SparkVersion: "13.3.x-scala2.12", + NodeTypeId: "i3.xlarge", + NumWorkers: 2, + }, + }, + { + NewCluster: &compute.ClusterSpec{ + NodeTypeId: "i3.2xlarge", + NumWorkers: 4, + }, + }, + }, + }, + }, + }, + }, + }, + } + + err := bundle.Apply(context.Background(), b, mutator.MergeJobTasks()) + assert.NoError(t, err) + assert.Len(t, b.Config.Resources.Jobs["foo"].Tasks, 1) +} diff --git a/bundle/config/mutator/merge_pipeline_clusters.go b/bundle/config/mutator/merge_pipeline_clusters.go new file mode 100644 index 0000000000..777ce611bf --- /dev/null +++ b/bundle/config/mutator/merge_pipeline_clusters.go @@ -0,0 +1,45 @@ +package mutator + +import ( + "context" + "strings" + + "github.com/databricks/cli/bundle" + "github.com/databricks/cli/libs/dyn" + "github.com/databricks/cli/libs/dyn/merge" +) + +type mergePipelineClusters struct{} + +func MergePipelineClusters() bundle.Mutator { + return &mergePipelineClusters{} +} + +func (m *mergePipelineClusters) Name() string { + return "MergePipelineClusters" +} + +func (m *mergePipelineClusters) clusterLabel(v dyn.Value) string { + switch v.Kind() { + case dyn.KindNil: + // Note: the cluster label is optional and defaults to 'default'. + // We therefore ALSO merge all clusters without a label. + return "default" + case dyn.KindString: + return strings.ToLower(v.MustString()) + default: + panic("task key must be a string") + } +} + +func (m *mergePipelineClusters) Apply(ctx context.Context, b *bundle.Bundle) error { + return b.Config.Mutate(func(v dyn.Value) (dyn.Value, error) { + if v == dyn.NilValue { + return v, nil + } + + return dyn.Map(v, "resources.pipelines", dyn.Foreach(func(pipeline dyn.Value) (dyn.Value, error) { + return dyn.Map(pipeline, "clusters", merge.ElementsByKey("label", m.clusterLabel)) + })) + }) +} diff --git a/bundle/config/mutator/merge_pipeline_clusters_test.go b/bundle/config/mutator/merge_pipeline_clusters_test.go new file mode 100644 index 0000000000..fb54a67d24 --- /dev/null +++ b/bundle/config/mutator/merge_pipeline_clusters_test.go @@ -0,0 +1,125 @@ +package mutator_test + +import ( + "context" + "strings" + "testing" + + "github.com/databricks/cli/bundle" + "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/pipelines" + "github.com/stretchr/testify/assert" +) + +func TestMergePipelineClusters(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Resources: config.Resources{ + Pipelines: map[string]*resources.Pipeline{ + "foo": { + PipelineSpec: &pipelines.PipelineSpec{ + Clusters: []pipelines.PipelineCluster{ + { + NodeTypeId: "i3.xlarge", + NumWorkers: 2, + PolicyId: "1234", + }, + { + Label: "maintenance", + NodeTypeId: "i3.2xlarge", + }, + { + NodeTypeId: "i3.2xlarge", + NumWorkers: 4, + }, + }, + }, + }, + }, + }, + }, + } + + err := bundle.Apply(context.Background(), b, mutator.MergePipelineClusters()) + assert.NoError(t, err) + + p := b.Config.Resources.Pipelines["foo"] + + assert.Len(t, p.Clusters, 2) + assert.Equal(t, "default", p.Clusters[0].Label) + assert.Equal(t, "maintenance", p.Clusters[1].Label) + + // The default cluster was merged with a subsequent one. + pc0 := p.Clusters[0] + assert.Equal(t, "i3.2xlarge", pc0.NodeTypeId) + assert.Equal(t, 4, pc0.NumWorkers) + assert.Equal(t, "1234", pc0.PolicyId) + + // The maintenance cluster was left untouched. + pc1 := p.Clusters[1] + assert.Equal(t, "i3.2xlarge", pc1.NodeTypeId) +} + +func TestMergePipelineClustersCaseInsensitive(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Resources: config.Resources{ + Pipelines: map[string]*resources.Pipeline{ + "foo": { + PipelineSpec: &pipelines.PipelineSpec{ + Clusters: []pipelines.PipelineCluster{ + { + Label: "default", + NumWorkers: 2, + }, + { + Label: "DEFAULT", + NumWorkers: 4, + }, + }, + }, + }, + }, + }, + }, + } + + err := bundle.Apply(context.Background(), b, mutator.MergePipelineClusters()) + assert.NoError(t, err) + + p := b.Config.Resources.Pipelines["foo"] + assert.Len(t, p.Clusters, 1) + + // The default cluster was merged with a subsequent one. + pc0 := p.Clusters[0] + assert.Equal(t, "default", strings.ToLower(pc0.Label)) + assert.Equal(t, 4, pc0.NumWorkers) +} + +func TestMergePipelineClustersNilPipelines(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Resources: config.Resources{ + Pipelines: nil, + }, + }, + } + + err := bundle.Apply(context.Background(), b, mutator.MergePipelineClusters()) + assert.NoError(t, err) +} + +func TestMergePipelineClustersEmptyPipelines(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Resources: config.Resources{ + Pipelines: map[string]*resources.Pipeline{}, + }, + }, + } + + err := bundle.Apply(context.Background(), b, mutator.MergePipelineClusters()) + assert.NoError(t, err) +} diff --git a/bundle/config/mutator/mutator.go b/bundle/config/mutator/mutator.go index b6327e8593..c45a6c15e1 100644 --- a/bundle/config/mutator/mutator.go +++ b/bundle/config/mutator/mutator.go @@ -10,12 +10,16 @@ func DefaultMutators() []bundle.Mutator { return []bundle.Mutator{ scripts.Execute(config.ScriptPreInit), ProcessRootIncludes(), + EnvironmentsToTargets(), InitializeVariables(), DefineDefaultTarget(), LoadGitDetails(), } } -func DefaultMutatorsForTarget(env string) []bundle.Mutator { - return append(DefaultMutators(), SelectTarget(env)) +func DefaultMutatorsForTarget(target string) []bundle.Mutator { + return append( + DefaultMutators(), + SelectTarget(target), + ) } diff --git a/bundle/config/mutator/override_compute_test.go b/bundle/config/mutator/override_compute_test.go index 4c5d4427db..7cc500c608 100644 --- a/bundle/config/mutator/override_compute_test.go +++ b/bundle/config/mutator/override_compute_test.go @@ -28,7 +28,9 @@ func TestOverrideDevelopment(t *testing.T) { Name: "job1", Tasks: []jobs.Task{ { - NewCluster: &compute.ClusterSpec{}, + NewCluster: &compute.ClusterSpec{ + SparkVersion: "14.2.x-scala2.12", + }, }, { ExistingClusterId: "cluster2", diff --git a/bundle/config/mutator/process_target_mode_test.go b/bundle/config/mutator/process_target_mode_test.go index f02d788657..6d80258039 100644 --- a/bundle/config/mutator/process_target_mode_test.go +++ b/bundle/config/mutator/process_target_mode_test.go @@ -274,12 +274,12 @@ func TestAllResourcesMocked(t *testing.T) { // Make sure that we at least rename all resources func TestAllResourcesRenamed(t *testing.T) { b := mockBundle(config.Development) - resources := reflect.ValueOf(b.Config.Resources) m := ProcessTargetMode() err := bundle.Apply(context.Background(), b, m) require.NoError(t, err) + resources := reflect.ValueOf(b.Config.Resources) for i := 0; i < resources.NumField(); i++ { field := resources.Field(i) diff --git a/bundle/config/mutator/resolve_variable_references.go b/bundle/config/mutator/resolve_variable_references.go new file mode 100644 index 0000000000..a9ff70f68f --- /dev/null +++ b/bundle/config/mutator/resolve_variable_references.go @@ -0,0 +1,81 @@ +package mutator + +import ( + "context" + + "github.com/databricks/cli/bundle" + "github.com/databricks/cli/libs/dyn" + "github.com/databricks/cli/libs/dyn/convert" + "github.com/databricks/cli/libs/dyn/dynvar" +) + +type resolveVariableReferences struct { + prefixes []string +} + +func ResolveVariableReferences(prefixes ...string) bundle.Mutator { + return &resolveVariableReferences{prefixes: prefixes} +} + +func (*resolveVariableReferences) Name() string { + return "ResolveVariableReferences" +} + +func (m *resolveVariableReferences) Validate(ctx context.Context, b *bundle.Bundle) error { + return nil +} + +func (m *resolveVariableReferences) Apply(ctx context.Context, b *bundle.Bundle) error { + prefixes := make([]dyn.Path, len(m.prefixes)) + for i, prefix := range m.prefixes { + prefixes[i] = dyn.MustPathFromString(prefix) + } + + // The path ${var.foo} is a shorthand for ${variables.foo.value}. + // We rewrite it here to make the resolution logic simpler. + varPath := dyn.NewPath(dyn.Key("var")) + + return b.Config.Mutate(func(root dyn.Value) (dyn.Value, error) { + // Synthesize a copy of the root that has all fields that are present in the type + // but not set in the dynamic value set to their corresponding empty value. + // This enables users to interpolate variable references to fields that haven't + // been explicitly set in the dynamic value. + // + // For example: ${bundle.git.origin_url} should resolve to an empty string + // if a bundle isn't located in a Git repository (yet). + // + // This is consistent with the behavior prior to using the dynamic value system. + // + // We can ignore the diagnostics return valuebecause we know that the dynamic value + // has already been normalized when it was first loaded from the configuration file. + // + normalized, _ := convert.Normalize(b.Config, root, convert.IncludeMissingFields) + lookup := func(path dyn.Path) (dyn.Value, error) { + // Future opportunity: if we lookup this path in both the given root + // and the synthesized root, we know if it was explicitly set or implied to be empty. + // Then we can emit a warning if it was not explicitly set. + return dyn.GetByPath(normalized, path) + } + + // Resolve variable references in all values. + return dynvar.Resolve(root, func(path dyn.Path) (dyn.Value, error) { + // Rewrite the shorthand path ${var.foo} into ${variables.foo.value}. + if path.HasPrefix(varPath) && len(path) == 2 { + path = dyn.NewPath( + dyn.Key("variables"), + path[1], + dyn.Key("value"), + ) + } + + // Perform resolution only if the path starts with one of the specified prefixes. + for _, prefix := range prefixes { + if path.HasPrefix(prefix) { + return lookup(path) + } + } + + return dyn.InvalidValue, dynvar.ErrSkipResolution + }) + }) +} diff --git a/bundle/config/mutator/resolve_variable_references_test.go b/bundle/config/mutator/resolve_variable_references_test.go new file mode 100644 index 0000000000..1f253d41c6 --- /dev/null +++ b/bundle/config/mutator/resolve_variable_references_test.go @@ -0,0 +1,97 @@ +package mutator + +import ( + "context" + "testing" + + "github.com/databricks/cli/bundle" + "github.com/databricks/cli/bundle/config" + "github.com/databricks/cli/bundle/config/resources" + "github.com/databricks/cli/bundle/config/variable" + "github.com/databricks/databricks-sdk-go/service/jobs" + "github.com/stretchr/testify/require" +) + +func TestResolveVariableReferences(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Bundle: config.Bundle{ + Name: "example", + }, + Workspace: config.Workspace{ + RootPath: "${bundle.name}/bar", + FilePath: "${workspace.root_path}/baz", + }, + }, + } + + // Apply with an invalid prefix. This should not change the workspace root path. + err := bundle.Apply(context.Background(), b, ResolveVariableReferences("doesntexist")) + require.NoError(t, err) + require.Equal(t, "${bundle.name}/bar", b.Config.Workspace.RootPath) + require.Equal(t, "${workspace.root_path}/baz", b.Config.Workspace.FilePath) + + // Apply with a valid prefix. This should change the workspace root path. + err = bundle.Apply(context.Background(), b, ResolveVariableReferences("bundle", "workspace")) + require.NoError(t, err) + require.Equal(t, "example/bar", b.Config.Workspace.RootPath) + require.Equal(t, "example/bar/baz", b.Config.Workspace.FilePath) +} + +func TestResolveVariableReferencesToBundleVariables(t *testing.T) { + s := func(s string) *string { + return &s + } + + b := &bundle.Bundle{ + Config: config.Root{ + Bundle: config.Bundle{ + Name: "example", + }, + Workspace: config.Workspace{ + RootPath: "${bundle.name}/${var.foo}", + }, + Variables: map[string]*variable.Variable{ + "foo": { + Value: s("bar"), + }, + }, + }, + } + + // Apply with a valid prefix. This should change the workspace root path. + err := bundle.Apply(context.Background(), b, ResolveVariableReferences("bundle", "variables")) + require.NoError(t, err) + require.Equal(t, "example/bar", b.Config.Workspace.RootPath) +} + +func TestResolveVariableReferencesToEmptyFields(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Bundle: config.Bundle{ + Name: "example", + Git: config.Git{ + Branch: "", + }, + }, + Resources: config.Resources{ + Jobs: map[string]*resources.Job{ + "job1": { + JobSettings: &jobs.JobSettings{ + Tags: map[string]string{ + "git_branch": "${bundle.git.branch}", + }, + }, + }, + }, + }, + }, + } + + // Apply for the bundle prefix. + err := bundle.Apply(context.Background(), b, ResolveVariableReferences("bundle")) + require.NoError(t, err) + + // The job settings should have been interpolated to an empty string. + require.Equal(t, "", b.Config.Resources.Jobs["job1"].JobSettings.Tags["git_branch"]) +} diff --git a/bundle/config/mutator/rewrite_sync_paths.go b/bundle/config/mutator/rewrite_sync_paths.go new file mode 100644 index 0000000000..c1761690d4 --- /dev/null +++ b/bundle/config/mutator/rewrite_sync_paths.go @@ -0,0 +1,58 @@ +package mutator + +import ( + "context" + "path/filepath" + + "github.com/databricks/cli/bundle" + + "github.com/databricks/cli/libs/dyn" +) + +type rewriteSyncPaths struct{} + +func RewriteSyncPaths() bundle.Mutator { + return &rewriteSyncPaths{} +} + +func (m *rewriteSyncPaths) Name() string { + return "RewriteSyncPaths" +} + +// makeRelativeTo returns a dyn.MapFunc that joins the relative path +// of the file it was defined in w.r.t. the bundle root path, with +// the contents of the string node. +// +// For example: +// - The bundle root is /foo +// - The configuration file that defines the string node is at /foo/bar/baz.yml +// - The string node contains "somefile.*" +// +// Then the resulting value will be "bar/somefile.*". +func (m *rewriteSyncPaths) makeRelativeTo(root string) dyn.MapFunc { + return func(v dyn.Value) (dyn.Value, error) { + dir := filepath.Dir(v.Location().File) + rel, err := filepath.Rel(root, dir) + if err != nil { + return dyn.NilValue, err + } + + return dyn.NewValue(filepath.Join(rel, v.MustString()), v.Location()), nil + } +} + +func (m *rewriteSyncPaths) Apply(ctx context.Context, b *bundle.Bundle) error { + return b.Config.Mutate(func(v dyn.Value) (dyn.Value, error) { + return dyn.Map(v, "sync", func(v dyn.Value) (nv dyn.Value, err error) { + v, err = dyn.Map(v, "include", dyn.Foreach(m.makeRelativeTo(b.Config.Path))) + if err != nil { + return dyn.NilValue, err + } + v, err = dyn.Map(v, "exclude", dyn.Foreach(m.makeRelativeTo(b.Config.Path))) + if err != nil { + return dyn.NilValue, err + } + return v, nil + }) + }) +} diff --git a/bundle/config/mutator/rewrite_sync_paths_test.go b/bundle/config/mutator/rewrite_sync_paths_test.go new file mode 100644 index 0000000000..576333e928 --- /dev/null +++ b/bundle/config/mutator/rewrite_sync_paths_test.go @@ -0,0 +1,103 @@ +package mutator_test + +import ( + "context" + "path/filepath" + "testing" + + "github.com/databricks/cli/bundle" + "github.com/databricks/cli/bundle/config" + "github.com/databricks/cli/bundle/config/mutator" + "github.com/databricks/cli/bundle/internal/bundletest" + "github.com/stretchr/testify/assert" +) + +func TestRewriteSyncPathsRelative(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Path: ".", + Sync: config.Sync{ + Include: []string{ + "foo", + "bar", + }, + Exclude: []string{ + "baz", + "qux", + }, + }, + }, + } + + bundletest.SetLocation(b, "sync.include[0]", "./file.yml") + bundletest.SetLocation(b, "sync.include[1]", "./a/file.yml") + bundletest.SetLocation(b, "sync.exclude[0]", "./a/b/file.yml") + bundletest.SetLocation(b, "sync.exclude[1]", "./a/b/c/file.yml") + + err := bundle.Apply(context.Background(), b, mutator.RewriteSyncPaths()) + assert.NoError(t, err) + + assert.Equal(t, filepath.Clean("foo"), b.Config.Sync.Include[0]) + assert.Equal(t, filepath.Clean("a/bar"), b.Config.Sync.Include[1]) + assert.Equal(t, filepath.Clean("a/b/baz"), b.Config.Sync.Exclude[0]) + assert.Equal(t, filepath.Clean("a/b/c/qux"), b.Config.Sync.Exclude[1]) +} + +func TestRewriteSyncPathsAbsolute(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Path: "/tmp/dir", + Sync: config.Sync{ + Include: []string{ + "foo", + "bar", + }, + Exclude: []string{ + "baz", + "qux", + }, + }, + }, + } + + bundletest.SetLocation(b, "sync.include[0]", "/tmp/dir/file.yml") + bundletest.SetLocation(b, "sync.include[1]", "/tmp/dir/a/file.yml") + bundletest.SetLocation(b, "sync.exclude[0]", "/tmp/dir/a/b/file.yml") + bundletest.SetLocation(b, "sync.exclude[1]", "/tmp/dir/a/b/c/file.yml") + + err := bundle.Apply(context.Background(), b, mutator.RewriteSyncPaths()) + assert.NoError(t, err) + + assert.Equal(t, filepath.Clean("foo"), b.Config.Sync.Include[0]) + assert.Equal(t, filepath.Clean("a/bar"), b.Config.Sync.Include[1]) + assert.Equal(t, filepath.Clean("a/b/baz"), b.Config.Sync.Exclude[0]) + assert.Equal(t, filepath.Clean("a/b/c/qux"), b.Config.Sync.Exclude[1]) +} + +func TestRewriteSyncPathsErrorPaths(t *testing.T) { + t.Run("no sync block", func(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Path: ".", + }, + } + + err := bundle.Apply(context.Background(), b, mutator.RewriteSyncPaths()) + assert.NoError(t, err) + }) + + t.Run("empty include/exclude blocks", func(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Path: ".", + Sync: config.Sync{ + Include: []string{}, + Exclude: []string{}, + }, + }, + } + + err := bundle.Apply(context.Background(), b, mutator.RewriteSyncPaths()) + assert.NoError(t, err) + }) +} diff --git a/bundle/config/mutator/select_target.go b/bundle/config/mutator/select_target.go index 2ad4311280..95558f030f 100644 --- a/bundle/config/mutator/select_target.go +++ b/bundle/config/mutator/select_target.go @@ -30,13 +30,13 @@ func (m *selectTarget) Apply(_ context.Context, b *bundle.Bundle) error { } // Get specified target - target, ok := b.Config.Targets[m.name] + _, ok := b.Config.Targets[m.name] if !ok { return fmt.Errorf("%s: no such target. Available targets: %s", m.name, strings.Join(maps.Keys(b.Config.Targets), ", ")) } // Merge specified target into root configuration structure. - err := b.Config.MergeTargetOverrides(target) + err := b.Config.MergeTargetOverrides(m.name) if err != nil { return err } diff --git a/bundle/config/mutator/translate_paths_test.go b/bundle/config/mutator/translate_paths_test.go index 67f15d4076..96ff88f3f0 100644 --- a/bundle/config/mutator/translate_paths_test.go +++ b/bundle/config/mutator/translate_paths_test.go @@ -9,8 +9,8 @@ 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/cli/bundle/internal/bundletest" "github.com/databricks/databricks-sdk-go/service/compute" "github.com/databricks/databricks-sdk-go/service/jobs" "github.com/databricks/databricks-sdk-go/service/pipelines" @@ -44,10 +44,6 @@ func TestTranslatePathsSkippedWithGitSource(t *testing.T) { Resources: config.Resources{ Jobs: map[string]*resources.Job{ "job": { - - Paths: paths.Paths{ - ConfigFilePath: filepath.Join(dir, "resource.yml"), - }, JobSettings: &jobs.JobSettings{ GitSource: &jobs.GitSource{ GitBranch: "somebranch", @@ -80,6 +76,8 @@ func TestTranslatePathsSkippedWithGitSource(t *testing.T) { }, } + bundletest.SetLocation(b, ".", filepath.Join(dir, "resource.yml")) + err := bundle.Apply(context.Background(), b, mutator.TranslatePaths()) require.NoError(t, err) @@ -116,9 +114,6 @@ func TestTranslatePaths(t *testing.T) { Resources: config.Resources{ Jobs: map[string]*resources.Job{ "job": { - Paths: paths.Paths{ - ConfigFilePath: filepath.Join(dir, "resource.yml"), - }, JobSettings: &jobs.JobSettings{ Tasks: []jobs.Task{ { @@ -171,9 +166,6 @@ func TestTranslatePaths(t *testing.T) { }, Pipelines: map[string]*resources.Pipeline{ "pipeline": { - Paths: paths.Paths{ - ConfigFilePath: filepath.Join(dir, "resource.yml"), - }, PipelineSpec: &pipelines.PipelineSpec{ Libraries: []pipelines.PipelineLibrary{ { @@ -207,6 +199,8 @@ func TestTranslatePaths(t *testing.T) { }, } + bundletest.SetLocation(b, ".", filepath.Join(dir, "resource.yml")) + err := bundle.Apply(context.Background(), b, mutator.TranslatePaths()) require.NoError(t, err) @@ -287,9 +281,6 @@ func TestTranslatePathsInSubdirectories(t *testing.T) { Resources: config.Resources{ Jobs: map[string]*resources.Job{ "job": { - Paths: paths.Paths{ - ConfigFilePath: filepath.Join(dir, "job/resource.yml"), - }, JobSettings: &jobs.JobSettings{ Tasks: []jobs.Task{ { @@ -323,10 +314,6 @@ func TestTranslatePathsInSubdirectories(t *testing.T) { }, Pipelines: map[string]*resources.Pipeline{ "pipeline": { - Paths: paths.Paths{ - ConfigFilePath: filepath.Join(dir, "pipeline/resource.yml"), - }, - PipelineSpec: &pipelines.PipelineSpec{ Libraries: []pipelines.PipelineLibrary{ { @@ -342,6 +329,9 @@ func TestTranslatePathsInSubdirectories(t *testing.T) { }, } + bundletest.SetLocation(b, "resources.jobs", filepath.Join(dir, "job/resource.yml")) + bundletest.SetLocation(b, "resources.pipelines", filepath.Join(dir, "pipeline/resource.yml")) + err := bundle.Apply(context.Background(), b, mutator.TranslatePaths()) require.NoError(t, err) @@ -385,9 +375,6 @@ func TestTranslatePathsOutsideBundleRoot(t *testing.T) { Resources: config.Resources{ Jobs: map[string]*resources.Job{ "job": { - Paths: paths.Paths{ - ConfigFilePath: filepath.Join(dir, "../resource.yml"), - }, JobSettings: &jobs.JobSettings{ Tasks: []jobs.Task{ { @@ -403,6 +390,8 @@ func TestTranslatePathsOutsideBundleRoot(t *testing.T) { }, } + bundletest.SetLocation(b, ".", filepath.Join(dir, "../resource.yml")) + err := bundle.Apply(context.Background(), b, mutator.TranslatePaths()) assert.ErrorContains(t, err, "is not contained in bundle root") } @@ -416,9 +405,6 @@ func TestJobNotebookDoesNotExistError(t *testing.T) { Resources: config.Resources{ Jobs: map[string]*resources.Job{ "job": { - Paths: paths.Paths{ - ConfigFilePath: filepath.Join(dir, "fake.yml"), - }, JobSettings: &jobs.JobSettings{ Tasks: []jobs.Task{ { @@ -434,6 +420,8 @@ func TestJobNotebookDoesNotExistError(t *testing.T) { }, } + bundletest.SetLocation(b, ".", filepath.Join(dir, "fake.yml")) + err := bundle.Apply(context.Background(), b, mutator.TranslatePaths()) assert.EqualError(t, err, "notebook ./doesnt_exist.py not found") } @@ -447,9 +435,6 @@ func TestJobFileDoesNotExistError(t *testing.T) { Resources: config.Resources{ Jobs: map[string]*resources.Job{ "job": { - Paths: paths.Paths{ - ConfigFilePath: filepath.Join(dir, "fake.yml"), - }, JobSettings: &jobs.JobSettings{ Tasks: []jobs.Task{ { @@ -465,6 +450,8 @@ func TestJobFileDoesNotExistError(t *testing.T) { }, } + bundletest.SetLocation(b, ".", filepath.Join(dir, "fake.yml")) + err := bundle.Apply(context.Background(), b, mutator.TranslatePaths()) assert.EqualError(t, err, "file ./doesnt_exist.py not found") } @@ -478,9 +465,6 @@ func TestPipelineNotebookDoesNotExistError(t *testing.T) { Resources: config.Resources{ Pipelines: map[string]*resources.Pipeline{ "pipeline": { - Paths: paths.Paths{ - ConfigFilePath: filepath.Join(dir, "fake.yml"), - }, PipelineSpec: &pipelines.PipelineSpec{ Libraries: []pipelines.PipelineLibrary{ { @@ -496,6 +480,8 @@ func TestPipelineNotebookDoesNotExistError(t *testing.T) { }, } + bundletest.SetLocation(b, ".", filepath.Join(dir, "fake.yml")) + err := bundle.Apply(context.Background(), b, mutator.TranslatePaths()) assert.EqualError(t, err, "notebook ./doesnt_exist.py not found") } @@ -509,9 +495,6 @@ func TestPipelineFileDoesNotExistError(t *testing.T) { Resources: config.Resources{ Pipelines: map[string]*resources.Pipeline{ "pipeline": { - Paths: paths.Paths{ - ConfigFilePath: filepath.Join(dir, "fake.yml"), - }, PipelineSpec: &pipelines.PipelineSpec{ Libraries: []pipelines.PipelineLibrary{ { @@ -527,6 +510,8 @@ func TestPipelineFileDoesNotExistError(t *testing.T) { }, } + bundletest.SetLocation(b, ".", filepath.Join(dir, "fake.yml")) + err := bundle.Apply(context.Background(), b, mutator.TranslatePaths()) assert.EqualError(t, err, "file ./doesnt_exist.py not found") } @@ -544,9 +529,6 @@ func TestJobSparkPythonTaskWithNotebookSourceError(t *testing.T) { Resources: config.Resources{ Jobs: map[string]*resources.Job{ "job": { - Paths: paths.Paths{ - ConfigFilePath: filepath.Join(dir, "resource.yml"), - }, JobSettings: &jobs.JobSettings{ Tasks: []jobs.Task{ { @@ -562,6 +544,8 @@ func TestJobSparkPythonTaskWithNotebookSourceError(t *testing.T) { }, } + bundletest.SetLocation(b, ".", filepath.Join(dir, "resource.yml")) + err := bundle.Apply(context.Background(), b, mutator.TranslatePaths()) assert.ErrorContains(t, err, `expected a file for "tasks.spark_python_task.python_file" but got a notebook`) } @@ -579,9 +563,6 @@ func TestJobNotebookTaskWithFileSourceError(t *testing.T) { Resources: config.Resources{ Jobs: map[string]*resources.Job{ "job": { - Paths: paths.Paths{ - ConfigFilePath: filepath.Join(dir, "resource.yml"), - }, JobSettings: &jobs.JobSettings{ Tasks: []jobs.Task{ { @@ -597,6 +578,8 @@ func TestJobNotebookTaskWithFileSourceError(t *testing.T) { }, } + bundletest.SetLocation(b, ".", filepath.Join(dir, "resource.yml")) + err := bundle.Apply(context.Background(), b, mutator.TranslatePaths()) assert.ErrorContains(t, err, `expected a notebook for "tasks.notebook_task.notebook_path" but got a file`) } @@ -614,9 +597,6 @@ func TestPipelineNotebookLibraryWithFileSourceError(t *testing.T) { Resources: config.Resources{ Pipelines: map[string]*resources.Pipeline{ "pipeline": { - Paths: paths.Paths{ - ConfigFilePath: filepath.Join(dir, "resource.yml"), - }, PipelineSpec: &pipelines.PipelineSpec{ Libraries: []pipelines.PipelineLibrary{ { @@ -632,6 +612,8 @@ func TestPipelineNotebookLibraryWithFileSourceError(t *testing.T) { }, } + bundletest.SetLocation(b, ".", filepath.Join(dir, "resource.yml")) + err := bundle.Apply(context.Background(), b, mutator.TranslatePaths()) assert.ErrorContains(t, err, `expected a notebook for "libraries.notebook.path" but got a file`) } @@ -649,9 +631,6 @@ func TestPipelineFileLibraryWithNotebookSourceError(t *testing.T) { Resources: config.Resources{ Pipelines: map[string]*resources.Pipeline{ "pipeline": { - Paths: paths.Paths{ - ConfigFilePath: filepath.Join(dir, "resource.yml"), - }, PipelineSpec: &pipelines.PipelineSpec{ Libraries: []pipelines.PipelineLibrary{ { @@ -667,6 +646,8 @@ func TestPipelineFileLibraryWithNotebookSourceError(t *testing.T) { }, } + bundletest.SetLocation(b, ".", filepath.Join(dir, "resource.yml")) + err := bundle.Apply(context.Background(), b, mutator.TranslatePaths()) assert.ErrorContains(t, err, `expected a file for "libraries.file.path" but got a notebook`) } diff --git a/bundle/config/paths/paths.go b/bundle/config/paths/paths.go index 2c9ecb8c0d..68c32a48c0 100644 --- a/bundle/config/paths/paths.go +++ b/bundle/config/paths/paths.go @@ -3,12 +3,25 @@ package paths import ( "fmt" "path/filepath" + + "github.com/databricks/cli/libs/dyn" ) type Paths struct { // Absolute path on the local file system to the configuration file that holds // the definition of this resource. ConfigFilePath string `json:"-" bundle:"readonly"` + + // DynamicValue stores the [dyn.Value] of the containing struct. + // This assumes that this struct is always embedded. + DynamicValue dyn.Value `json:"-"` +} + +func (p *Paths) ConfigureConfigFilePath() { + if !p.DynamicValue.IsValid() { + panic("DynamicValue not set") + } + p.ConfigFilePath = p.DynamicValue.Location().File } func (p *Paths) ConfigFileDirectory() (string, error) { diff --git a/bundle/config/resources.go b/bundle/config/resources.go index d0b64d1a36..457360a0cb 100644 --- a/bundle/config/resources.go +++ b/bundle/config/resources.go @@ -126,51 +126,30 @@ func (r *Resources) VerifyUniqueResourceIdentifiers() (*UniqueResourceIdTracker, return tracker, nil } -// SetConfigFilePath sets the specified path for all resources contained in this instance. +// ConfigureConfigFilePath sets the specified path for all resources contained in this instance. // This property is used to correctly resolve paths relative to the path // of the configuration file they were defined in. -func (r *Resources) SetConfigFilePath(path string) { +func (r *Resources) ConfigureConfigFilePath() { for _, e := range r.Jobs { - e.ConfigFilePath = path + e.ConfigureConfigFilePath() } for _, e := range r.Pipelines { - e.ConfigFilePath = path + e.ConfigureConfigFilePath() } for _, e := range r.Models { - e.ConfigFilePath = path + e.ConfigureConfigFilePath() } for _, e := range r.Experiments { - e.ConfigFilePath = path + e.ConfigureConfigFilePath() } for _, e := range r.ModelServingEndpoints { - e.ConfigFilePath = path + e.ConfigureConfigFilePath() } for _, e := range r.RegisteredModels { - e.ConfigFilePath = path + e.ConfigureConfigFilePath() } } -// Merge iterates over all resources and merges chunks of the -// resource configuration that can be merged. For example, for -// jobs, this merges job cluster definitions and tasks that -// use the same `job_cluster_key`, or `task_key`, respectively. -func (r *Resources) Merge() error { - for _, job := range r.Jobs { - if err := job.MergeJobClusters(); err != nil { - return err - } - if err := job.MergeTasks(); err != nil { - return err - } - } - for _, pipeline := range r.Pipelines { - if err := pipeline.MergeClusters(); err != nil { - return err - } - } - return nil -} - type ConfigResource interface { Exists(ctx context.Context, w *databricks.WorkspaceClient, id string) (bool, error) TerraformResourceName() string diff --git a/bundle/config/resources/job.go b/bundle/config/resources/job.go index da85f94dcc..45e9662d9c 100644 --- a/bundle/config/resources/job.go +++ b/bundle/config/resources/job.go @@ -9,7 +9,6 @@ import ( "github.com/databricks/databricks-sdk-go" "github.com/databricks/databricks-sdk-go/marshal" "github.com/databricks/databricks-sdk-go/service/jobs" - "github.com/imdario/mergo" ) type Job struct { @@ -30,72 +29,6 @@ func (s Job) MarshalJSON() ([]byte, error) { return marshal.Marshal(s) } -// MergeJobClusters merges job clusters with the same key. -// The job clusters field is a slice, and as such, overrides are appended to it. -// We can identify a job cluster by its key, however, so we can use this key -// to figure out which definitions are actually overrides and merge them. -func (j *Job) MergeJobClusters() error { - keys := make(map[string]*jobs.JobCluster) - output := make([]jobs.JobCluster, 0, len(j.JobClusters)) - - // Target overrides are always appended, so we can iterate in natural order to - // first find the base definition, and merge instances we encounter later. - for i := range j.JobClusters { - key := j.JobClusters[i].JobClusterKey - - // Register job cluster with key if not yet seen before. - ref, ok := keys[key] - if !ok { - output = append(output, j.JobClusters[i]) - keys[key] = &output[len(output)-1] - continue - } - - // Merge this instance into the reference. - err := mergo.Merge(ref, &j.JobClusters[i], mergo.WithOverride, mergo.WithAppendSlice) - if err != nil { - return err - } - } - - // Overwrite resulting slice. - j.JobClusters = output - return nil -} - -// MergeTasks merges tasks with the same key. -// The tasks field is a slice, and as such, overrides are appended to it. -// We can identify a task by its task key, however, so we can use this key -// to figure out which definitions are actually overrides and merge them. -func (j *Job) MergeTasks() error { - keys := make(map[string]*jobs.Task) - tasks := make([]jobs.Task, 0, len(j.Tasks)) - - // Target overrides are always appended, so we can iterate in natural order to - // first find the base definition, and merge instances we encounter later. - for i := range j.Tasks { - key := j.Tasks[i].TaskKey - - // Register the task with key if not yet seen before. - ref, ok := keys[key] - if !ok { - tasks = append(tasks, j.Tasks[i]) - keys[key] = &tasks[len(tasks)-1] - continue - } - - // Merge this instance into the reference. - err := mergo.Merge(ref, &j.Tasks[i], mergo.WithOverride, mergo.WithAppendSlice) - if err != nil { - return err - } - } - - // Overwrite resulting slice. - j.Tasks = tasks - return nil -} - func (j *Job) Exists(ctx context.Context, w *databricks.WorkspaceClient, id string) (bool, error) { jobId, err := strconv.Atoi(id) if err != nil { diff --git a/bundle/config/resources/job_test.go b/bundle/config/resources/job_test.go deleted file mode 100644 index 24b82fabbe..0000000000 --- a/bundle/config/resources/job_test.go +++ /dev/null @@ -1,116 +0,0 @@ -package resources - -import ( - "testing" - - "github.com/databricks/databricks-sdk-go/service/compute" - "github.com/databricks/databricks-sdk-go/service/jobs" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -func TestJobMergeJobClusters(t *testing.T) { - j := &Job{ - JobSettings: &jobs.JobSettings{ - JobClusters: []jobs.JobCluster{ - { - JobClusterKey: "foo", - NewCluster: &compute.ClusterSpec{ - SparkVersion: "13.3.x-scala2.12", - NodeTypeId: "i3.xlarge", - NumWorkers: 2, - }, - }, - { - JobClusterKey: "bar", - NewCluster: &compute.ClusterSpec{ - SparkVersion: "10.4.x-scala2.12", - }, - }, - { - JobClusterKey: "foo", - NewCluster: &compute.ClusterSpec{ - NodeTypeId: "i3.2xlarge", - NumWorkers: 4, - }, - }, - }, - }, - } - - err := j.MergeJobClusters() - require.NoError(t, err) - - assert.Len(t, j.JobClusters, 2) - assert.Equal(t, "foo", j.JobClusters[0].JobClusterKey) - assert.Equal(t, "bar", j.JobClusters[1].JobClusterKey) - - // This job cluster was merged with a subsequent one. - jc0 := j.JobClusters[0].NewCluster - assert.Equal(t, "13.3.x-scala2.12", jc0.SparkVersion) - assert.Equal(t, "i3.2xlarge", jc0.NodeTypeId) - assert.Equal(t, 4, jc0.NumWorkers) - - // This job cluster was left untouched. - jc1 := j.JobClusters[1].NewCluster - assert.Equal(t, "10.4.x-scala2.12", jc1.SparkVersion) -} - -func TestJobMergeTasks(t *testing.T) { - j := &Job{ - JobSettings: &jobs.JobSettings{ - Tasks: []jobs.Task{ - { - TaskKey: "foo", - NewCluster: &compute.ClusterSpec{ - SparkVersion: "13.3.x-scala2.12", - NodeTypeId: "i3.xlarge", - NumWorkers: 2, - }, - Libraries: []compute.Library{ - {Whl: "package1"}, - }, - }, - { - TaskKey: "bar", - NewCluster: &compute.ClusterSpec{ - SparkVersion: "10.4.x-scala2.12", - }, - }, - { - TaskKey: "foo", - NewCluster: &compute.ClusterSpec{ - NodeTypeId: "i3.2xlarge", - NumWorkers: 4, - }, - Libraries: []compute.Library{ - {Pypi: &compute.PythonPyPiLibrary{ - Package: "package2", - }}, - }, - }, - }, - }, - } - - err := j.MergeTasks() - require.NoError(t, err) - - assert.Len(t, j.Tasks, 2) - assert.Equal(t, "foo", j.Tasks[0].TaskKey) - assert.Equal(t, "bar", j.Tasks[1].TaskKey) - - // This task was merged with a subsequent one. - task0 := j.Tasks[0] - cluster := task0.NewCluster - assert.Equal(t, "13.3.x-scala2.12", cluster.SparkVersion) - assert.Equal(t, "i3.2xlarge", cluster.NodeTypeId) - assert.Equal(t, 4, cluster.NumWorkers) - assert.Len(t, task0.Libraries, 2) - assert.Equal(t, task0.Libraries[0].Whl, "package1") - assert.Equal(t, task0.Libraries[1].Pypi.Package, "package2") - - // This task was left untouched. - task1 := j.Tasks[1].NewCluster - assert.Equal(t, "10.4.x-scala2.12", task1.SparkVersion) -} diff --git a/bundle/config/resources/pipeline.go b/bundle/config/resources/pipeline.go index 97aeef156d..2f9ff8d0db 100644 --- a/bundle/config/resources/pipeline.go +++ b/bundle/config/resources/pipeline.go @@ -2,14 +2,12 @@ package resources import ( "context" - "strings" "github.com/databricks/cli/bundle/config/paths" "github.com/databricks/cli/libs/log" "github.com/databricks/databricks-sdk-go" "github.com/databricks/databricks-sdk-go/marshal" "github.com/databricks/databricks-sdk-go/service/pipelines" - "github.com/imdario/mergo" ) type Pipeline struct { @@ -30,53 +28,6 @@ func (s Pipeline) MarshalJSON() ([]byte, error) { return marshal.Marshal(s) } -// MergeClusters merges cluster definitions with same label. -// The clusters field is a slice, and as such, overrides are appended to it. -// We can identify a cluster by its label, however, so we can use this label -// to figure out which definitions are actually overrides and merge them. -// -// Note: the cluster label is optional and defaults to 'default'. -// We therefore ALSO merge all clusters without a label. -func (p *Pipeline) MergeClusters() error { - clusters := make(map[string]*pipelines.PipelineCluster) - output := make([]pipelines.PipelineCluster, 0, len(p.Clusters)) - - // Normalize cluster labels. - // If empty, this defaults to "default". - // To make matching case insensitive, labels are lowercased. - for i := range p.Clusters { - label := p.Clusters[i].Label - if label == "" { - label = "default" - } - p.Clusters[i].Label = strings.ToLower(label) - } - - // Target overrides are always appended, so we can iterate in natural order to - // first find the base definition, and merge instances we encounter later. - for i := range p.Clusters { - label := p.Clusters[i].Label - - // Register pipeline cluster with label if not yet seen before. - ref, ok := clusters[label] - if !ok { - output = append(output, p.Clusters[i]) - clusters[label] = &output[len(output)-1] - continue - } - - // Merge this instance into the reference. - err := mergo.Merge(ref, &p.Clusters[i], mergo.WithOverride, mergo.WithAppendSlice) - if err != nil { - return err - } - } - - // Overwrite resulting slice. - p.Clusters = output - return nil -} - func (p *Pipeline) Exists(ctx context.Context, w *databricks.WorkspaceClient, id string) (bool, error) { _, err := w.Pipelines.Get(ctx, pipelines.GetPipelineRequest{ PipelineId: id, diff --git a/bundle/config/resources/pipeline_test.go b/bundle/config/resources/pipeline_test.go deleted file mode 100644 index 316e3d1459..0000000000 --- a/bundle/config/resources/pipeline_test.go +++ /dev/null @@ -1,76 +0,0 @@ -package resources - -import ( - "strings" - "testing" - - "github.com/databricks/databricks-sdk-go/service/pipelines" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -func TestPipelineMergeClusters(t *testing.T) { - p := &Pipeline{ - PipelineSpec: &pipelines.PipelineSpec{ - Clusters: []pipelines.PipelineCluster{ - { - NodeTypeId: "i3.xlarge", - NumWorkers: 2, - PolicyId: "1234", - }, - { - Label: "maintenance", - NodeTypeId: "i3.2xlarge", - }, - { - NodeTypeId: "i3.2xlarge", - NumWorkers: 4, - }, - }, - }, - } - - err := p.MergeClusters() - require.NoError(t, err) - - assert.Len(t, p.Clusters, 2) - assert.Equal(t, "default", p.Clusters[0].Label) - assert.Equal(t, "maintenance", p.Clusters[1].Label) - - // The default cluster was merged with a subsequent one. - pc0 := p.Clusters[0] - assert.Equal(t, "i3.2xlarge", pc0.NodeTypeId) - assert.Equal(t, 4, pc0.NumWorkers) - assert.Equal(t, "1234", pc0.PolicyId) - - // The maintenance cluster was left untouched. - pc1 := p.Clusters[1] - assert.Equal(t, "i3.2xlarge", pc1.NodeTypeId) -} - -func TestPipelineMergeClustersCaseInsensitive(t *testing.T) { - p := &Pipeline{ - PipelineSpec: &pipelines.PipelineSpec{ - Clusters: []pipelines.PipelineCluster{ - { - Label: "default", - NumWorkers: 2, - }, - { - Label: "DEFAULT", - NumWorkers: 4, - }, - }, - }, - } - - err := p.MergeClusters() - require.NoError(t, err) - - assert.Len(t, p.Clusters, 1) - - // The default cluster was merged with a subsequent one. - pc0 := p.Clusters[0] - assert.Equal(t, "default", strings.ToLower(pc0.Label)) - assert.Equal(t, 4, pc0.NumWorkers) -} diff --git a/bundle/config/root.go b/bundle/config/root.go index 94cc0b177b..c8b6c59998 100644 --- a/bundle/config/root.go +++ b/bundle/config/root.go @@ -1,6 +1,8 @@ package config import ( + "bytes" + "context" "fmt" "os" "path/filepath" @@ -8,12 +10,20 @@ import ( "github.com/databricks/cli/bundle/config/resources" "github.com/databricks/cli/bundle/config/variable" + "github.com/databricks/cli/libs/diag" + "github.com/databricks/cli/libs/dyn" + "github.com/databricks/cli/libs/dyn/convert" + "github.com/databricks/cli/libs/dyn/merge" + "github.com/databricks/cli/libs/dyn/yamlloader" + "github.com/databricks/cli/libs/log" "github.com/databricks/databricks-sdk-go/service/jobs" - "github.com/ghodss/yaml" - "github.com/imdario/mergo" ) type Root struct { + value dyn.Value + diags diag.Diagnostics + depth int + // Path contains the directory path to the root of the bundle. // It is set when loading `databricks.yml`. Path string `json:"-" bundle:"readonly"` @@ -70,49 +80,170 @@ func Load(path string) (*Root, error) { return nil, err } - var r Root - err = yaml.Unmarshal(raw, &r) + r := Root{ + Path: filepath.Dir(path), + } + + // Load configuration tree from YAML. + v, err := yamlloader.LoadYAML(path, bytes.NewBuffer(raw)) if err != nil { return nil, fmt.Errorf("failed to load %s: %w", path, err) } - if r.Environments != nil && r.Targets != nil { - return nil, fmt.Errorf("both 'environments' and 'targets' are specified, only 'targets' should be used: %s", path) + // Rewrite configuration tree where necessary. + v, err = rewriteShorthands(v) + if err != nil { + return nil, fmt.Errorf("failed to rewrite %s: %w", path, err) } - if r.Environments != nil { - //TODO: add a command line notice that this is a deprecated option. - r.Targets = r.Environments - } + // Normalize dynamic configuration tree according to configuration type. + v, diags := convert.Normalize(r, v) - r.Path = filepath.Dir(path) - r.SetConfigFilePath(path) + // Keep track of diagnostics (warnings and errors in the schema). + // We delay acting on diagnostics until we have loaded all + // configuration files and merged them together. + r.diags = diags + + // Convert normalized configuration tree to typed configuration. + err = r.updateWithDynamicValue(v) + if err != nil { + return nil, fmt.Errorf("failed to load %s: %w", path, err) + } _, err = r.Resources.VerifyUniqueResourceIdentifiers() return &r, err } -// SetConfigFilePath configures the path that its configuration -// 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) +func (r *Root) initializeDynamicValue() error { + // Many test cases initialize a config as a Go struct literal. + // The value will be invalid and we need to populate it from the typed configuration. + if r.value.IsValid() { + return nil } - if r.Targets != nil { - for _, env := range r.Targets { - if env == nil { - continue - } - if env.Resources != nil { - env.Resources.SetConfigFilePath(path) - } - if env.Artifacts != nil { - env.Artifacts.SetConfigFilePath(path) - } + nv, err := convert.FromTyped(r, dyn.NilValue) + if err != nil { + return err + } + + r.value = nv + return nil +} + +func (r *Root) updateWithDynamicValue(nv dyn.Value) error { + // Hack: restore state; it may be cleared by [ToTyped] if + // the configuration equals nil (happens in tests). + diags := r.diags + depth := r.depth + path := r.Path + + defer func() { + r.diags = diags + r.depth = depth + r.Path = path + }() + + // Convert normalized configuration tree to typed configuration. + err := convert.ToTyped(r, nv) + if err != nil { + return err + } + + // Assign the normalized configuration tree. + r.value = nv + + // Assign config file paths after converting to typed configuration. + r.ConfigureConfigFilePath() + return nil +} + +func (r *Root) Mutate(fn func(dyn.Value) (dyn.Value, error)) error { + err := r.initializeDynamicValue() + if err != nil { + return err + } + nv, err := fn(r.value) + if err != nil { + return err + } + err = r.updateWithDynamicValue(nv) + if err != nil { + return err + } + return nil +} + +func (r *Root) MarkMutatorEntry(ctx context.Context) error { + err := r.initializeDynamicValue() + if err != nil { + return err + } + + r.depth++ + + // If we are entering a mutator at depth 1, we need to convert + // the dynamic configuration tree to typed configuration. + if r.depth == 1 { + // Always run ToTyped upon entering a mutator. + // Convert normalized configuration tree to typed configuration. + err := r.updateWithDynamicValue(r.value) + if err != nil { + log.Warnf(ctx, "unable to convert dynamic configuration to typed configuration: %v", err) + return err + } + + } else { + nv, err := convert.FromTyped(r, r.value) + if err != nil { + log.Warnf(ctx, "unable to convert typed configuration to dynamic configuration: %v", err) + return err + } + + // Re-run ToTyped to ensure that no state is piggybacked + err = r.updateWithDynamicValue(nv) + if err != nil { + log.Warnf(ctx, "unable to convert dynamic configuration to typed configuration: %v", err) + return err } } + + return nil +} + +func (r *Root) MarkMutatorExit(ctx context.Context) error { + r.depth-- + + // If we are exiting a mutator at depth 0, we need to convert + // the typed configuration to a dynamic configuration tree. + if r.depth == 0 { + nv, err := convert.FromTyped(r, r.value) + if err != nil { + log.Warnf(ctx, "unable to convert typed configuration to dynamic configuration: %v", err) + return err + } + + // Re-run ToTyped to ensure that no state is piggybacked + err = r.updateWithDynamicValue(nv) + if err != nil { + log.Warnf(ctx, "unable to convert dynamic configuration to typed configuration: %v", err) + return err + } + } + + return nil +} + +func (r *Root) Diagnostics() diag.Diagnostics { + return r.diags +} + +// SetConfigFilePath configures the path that its configuration +// was loaded from in configuration leafs that require it. +func (r *Root) ConfigureConfigFilePath() { + r.Resources.ConfigureConfigFilePath() + if r.Artifacts != nil { + r.Artifacts.ConfigureConfigFilePath() + } } // Initializes variables using values passed from the command line flag @@ -139,125 +270,188 @@ func (r *Root) InitializeVariables(vars []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 = "" + // Merge diagnostics. + r.diags = append(r.diags, other.diags...) // 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 } - // TODO: define and test semantics for merging. - return mergo.Merge(r, other, mergo.WithOverride) + // Merge dynamic configuration values. + return r.Mutate(func(root dyn.Value) (dyn.Value, error) { + return merge.Merge(root, other.value) + }) } -func (r *Root) MergeTargetOverrides(target *Target) error { +func mergeField(rv, ov dyn.Value, name string) (dyn.Value, error) { + path := dyn.NewPath(dyn.Key(name)) + reference, _ := dyn.GetByPath(rv, path) + override, _ := dyn.GetByPath(ov, path) + + // Merge the override into the reference. + var out dyn.Value var err error + if reference.IsValid() && override.IsValid() { + out, err = merge.Merge(reference, override) + if err != nil { + return dyn.InvalidValue, err + } + } else if reference.IsValid() { + out = reference + } else if override.IsValid() { + out = override + } else { + return rv, nil + } - // Target may be nil if it's empty. - if target == nil { - return nil + return dyn.SetByPath(rv, path, out) +} + +func (r *Root) MergeTargetOverrides(name string) error { + root := r.value + target, err := dyn.GetByPath(root, dyn.NewPath(dyn.Key("targets"), dyn.Key(name))) + if err != nil { + return err } - if target.Bundle != nil { - err = mergo.Merge(&r.Bundle, target.Bundle, mergo.WithOverride) - if err != nil { + // Confirm validity of variable overrides. + err = validateVariableOverrides(root, target) + if err != nil { + return err + } + + // Merge fields that can be merged 1:1. + for _, f := range []string{ + "bundle", + "workspace", + "artifacts", + "resources", + "sync", + "permissions", + "variables", + } { + if root, err = mergeField(root, target, f); err != nil { return err } } - if target.Workspace != nil { - err = mergo.Merge(&r.Workspace, target.Workspace, mergo.WithOverride) + // Merge `run_as`. This field must be overwritten if set, not merged. + if v := target.Get("run_as"); v != dyn.NilValue { + root, err = dyn.Set(root, "run_as", v) if err != nil { return err } } - if target.Artifacts != nil { - err = mergo.Merge(&r.Artifacts, target.Artifacts, mergo.WithOverride, mergo.WithAppendSlice) + // Below, we're setting fields on the bundle key, so make sure it exists. + if root.Get("bundle") == dyn.NilValue { + root, err = dyn.Set(root, "bundle", dyn.NewValue(map[string]dyn.Value{}, dyn.Location{})) if err != nil { return err } } - if target.Resources != nil { - err = mergo.Merge(&r.Resources, target.Resources, mergo.WithOverride, mergo.WithAppendSlice) + // Merge `mode`. This field must be overwritten if set, not merged. + if v := target.Get("mode"); v != dyn.NilValue { + root, err = dyn.SetByPath(root, dyn.NewPath(dyn.Key("bundle"), dyn.Key("mode")), v) if err != nil { return err } + } - err = r.Resources.Merge() + // Merge `compute_id`. This field must be overwritten if set, not merged. + if v := target.Get("compute_id"); v != dyn.NilValue { + root, err = dyn.SetByPath(root, dyn.NewPath(dyn.Key("bundle"), dyn.Key("compute_id")), v) if err != nil { return err } } - if target.Variables != nil { - for k, v := range target.Variables { - rootVariable, ok := r.Variables[k] - if !ok { - return fmt.Errorf("variable %s is not defined but is assigned a value", k) - } + // Merge `git`. + if v := target.Get("git"); v != dyn.NilValue { + ref, err := dyn.GetByPath(root, dyn.NewPath(dyn.Key("bundle"), dyn.Key("git"))) + if err != nil { + ref = dyn.NewValue(map[string]dyn.Value{}, dyn.Location{}) + } + + // Merge the override into the reference. + out, err := merge.Merge(ref, v) + if err != nil { + return err + } - if sv, ok := v.(string); ok { - // we allow overrides of the default value for a variable - defaultVal := sv - rootVariable.Default = &defaultVal - } else if vv, ok := v.(map[string]any); ok { - // we also allow overrides of the lookup value for a variable - lookup, ok := vv["lookup"] - if !ok { - return fmt.Errorf("variable %s is incorrectly defined lookup override, no 'lookup' key defined", k) - } - rootVariable.Lookup = variable.LookupFromMap(lookup.(map[string]any)) - } else { - return fmt.Errorf("variable %s is incorrectly defined in target override", k) + // If the branch was overridden, we need to clear the inferred flag. + if branch := v.Get("branch"); branch != dyn.NilValue { + out, err = dyn.SetByPath(out, dyn.NewPath(dyn.Key("inferred")), dyn.NewValue(false, dyn.Location{})) + if err != nil { + return err } } - } - if target.RunAs != nil { - r.RunAs = target.RunAs + // Set the merged value. + root, err = dyn.SetByPath(root, dyn.NewPath(dyn.Key("bundle"), dyn.Key("git")), out) + if err != nil { + return err + } } - if target.Mode != "" { - r.Bundle.Mode = target.Mode - } + // Convert normalized configuration tree to typed configuration. + return r.updateWithDynamicValue(root) +} - if target.ComputeID != "" { - r.Bundle.ComputeID = target.ComputeID +// rewriteShorthands performs lightweight rewriting of the configuration +// tree where we allow users to write a shorthand and must rewrite to the full form. +func rewriteShorthands(v dyn.Value) (dyn.Value, error) { + if v.Kind() != dyn.KindMap { + return v, nil } - git := &r.Bundle.Git - if target.Git.Branch != "" { - git.Branch = target.Git.Branch - git.Inferred = false - } - if target.Git.Commit != "" { - git.Commit = target.Git.Commit - } - if target.Git.OriginURL != "" { - git.OriginURL = target.Git.OriginURL + // For each target, rewrite the variables block. + return dyn.Map(v, "targets", dyn.Foreach(func(target dyn.Value) (dyn.Value, error) { + // Confirm it has a variables block. + if target.Get("variables") == dyn.NilValue { + return target, nil + } + + // For each variable, normalize its contents if it is a single string. + return dyn.Map(target, "variables", dyn.Foreach(func(variable dyn.Value) (dyn.Value, error) { + if variable.Kind() != dyn.KindString { + return variable, nil + } + + // Rewrite the variable to a map with a single key called "default". + // This conforms to the variable type. + return dyn.NewValue(map[string]dyn.Value{ + "default": variable, + }, variable.Location()), nil + })) + })) +} + +// validateVariableOverrides checks that all variables specified +// in the target override are also defined in the root. +func validateVariableOverrides(root, target dyn.Value) (err error) { + var rv map[string]variable.Variable + var tv map[string]variable.Variable + + // Collect variables from the root. + err = convert.ToTyped(&rv, root.Get("variables")) + if err != nil { + return fmt.Errorf("unable to collect variables from root: %w", err) } - if target.Sync != nil { - err = mergo.Merge(&r.Sync, target.Sync, mergo.WithAppendSlice) - if err != nil { - return err - } + // Collect variables from the target. + err = convert.ToTyped(&tv, target.Get("variables")) + if err != nil { + return fmt.Errorf("unable to collect variables from target: %w", err) } - if target.Permissions != nil { - err = mergo.Merge(&r.Permissions, target.Permissions, mergo.WithAppendSlice) - if err != nil { - return err + // Check that all variables in the target exist in the root. + for k := range tv { + if _, ok := rv[k]; !ok { + return fmt.Errorf("variable %s is not defined but is assigned a value", k) } } diff --git a/bundle/config/root_test.go b/bundle/config/root_test.go index 3f37da07a5..3b25fb1f8e 100644 --- a/bundle/config/root_test.go +++ b/bundle/config/root_test.go @@ -30,51 +30,6 @@ func TestRootLoad(t *testing.T) { assert.Equal(t, "basic", root.Bundle.Name) } -func TestRootMergeStruct(t *testing.T) { - root := &Root{ - Path: "path", - Workspace: Workspace{ - Host: "foo", - Profile: "profile", - }, - } - other := &Root{ - Path: "path", - Workspace: Workspace{ - Host: "bar", - }, - } - assert.NoError(t, root.Merge(other)) - assert.Equal(t, "bar", root.Workspace.Host) - assert.Equal(t, "profile", root.Workspace.Profile) -} - -func TestRootMergeMap(t *testing.T) { - root := &Root{ - Path: "path", - Targets: map[string]*Target{ - "development": { - Workspace: &Workspace{ - Host: "foo", - Profile: "profile", - }, - }, - }, - } - other := &Root{ - Path: "path", - Targets: map[string]*Target{ - "development": { - Workspace: &Workspace{ - Host: "bar", - }, - }, - }, - } - assert.NoError(t, root.Merge(other)) - assert.Equal(t, &Workspace{Host: "bar", Profile: "profile"}, root.Targets["development"].Workspace) -} - func TestDuplicateIdOnLoadReturnsError(t *testing.T) { _, err := Load("./testdata/duplicate_resource_names_in_root/databricks.yml") assert.ErrorContains(t, err, "multiple resources named foo (job at ./testdata/duplicate_resource_names_in_root/databricks.yml, pipeline at ./testdata/duplicate_resource_names_in_root/databricks.yml)") @@ -154,8 +109,13 @@ func TestInitializeVariablesUndefinedVariables(t *testing.T) { func TestRootMergeTargetOverridesWithMode(t *testing.T) { root := &Root{ Bundle: Bundle{}, + Targets: map[string]*Target{ + "development": { + Mode: Development, + }, + }, } - env := &Target{Mode: Development} - require.NoError(t, root.MergeTargetOverrides(env)) + root.initializeDynamicValue() + require.NoError(t, root.MergeTargetOverrides("development")) assert.Equal(t, Development, root.Bundle.Mode) } diff --git a/bundle/config/sync.go b/bundle/config/sync.go index 6ba2603c41..0580e4c4ff 100644 --- a/bundle/config/sync.go +++ b/bundle/config/sync.go @@ -1,7 +1,5 @@ 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. @@ -13,19 +11,3 @@ 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 158f256060..acc493574b 100644 --- a/bundle/config/target.go +++ b/bundle/config/target.go @@ -2,6 +2,7 @@ package config import ( "github.com/databricks/cli/bundle/config/resources" + "github.com/databricks/cli/bundle/config/variable" "github.com/databricks/databricks-sdk-go/service/jobs" ) @@ -33,7 +34,7 @@ type Target struct { // Override default values or lookup name for defined variables // Does not permit defining new variables or redefining existing ones // in the scope of an target - Variables map[string]any `json:"variables,omitempty"` + Variables map[string]*variable.Variable `json:"variables,omitempty"` Git Git `json:"git,omitempty"` diff --git a/bundle/config/variable/variable.go b/bundle/config/variable/variable.go index 9057f1cb95..5e700a9b0c 100644 --- a/bundle/config/variable/variable.go +++ b/bundle/config/variable/variable.go @@ -4,8 +4,6 @@ import ( "fmt" ) -const VariableReferencePrefix = "var" - // An input variable for the bundle config type Variable struct { // A default value which then makes the variable optional diff --git a/bundle/deploy/metadata/compute_test.go b/bundle/deploy/metadata/compute_test.go index a1a97aab33..e717ebd53e 100644 --- a/bundle/deploy/metadata/compute_test.go +++ b/bundle/deploy/metadata/compute_test.go @@ -6,8 +6,8 @@ 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/cli/bundle/internal/bundletest" "github.com/databricks/cli/bundle/metadata" "github.com/databricks/databricks-sdk-go/service/jobs" "github.com/stretchr/testify/assert" @@ -36,18 +36,12 @@ func TestComputeMetadataMutator(t *testing.T) { Resources: config.Resources{ Jobs: map[string]*resources.Job{ "my-job-1": { - Paths: paths.Paths{ - ConfigFilePath: "a/b/c", - }, ID: "1111", JobSettings: &jobs.JobSettings{ Name: "My Job One", }, }, "my-job-2": { - Paths: paths.Paths{ - ConfigFilePath: "d/e/f", - }, ID: "2222", JobSettings: &jobs.JobSettings{ Name: "My Job Two", @@ -55,16 +49,16 @@ func TestComputeMetadataMutator(t *testing.T) { }, }, Pipelines: map[string]*resources.Pipeline{ - "my-pipeline": { - Paths: paths.Paths{ - ConfigFilePath: "abc", - }, - }, + "my-pipeline": {}, }, }, }, } + bundletest.SetLocation(b, "resources.jobs.my-job-1", "a/b/c") + bundletest.SetLocation(b, "resources.jobs.my-job-2", "d/e/f") + bundletest.SetLocation(b, "resources.pipelines.my-pipeline", "abc") + expectedMetadata := metadata.Metadata{ Version: metadata.Version, Config: metadata.Config{ diff --git a/bundle/deploy/terraform/interpolate.go b/bundle/deploy/terraform/interpolate.go index 4f00c27ebb..525a38fa88 100644 --- a/bundle/deploy/terraform/interpolate.go +++ b/bundle/deploy/terraform/interpolate.go @@ -1,44 +1,64 @@ package terraform import ( + "context" "fmt" - "strings" "github.com/databricks/cli/bundle" - "github.com/databricks/cli/bundle/config/interpolation" + "github.com/databricks/cli/libs/dyn" + "github.com/databricks/cli/libs/dyn/dynvar" ) -// Rewrite variable references to resources into Terraform compatible format. -func interpolateTerraformResourceIdentifiers(path string, lookup map[string]string) (string, error) { - parts := strings.Split(path, interpolation.Delimiter) - if parts[0] == "resources" { - switch parts[1] { - case "pipelines": - path = strings.Join(append([]string{"databricks_pipeline"}, parts[2:]...), interpolation.Delimiter) - return fmt.Sprintf("${%s}", path), nil - case "jobs": - path = strings.Join(append([]string{"databricks_job"}, parts[2:]...), interpolation.Delimiter) - return fmt.Sprintf("${%s}", path), nil - case "models": - path = strings.Join(append([]string{"databricks_mlflow_model"}, parts[2:]...), interpolation.Delimiter) - return fmt.Sprintf("${%s}", path), nil - case "experiments": - path = strings.Join(append([]string{"databricks_mlflow_experiment"}, parts[2:]...), interpolation.Delimiter) - return fmt.Sprintf("${%s}", path), nil - case "model_serving_endpoints": - path = strings.Join(append([]string{"databricks_model_serving"}, parts[2:]...), interpolation.Delimiter) - return fmt.Sprintf("${%s}", path), nil - case "registered_models": - path = strings.Join(append([]string{"databricks_registered_model"}, parts[2:]...), interpolation.Delimiter) - return fmt.Sprintf("${%s}", path), nil - default: - panic("TODO: " + parts[1]) - } - } - - return interpolation.DefaultLookup(path, lookup) +type interpolateMutator struct { } func Interpolate() bundle.Mutator { - return interpolation.Interpolate(interpolateTerraformResourceIdentifiers) + return &interpolateMutator{} +} + +func (m *interpolateMutator) Name() string { + return "terraform.Interpolate" +} + +func (m *interpolateMutator) Apply(ctx context.Context, b *bundle.Bundle) error { + return b.Config.Mutate(func(root dyn.Value) (dyn.Value, error) { + prefix := dyn.MustPathFromString("resources") + + // Resolve variable references in all values. + return dynvar.Resolve(root, func(path dyn.Path) (dyn.Value, error) { + // Expect paths of the form: + // - resources...... + if !path.HasPrefix(prefix) || len(path) < 4 { + return dyn.InvalidValue, dynvar.ErrSkipResolution + } + + // Rewrite the bundle configuration path: + // + // ${resources.pipelines.my_pipeline.id} + // + // into the Terraform-compatible resource identifier: + // + // ${databricks_pipeline.my_pipeline.id} + // + switch path[1] { + case dyn.Key("pipelines"): + path = dyn.NewPath(dyn.Key("databricks_pipeline")).Append(path[2:]...) + case dyn.Key("jobs"): + path = dyn.NewPath(dyn.Key("databricks_job")).Append(path[2:]...) + case dyn.Key("models"): + path = dyn.NewPath(dyn.Key("databricks_mlflow_model")).Append(path[2:]...) + case dyn.Key("experiments"): + path = dyn.NewPath(dyn.Key("databricks_mlflow_experiment")).Append(path[2:]...) + case dyn.Key("model_serving_endpoints"): + path = dyn.NewPath(dyn.Key("databricks_model_serving")).Append(path[2:]...) + case dyn.Key("registered_models"): + path = dyn.NewPath(dyn.Key("databricks_registered_model")).Append(path[2:]...) + default: + // Trigger "key not found" for unknown resource types. + return dyn.GetByPath(root, path) + } + + return dyn.V(fmt.Sprintf("${%s}", path.String())), nil + }) + }) } diff --git a/bundle/deploy/terraform/interpolate_test.go b/bundle/deploy/terraform/interpolate_test.go new file mode 100644 index 0000000000..be905ad772 --- /dev/null +++ b/bundle/deploy/terraform/interpolate_test.go @@ -0,0 +1,92 @@ +package terraform + +import ( + "context" + "testing" + + "github.com/databricks/cli/bundle" + "github.com/databricks/cli/bundle/config" + "github.com/databricks/cli/bundle/config/resources" + "github.com/databricks/databricks-sdk-go/service/jobs" + "github.com/databricks/databricks-sdk-go/service/ml" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestInterpolate(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Bundle: config.Bundle{ + Name: "example", + }, + Resources: config.Resources{ + Jobs: map[string]*resources.Job{ + "my_job": { + JobSettings: &jobs.JobSettings{ + Tags: map[string]string{ + "other_pipeline": "${resources.pipelines.other_pipeline.id}", + "other_job": "${resources.jobs.other_job.id}", + "other_model": "${resources.models.other_model.id}", + "other_experiment": "${resources.experiments.other_experiment.id}", + "other_model_serving": "${resources.model_serving_endpoints.other_model_serving.id}", + "other_registered_model": "${resources.registered_models.other_registered_model.id}", + }, + Tasks: []jobs.Task{ + { + TaskKey: "my_task", + NotebookTask: &jobs.NotebookTask{ + BaseParameters: map[string]string{ + "model_name": "${resources.models.my_model.name}", + }, + }, + }, + }, + }, + }, + }, + Models: map[string]*resources.MlflowModel{ + "my_model": { + Model: &ml.Model{ + Name: "my_model", + }, + }, + }, + }, + }, + } + + err := bundle.Apply(context.Background(), b, Interpolate()) + require.NoError(t, err) + + j := b.Config.Resources.Jobs["my_job"] + assert.Equal(t, "${databricks_pipeline.other_pipeline.id}", j.Tags["other_pipeline"]) + assert.Equal(t, "${databricks_job.other_job.id}", j.Tags["other_job"]) + assert.Equal(t, "${databricks_mlflow_model.other_model.id}", j.Tags["other_model"]) + assert.Equal(t, "${databricks_mlflow_experiment.other_experiment.id}", j.Tags["other_experiment"]) + assert.Equal(t, "${databricks_model_serving.other_model_serving.id}", j.Tags["other_model_serving"]) + assert.Equal(t, "${databricks_registered_model.other_registered_model.id}", j.Tags["other_registered_model"]) + + m := b.Config.Resources.Models["my_model"] + assert.Equal(t, "my_model", m.Model.Name) +} + +func TestInterpolateUnknownResourceType(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Resources: config.Resources{ + Jobs: map[string]*resources.Job{ + "my_job": { + JobSettings: &jobs.JobSettings{ + Tags: map[string]string{ + "other_unknown": "${resources.unknown.other_unknown.id}", + }, + }, + }, + }, + }, + }, + } + + err := bundle.Apply(context.Background(), b, Interpolate()) + assert.Contains(t, err.Error(), `reference does not exist: ${resources.unknown.other_unknown.id}`) +} diff --git a/bundle/internal/bundletest/location.go b/bundle/internal/bundletest/location.go new file mode 100644 index 0000000000..1fd6f968c2 --- /dev/null +++ b/bundle/internal/bundletest/location.go @@ -0,0 +1,34 @@ +package bundletest + +import ( + "github.com/databricks/cli/bundle" + "github.com/databricks/cli/libs/dyn" +) + +// SetLocation sets the location of all values in the bundle to the given path. +// This is useful for testing where we need to associate configuration +// with the path it is loaded from. +func SetLocation(b *bundle.Bundle, prefix string, filePath string) { + start := dyn.MustPathFromString(prefix) + b.Config.Mutate(func(root dyn.Value) (dyn.Value, error) { + return dyn.Walk(root, func(p dyn.Path, v dyn.Value) (dyn.Value, error) { + // If the path has the given prefix, set the location. + if p.HasPrefix(start) { + return v.WithLocation(dyn.Location{ + File: filePath, + }), nil + } + + // The path is not nested under the given prefix. + // If the path is a prefix of the prefix, keep traversing and return the node verbatim. + if start.HasPrefix(p) { + return v, nil + } + + // Return verbatim, but skip traversal. + return v, dyn.ErrSkip + }) + }) + + b.Config.ConfigureConfigFilePath() +} diff --git a/bundle/mutator.go b/bundle/mutator.go index e559d2375f..bd1615fd76 100644 --- a/bundle/mutator.go +++ b/bundle/mutator.go @@ -20,7 +20,21 @@ func Apply(ctx context.Context, b *Bundle, m Mutator) error { ctx = log.NewContext(ctx, log.GetLogger(ctx).With("mutator", m.Name())) log.Debugf(ctx, "Apply") - err := m.Apply(ctx, b) + + err := b.Config.MarkMutatorEntry(ctx) + if err != nil { + log.Errorf(ctx, "entry error: %s", err) + return err + } + + defer func() { + err := b.Config.MarkMutatorExit(ctx) + if err != nil { + log.Errorf(ctx, "exit error: %s", err) + } + }() + + err = m.Apply(ctx, b) if err != nil { log.Errorf(ctx, "Error: %s", err) return err @@ -28,3 +42,20 @@ func Apply(ctx context.Context, b *Bundle, m Mutator) error { return nil } + +type funcMutator struct { + fn func(context.Context, *Bundle) error +} + +func (m funcMutator) Name() string { + return "" +} + +func (m funcMutator) Apply(ctx context.Context, b *Bundle) error { + return m.fn(ctx, b) +} + +// ApplyFunc applies an inline-specified function mutator. +func ApplyFunc(ctx context.Context, b *Bundle, fn func(context.Context, *Bundle) error) error { + return Apply(ctx, b, funcMutator{fn}) +} diff --git a/bundle/phases/build.go b/bundle/phases/build.go index 760967fca2..362d23be14 100644 --- a/bundle/phases/build.go +++ b/bundle/phases/build.go @@ -4,7 +4,7 @@ import ( "github.com/databricks/cli/bundle" "github.com/databricks/cli/bundle/artifacts" "github.com/databricks/cli/bundle/config" - "github.com/databricks/cli/bundle/config/interpolation" + "github.com/databricks/cli/bundle/config/mutator" "github.com/databricks/cli/bundle/scripts" ) @@ -18,8 +18,8 @@ func Build() bundle.Mutator { artifacts.InferMissingProperties(), artifacts.BuildAll(), scripts.Execute(config.ScriptPostBuild), - interpolation.Interpolate( - interpolation.IncludeLookupsInPath("artifacts"), + mutator.ResolveVariableReferences( + "artifacts", ), }, ) diff --git a/bundle/phases/initialize.go b/bundle/phases/initialize.go index e0558d9378..2c401c6b2a 100644 --- a/bundle/phases/initialize.go +++ b/bundle/phases/initialize.go @@ -3,9 +3,7 @@ package phases import ( "github.com/databricks/cli/bundle" "github.com/databricks/cli/bundle/config" - "github.com/databricks/cli/bundle/config/interpolation" "github.com/databricks/cli/bundle/config/mutator" - "github.com/databricks/cli/bundle/config/variable" "github.com/databricks/cli/bundle/deploy/metadata" "github.com/databricks/cli/bundle/deploy/terraform" "github.com/databricks/cli/bundle/permissions" @@ -20,6 +18,10 @@ func Initialize() bundle.Mutator { return newPhase( "initialize", []bundle.Mutator{ + mutator.RewriteSyncPaths(), + mutator.MergeJobClusters(), + mutator.MergeJobTasks(), + mutator.MergePipelineClusters(), mutator.InitializeWorkspaceClient(), mutator.PopulateCurrentUser(), mutator.DefineDefaultWorkspaceRoot(), @@ -27,10 +29,10 @@ func Initialize() bundle.Mutator { mutator.DefineDefaultWorkspacePaths(), mutator.SetVariables(), mutator.ResolveResourceReferences(), - interpolation.Interpolate( - interpolation.IncludeLookupsInPath("bundle"), - interpolation.IncludeLookupsInPath("workspace"), - interpolation.IncludeLookupsInPath(variable.VariableReferencePrefix), + mutator.ResolveVariableReferences( + "bundle", + "workspace", + "variables", ), mutator.SetRunAs(), mutator.OverrideCompute(), diff --git a/bundle/tests/bundle/pipeline_glob_paths_test.go b/bundle/tests/bundle/pipeline_glob_paths_test.go index 539ffc9d31..8f2b62a6b6 100644 --- a/bundle/tests/bundle/pipeline_glob_paths_test.go +++ b/bundle/tests/bundle/pipeline_glob_paths_test.go @@ -5,30 +5,34 @@ import ( "testing" "github.com/databricks/cli/bundle" - "github.com/databricks/cli/bundle/config" "github.com/databricks/cli/bundle/config/mutator" "github.com/databricks/cli/bundle/phases" - "github.com/databricks/cli/cmd/root" + "github.com/databricks/databricks-sdk-go/config" + "github.com/databricks/databricks-sdk-go/experimental/mocks" "github.com/databricks/databricks-sdk-go/service/iam" + "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" ) func TestExpandPipelineGlobPathsWithNonExistent(t *testing.T) { ctx := context.Background() - ctx = root.SetWorkspaceClient(ctx, nil) - b, err := bundle.Load(ctx, "./pipeline_glob_paths") require.NoError(t, err) - err = bundle.Apply(ctx, b, bundle.Seq(mutator.DefaultMutators()...)) + err = bundle.Apply(ctx, b, bundle.Seq(mutator.DefaultMutatorsForTarget("default")...)) require.NoError(t, err) - b.Config.Bundle.Target = "default" - - b.Config.Workspace.CurrentUser = &config.User{User: &iam.User{UserName: "user@domain.com"}} - b.WorkspaceClient() - m := phases.Initialize() - err = bundle.Apply(ctx, b, m) + // Configure mock workspace client + m := mocks.NewMockWorkspaceClient(t) + m.WorkspaceClient.Config = &config.Config{ + Host: "https://mock.databricks.workspace.com", + } + m.GetMockCurrentUserAPI().EXPECT().Me(mock.Anything).Return(&iam.User{ + UserName: "user@domain.com", + }, nil) + b.SetWorkpaceClient(m.WorkspaceClient) + + err = bundle.Apply(ctx, b, phases.Initialize()) require.Error(t, err) require.ErrorContains(t, err, "notebook ./non-existent not found") diff --git a/bundle/tests/environment_overrides/resources/databricks.yml b/bundle/tests/environment_overrides/resources/databricks.yml index df261ba034..137f8d9df5 100644 --- a/bundle/tests/environment_overrides/resources/databricks.yml +++ b/bundle/tests/environment_overrides/resources/databricks.yml @@ -28,8 +28,6 @@ environments: pipelines: boolean1: - # Note: setting a property to a zero value (in Go) does not have effect. - # See the corresponding test for details. photon: false boolean2: diff --git a/bundle/tests/environment_overrides_test.go b/bundle/tests/environment_overrides_test.go index 91dc2c8114..4a11150486 100644 --- a/bundle/tests/environment_overrides_test.go +++ b/bundle/tests/environment_overrides_test.go @@ -29,10 +29,7 @@ func TestEnvironmentOverridesResourcesStaging(t *testing.T) { b := loadTarget(t, "./environment_overrides/resources", "staging") assert.Equal(t, "staging job", b.Config.Resources.Jobs["job1"].Name) - // Overrides are only applied if they are not zero-valued. - // This means that in its current form, we cannot override a true value with a false value. - // Note: this is not desirable and will be addressed by representing our configuration - // in a different structure (e.g. with cty), instead of Go structs. - assert.Equal(t, true, b.Config.Resources.Pipelines["boolean1"].Photon) + // Override values are applied in the staging environment. + assert.Equal(t, false, b.Config.Resources.Pipelines["boolean1"].Photon) assert.Equal(t, true, b.Config.Resources.Pipelines["boolean2"].Photon) } diff --git a/bundle/tests/interpolation_test.go b/bundle/tests/interpolation_test.go index 837891a072..a9659d33f8 100644 --- a/bundle/tests/interpolation_test.go +++ b/bundle/tests/interpolation_test.go @@ -5,16 +5,16 @@ import ( "testing" "github.com/databricks/cli/bundle" - "github.com/databricks/cli/bundle/config/interpolation" + "github.com/databricks/cli/bundle/config/mutator" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) func TestInterpolation(t *testing.T) { b := load(t, "./interpolation") - err := bundle.Apply(context.Background(), b, interpolation.Interpolate( - interpolation.IncludeLookupsInPath("bundle"), - interpolation.IncludeLookupsInPath("workspace"), + err := bundle.Apply(context.Background(), b, mutator.ResolveVariableReferences( + "bundle", + "workspace", )) require.NoError(t, err) assert.Equal(t, "foo bar", b.Config.Bundle.Name) @@ -23,9 +23,9 @@ func TestInterpolation(t *testing.T) { func TestInterpolationWithTarget(t *testing.T) { b := loadTarget(t, "./interpolation_target", "development") - err := bundle.Apply(context.Background(), b, interpolation.Interpolate( - interpolation.IncludeLookupsInPath("bundle"), - interpolation.IncludeLookupsInPath("workspace"), + err := bundle.Apply(context.Background(), b, mutator.ResolveVariableReferences( + "bundle", + "workspace", )) require.NoError(t, err) assert.Equal(t, "foo bar", b.Config.Bundle.Name) diff --git a/bundle/tests/job_with_spark_conf_test.go b/bundle/tests/job_with_spark_conf_test.go index a2c04c5eea..90bdc977d6 100644 --- a/bundle/tests/job_with_spark_conf_test.go +++ b/bundle/tests/job_with_spark_conf_test.go @@ -4,6 +4,7 @@ import ( "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestJobWithSparkConf(t *testing.T) { @@ -14,9 +15,17 @@ func TestJobWithSparkConf(t *testing.T) { assert.Len(t, job.JobClusters, 1) assert.Equal(t, "test_cluster", job.JobClusters[0].JobClusterKey) - // Existing behavior is such that including non-string values - // in the spark_conf map will cause the job to fail to load. - // This is expected to be solved once we switch to the custom YAML loader. - tasks := job.Tasks - assert.Len(t, tasks, 0, "see https://github.com/databricks/cli/issues/992") + // This test exists because of https://github.com/databricks/cli/issues/992. + // It is solved for bundles as of https://github.com/databricks/cli/pull/1098. + require.Len(t, job.JobClusters, 1) + cluster := job.JobClusters[0] + assert.Equal(t, "14.2.x-scala2.12", cluster.NewCluster.SparkVersion) + assert.Equal(t, "i3.xlarge", cluster.NewCluster.NodeTypeId) + assert.Equal(t, 2, cluster.NewCluster.NumWorkers) + assert.Equal(t, map[string]string{ + "spark.string": "string", + "spark.int": "1", + "spark.bool": "true", + "spark.float": "1.2", + }, cluster.NewCluster.SparkConf) } diff --git a/bundle/tests/loader.go b/bundle/tests/loader.go index f23b107649..3a28d822a2 100644 --- a/bundle/tests/loader.go +++ b/bundle/tests/loader.go @@ -19,8 +19,17 @@ func load(t *testing.T, path string) *bundle.Bundle { } func loadTarget(t *testing.T, path, env string) *bundle.Bundle { - b := load(t, path) - err := bundle.Apply(context.Background(), b, mutator.SelectTarget(env)) + ctx := context.Background() + b, err := bundle.Load(ctx, path) + require.NoError(t, err) + err = bundle.Apply(ctx, b, bundle.Seq(mutator.DefaultMutatorsForTarget(env)...)) + require.NoError(t, err) + err = bundle.Apply(ctx, b, bundle.Seq( + mutator.RewriteSyncPaths(), + mutator.MergeJobClusters(), + mutator.MergeJobTasks(), + mutator.MergePipelineClusters(), + )) require.NoError(t, err) return b } diff --git a/bundle/tests/override_sync_test.go b/bundle/tests/override_sync_test.go index a2d3a05f5a..64f28e377e 100644 --- a/bundle/tests/override_sync_test.go +++ b/bundle/tests/override_sync_test.go @@ -1,40 +1,38 @@ package config_tests import ( + "path/filepath" "testing" + "github.com/databricks/cli/bundle" "github.com/stretchr/testify/assert" ) func TestOverrideSyncTarget(t *testing.T) { - b := load(t, "./override_sync") - assert.ElementsMatch(t, []string{"src/*"}, b.Config.Sync.Include) - assert.ElementsMatch(t, []string{}, b.Config.Sync.Exclude) + var b *bundle.Bundle b = loadTarget(t, "./override_sync", "development") - assert.ElementsMatch(t, []string{"src/*", "tests/*"}, b.Config.Sync.Include) - assert.ElementsMatch(t, []string{"dist"}, b.Config.Sync.Exclude) + assert.ElementsMatch(t, []string{filepath.FromSlash("src/*"), filepath.FromSlash("tests/*")}, b.Config.Sync.Include) + assert.ElementsMatch(t, []string{filepath.FromSlash("dist")}, b.Config.Sync.Exclude) b = loadTarget(t, "./override_sync", "staging") - assert.ElementsMatch(t, []string{"src/*", "fixtures/*"}, b.Config.Sync.Include) + assert.ElementsMatch(t, []string{filepath.FromSlash("src/*"), filepath.FromSlash("fixtures/*")}, b.Config.Sync.Include) assert.ElementsMatch(t, []string{}, b.Config.Sync.Exclude) b = loadTarget(t, "./override_sync", "prod") - assert.ElementsMatch(t, []string{"src/*"}, b.Config.Sync.Include) + assert.ElementsMatch(t, []string{filepath.FromSlash("src/*")}, b.Config.Sync.Include) assert.ElementsMatch(t, []string{}, b.Config.Sync.Exclude) } func TestOverrideSyncTargetNoRootSync(t *testing.T) { - b := load(t, "./override_sync_no_root") - assert.ElementsMatch(t, []string{}, b.Config.Sync.Include) - assert.ElementsMatch(t, []string{}, b.Config.Sync.Exclude) + var b *bundle.Bundle b = loadTarget(t, "./override_sync_no_root", "development") - assert.ElementsMatch(t, []string{"tests/*"}, b.Config.Sync.Include) - assert.ElementsMatch(t, []string{"dist"}, b.Config.Sync.Exclude) + assert.ElementsMatch(t, []string{filepath.FromSlash("tests/*")}, b.Config.Sync.Include) + assert.ElementsMatch(t, []string{filepath.FromSlash("dist")}, b.Config.Sync.Exclude) b = loadTarget(t, "./override_sync_no_root", "staging") - assert.ElementsMatch(t, []string{"fixtures/*"}, b.Config.Sync.Include) + assert.ElementsMatch(t, []string{filepath.FromSlash("fixtures/*")}, b.Config.Sync.Include) assert.ElementsMatch(t, []string{}, b.Config.Sync.Exclude) b = loadTarget(t, "./override_sync_no_root", "prod") diff --git a/bundle/tests/relative_path_with_includes_test.go b/bundle/tests/relative_path_with_includes_test.go index 92249c412c..1d1f321d4b 100644 --- a/bundle/tests/relative_path_with_includes_test.go +++ b/bundle/tests/relative_path_with_includes_test.go @@ -11,7 +11,7 @@ import ( ) func TestRelativePathsWithIncludes(t *testing.T) { - b := load(t, "./relative_path_with_includes") + b := loadTarget(t, "./relative_path_with_includes", "default") m := mutator.TranslatePaths() err := bundle.Apply(context.Background(), b, m) @@ -20,8 +20,22 @@ func TestRelativePathsWithIncludes(t *testing.T) { assert.Equal(t, "artifact_a", b.Config.Artifacts["test_a"].Path) assert.Equal(t, filepath.Join("subfolder", "artifact_b"), b.Config.Artifacts["test_b"].Path) - 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.ElementsMatch( + t, + []string{ + filepath.Join("folder_a", "*.*"), + filepath.Join("subfolder", "folder_c", "*.*"), + }, + b.Config.Sync.Include, + ) + assert.ElementsMatch( + t, + []string{ + filepath.Join("folder_b", "*.*"), + filepath.Join("subfolder", "folder_d", "*.*"), + }, + b.Config.Sync.Exclude, + ) 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) diff --git a/bundle/tests/run_as/databricks.yml b/bundle/tests/run_as/databricks.yml index 18ea55736d..1cdc9e44b2 100644 --- a/bundle/tests/run_as/databricks.yml +++ b/bundle/tests/run_as/databricks.yml @@ -13,30 +13,42 @@ targets: resources: pipelines: nyc_taxi_pipeline: + name: "nyc taxi loader" + permissions: - level: CAN_VIEW service_principal_name: my_service_principal - level: CAN_VIEW user_name: my_user_name - name: "nyc taxi loader" + libraries: - notebook: path: ./dlt/nyc_taxi_loader + jobs: job_one: name: Job One + tasks: - - task: + - task_key: "task_one" + notebook_task: notebook_path: "./test.py" + job_two: name: Job Two + tasks: - - task: + - task_key: "task_two" + notebook_task: notebook_path: "./test.py" + job_three: name: Job Three + run_as: service_principal_name: "my_service_principal_for_job" + tasks: - - task: + - task_key: "task_three" + notebook_task: notebook_path: "./test.py" diff --git a/bundle/tests/run_as_test.go b/bundle/tests/run_as_test.go index 44c068165a..98aaf63580 100644 --- a/bundle/tests/run_as_test.go +++ b/bundle/tests/run_as_test.go @@ -13,12 +13,17 @@ import ( func TestRunAsDefault(t *testing.T) { b := load(t, "./run_as") - b.Config.Workspace.CurrentUser = &config.User{ - User: &iam.User{ - UserName: "jane@doe.com", - }, - } + ctx := context.Background() + bundle.ApplyFunc(ctx, b, func(ctx context.Context, b *bundle.Bundle) error { + b.Config.Workspace.CurrentUser = &config.User{ + User: &iam.User{ + UserName: "jane@doe.com", + }, + } + return nil + }) + err := bundle.Apply(ctx, b, mutator.SetRunAs()) assert.NoError(t, err) @@ -39,21 +44,26 @@ func TestRunAsDefault(t *testing.T) { pipelines := b.Config.Resources.Pipelines assert.Len(t, pipelines["nyc_taxi_pipeline"].Permissions, 2) - assert.Equal(t, pipelines["nyc_taxi_pipeline"].Permissions[0].Level, "CAN_VIEW") - assert.Equal(t, pipelines["nyc_taxi_pipeline"].Permissions[0].UserName, "my_user_name") + assert.Equal(t, "CAN_VIEW", pipelines["nyc_taxi_pipeline"].Permissions[0].Level) + assert.Equal(t, "my_user_name", pipelines["nyc_taxi_pipeline"].Permissions[0].UserName) - assert.Equal(t, pipelines["nyc_taxi_pipeline"].Permissions[1].Level, "IS_OWNER") - assert.Equal(t, pipelines["nyc_taxi_pipeline"].Permissions[1].ServicePrincipalName, "my_service_principal") + assert.Equal(t, "IS_OWNER", pipelines["nyc_taxi_pipeline"].Permissions[1].Level) + assert.Equal(t, "my_service_principal", pipelines["nyc_taxi_pipeline"].Permissions[1].ServicePrincipalName) } func TestRunAsDevelopment(t *testing.T) { b := loadTarget(t, "./run_as", "development") - b.Config.Workspace.CurrentUser = &config.User{ - User: &iam.User{ - UserName: "jane@doe.com", - }, - } + ctx := context.Background() + bundle.ApplyFunc(ctx, b, func(ctx context.Context, b *bundle.Bundle) error { + b.Config.Workspace.CurrentUser = &config.User{ + User: &iam.User{ + UserName: "jane@doe.com", + }, + } + return nil + }) + err := bundle.Apply(ctx, b, mutator.SetRunAs()) assert.NoError(t, err) @@ -74,9 +84,9 @@ func TestRunAsDevelopment(t *testing.T) { pipelines := b.Config.Resources.Pipelines assert.Len(t, pipelines["nyc_taxi_pipeline"].Permissions, 2) - assert.Equal(t, pipelines["nyc_taxi_pipeline"].Permissions[0].Level, "CAN_VIEW") - assert.Equal(t, pipelines["nyc_taxi_pipeline"].Permissions[0].ServicePrincipalName, "my_service_principal") + assert.Equal(t, "CAN_VIEW", pipelines["nyc_taxi_pipeline"].Permissions[0].Level) + assert.Equal(t, "my_service_principal", pipelines["nyc_taxi_pipeline"].Permissions[0].ServicePrincipalName) - assert.Equal(t, pipelines["nyc_taxi_pipeline"].Permissions[1].Level, "IS_OWNER") - assert.Equal(t, pipelines["nyc_taxi_pipeline"].Permissions[1].UserName, "my_user_name") + assert.Equal(t, "IS_OWNER", pipelines["nyc_taxi_pipeline"].Permissions[1].Level) + assert.Equal(t, "my_user_name", pipelines["nyc_taxi_pipeline"].Permissions[1].UserName) } diff --git a/bundle/tests/variables_test.go b/bundle/tests/variables_test.go index 91e165b158..05314a8465 100644 --- a/bundle/tests/variables_test.go +++ b/bundle/tests/variables_test.go @@ -5,9 +5,7 @@ import ( "testing" "github.com/databricks/cli/bundle" - "github.com/databricks/cli/bundle/config/interpolation" "github.com/databricks/cli/bundle/config/mutator" - "github.com/databricks/cli/bundle/config/variable" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -17,9 +15,10 @@ func TestVariables(t *testing.T) { b := load(t, "./variables/vanilla") err := bundle.Apply(context.Background(), b, bundle.Seq( mutator.SetVariables(), - interpolation.Interpolate( - interpolation.IncludeLookupsInPath(variable.VariableReferencePrefix), - ))) + mutator.ResolveVariableReferences( + "variables", + ), + )) require.NoError(t, err) assert.Equal(t, "abc def", b.Config.Bundle.Name) } @@ -28,9 +27,10 @@ func TestVariablesLoadingFailsWhenRequiredVariableIsNotSpecified(t *testing.T) { b := load(t, "./variables/vanilla") err := bundle.Apply(context.Background(), b, bundle.Seq( mutator.SetVariables(), - interpolation.Interpolate( - interpolation.IncludeLookupsInPath(variable.VariableReferencePrefix), - ))) + mutator.ResolveVariableReferences( + "variables", + ), + )) assert.ErrorContains(t, err, "no value assigned to required variable b. Assignment can be done through the \"--var\" flag or by setting the BUNDLE_VAR_b environment variable") } @@ -39,9 +39,10 @@ func TestVariablesTargetsBlockOverride(t *testing.T) { err := bundle.Apply(context.Background(), b, bundle.Seq( mutator.SelectTarget("env-with-single-variable-override"), mutator.SetVariables(), - interpolation.Interpolate( - interpolation.IncludeLookupsInPath(variable.VariableReferencePrefix), - ))) + mutator.ResolveVariableReferences( + "variables", + ), + )) require.NoError(t, err) assert.Equal(t, "default-a dev-b", b.Config.Workspace.Profile) } @@ -51,9 +52,10 @@ func TestVariablesTargetsBlockOverrideForMultipleVariables(t *testing.T) { err := bundle.Apply(context.Background(), b, bundle.Seq( mutator.SelectTarget("env-with-two-variable-overrides"), mutator.SetVariables(), - interpolation.Interpolate( - interpolation.IncludeLookupsInPath(variable.VariableReferencePrefix), - ))) + mutator.ResolveVariableReferences( + "variables", + ), + )) require.NoError(t, err) assert.Equal(t, "prod-a prod-b", b.Config.Workspace.Profile) } @@ -64,9 +66,10 @@ func TestVariablesTargetsBlockOverrideWithProcessEnvVars(t *testing.T) { err := bundle.Apply(context.Background(), b, bundle.Seq( mutator.SelectTarget("env-with-two-variable-overrides"), mutator.SetVariables(), - interpolation.Interpolate( - interpolation.IncludeLookupsInPath(variable.VariableReferencePrefix), - ))) + mutator.ResolveVariableReferences( + "variables", + ), + )) require.NoError(t, err) assert.Equal(t, "prod-a env-var-b", b.Config.Workspace.Profile) } @@ -76,9 +79,10 @@ func TestVariablesTargetsBlockOverrideWithMissingVariables(t *testing.T) { err := bundle.Apply(context.Background(), b, bundle.Seq( mutator.SelectTarget("env-missing-a-required-variable-assignment"), mutator.SetVariables(), - interpolation.Interpolate( - interpolation.IncludeLookupsInPath(variable.VariableReferencePrefix), - ))) + mutator.ResolveVariableReferences( + "variables", + ), + )) assert.ErrorContains(t, err, "no value assigned to required variable b. Assignment can be done through the \"--var\" flag or by setting the BUNDLE_VAR_b environment variable") } @@ -87,9 +91,10 @@ func TestVariablesTargetsBlockOverrideWithUndefinedVariables(t *testing.T) { err := bundle.Apply(context.Background(), b, bundle.Seq( mutator.SelectTarget("env-using-an-undefined-variable"), mutator.SetVariables(), - interpolation.Interpolate( - interpolation.IncludeLookupsInPath(variable.VariableReferencePrefix), - ))) + mutator.ResolveVariableReferences( + "variables", + ), + )) assert.ErrorContains(t, err, "variable c is not defined but is assigned a value") } @@ -110,9 +115,7 @@ func TestVariablesWithTargetLookupOverrides(t *testing.T) { err := bundle.Apply(context.Background(), b, bundle.Seq( mutator.SelectTarget("env-overrides-lookup"), mutator.SetVariables(), - interpolation.Interpolate( - interpolation.IncludeLookupsInPath(variable.VariableReferencePrefix), - ))) + )) require.NoError(t, err) assert.Equal(t, "cluster: some-test-cluster", b.Config.Variables["d"].Lookup.String()) assert.Equal(t, "instance-pool: some-test-instance-pool", b.Config.Variables["e"].Lookup.String()) diff --git a/cmd/bundle/deploy.go b/cmd/bundle/deploy.go index c76789c17d..c1f0cdf297 100644 --- a/cmd/bundle/deploy.go +++ b/cmd/bundle/deploy.go @@ -1,6 +1,8 @@ package bundle import ( + "context" + "github.com/databricks/cli/bundle" "github.com/databricks/cli/bundle/phases" "github.com/databricks/cli/cmd/bundle/utils" @@ -24,17 +26,22 @@ func newDeployCommand() *cobra.Command { cmd.Flags().StringVarP(&computeID, "compute-id", "c", "", "Override compute in the deployment with the given compute ID.") cmd.RunE = func(cmd *cobra.Command, args []string) error { - b := bundle.Get(cmd.Context()) + ctx := cmd.Context() + b := bundle.Get(ctx) + + bundle.ApplyFunc(ctx, b, func(context.Context, *bundle.Bundle) error { + b.Config.Bundle.Force = force + b.Config.Bundle.Deployment.Lock.Force = forceLock + b.Config.Bundle.ComputeID = computeID - b.Config.Bundle.Force = force - b.Config.Bundle.Deployment.Lock.Force = forceLock - b.Config.Bundle.ComputeID = computeID + if cmd.Flag("fail-on-active-runs").Changed { + b.Config.Bundle.Deployment.FailOnActiveRuns = failOnActiveRuns + } - if cmd.Flag("fail-on-active-runs").Changed { - b.Config.Bundle.Deployment.FailOnActiveRuns = failOnActiveRuns - } + return nil + }) - return bundle.Apply(cmd.Context(), b, bundle.Seq( + return bundle.Apply(ctx, b, bundle.Seq( phases.Initialize(), phases.Build(), phases.Deploy(), diff --git a/cmd/bundle/deployment/bind.go b/cmd/bundle/deployment/bind.go index 5412928070..1287eb0449 100644 --- a/cmd/bundle/deployment/bind.go +++ b/cmd/bundle/deployment/bind.go @@ -1,6 +1,7 @@ package deployment import ( + "context" "fmt" "github.com/databricks/cli/bundle" @@ -25,15 +26,14 @@ func newBindCommand() *cobra.Command { cmd.Flags().BoolVar(&forceLock, "force-lock", false, "Force acquisition of deployment lock.") cmd.RunE = func(cmd *cobra.Command, args []string) error { - b := bundle.Get(cmd.Context()) - r := b.Config.Resources - resource, err := r.FindResourceByConfigKey(args[0]) + ctx := cmd.Context() + b := bundle.Get(ctx) + resource, err := b.Config.Resources.FindResourceByConfigKey(args[0]) if err != nil { return err } w := b.WorkspaceClient() - ctx := cmd.Context() exists, err := resource.Exists(ctx, w, args[1]) if err != nil { return fmt.Errorf("failed to fetch the resource, err: %w", err) @@ -43,8 +43,12 @@ func newBindCommand() *cobra.Command { return fmt.Errorf("%s with an id '%s' is not found", resource.TerraformResourceName(), args[1]) } - b.Config.Bundle.Deployment.Lock.Force = forceLock - err = bundle.Apply(cmd.Context(), b, bundle.Seq( + bundle.ApplyFunc(ctx, b, func(context.Context, *bundle.Bundle) error { + b.Config.Bundle.Deployment.Lock.Force = forceLock + return nil + }) + + err = bundle.Apply(ctx, b, bundle.Seq( phases.Initialize(), phases.Bind(&terraform.BindOptions{ AutoApprove: autoApprove, diff --git a/cmd/bundle/deployment/unbind.go b/cmd/bundle/deployment/unbind.go index e7de8a3d47..9f0e4f7c79 100644 --- a/cmd/bundle/deployment/unbind.go +++ b/cmd/bundle/deployment/unbind.go @@ -1,6 +1,8 @@ package deployment import ( + "context" + "github.com/databricks/cli/bundle" "github.com/databricks/cli/bundle/phases" "github.com/databricks/cli/cmd/bundle/utils" @@ -19,14 +21,18 @@ func newUnbindCommand() *cobra.Command { cmd.Flags().BoolVar(&forceLock, "force-lock", false, "Force acquisition of deployment lock.") cmd.RunE = func(cmd *cobra.Command, args []string) error { - b := bundle.Get(cmd.Context()) - r := b.Config.Resources - resource, err := r.FindResourceByConfigKey(args[0]) + ctx := cmd.Context() + b := bundle.Get(ctx) + resource, err := b.Config.Resources.FindResourceByConfigKey(args[0]) if err != nil { return err } - b.Config.Bundle.Deployment.Lock.Force = forceLock + bundle.ApplyFunc(ctx, b, func(context.Context, *bundle.Bundle) error { + b.Config.Bundle.Deployment.Lock.Force = forceLock + return nil + }) + return bundle.Apply(cmd.Context(), b, bundle.Seq( phases.Initialize(), phases.Unbind(resource.TerraformResourceName(), args[0]), diff --git a/cmd/bundle/destroy.go b/cmd/bundle/destroy.go index a0bfb1a4a1..958681f06c 100644 --- a/cmd/bundle/destroy.go +++ b/cmd/bundle/destroy.go @@ -1,6 +1,7 @@ package bundle import ( + "context" "fmt" "os" @@ -30,11 +31,15 @@ func newDestroyCommand() *cobra.Command { ctx := cmd.Context() b := bundle.Get(ctx) - // If `--force-lock` is specified, force acquisition of the deployment lock. - b.Config.Bundle.Deployment.Lock.Force = forceDestroy + bundle.ApplyFunc(ctx, b, func(ctx context.Context, b *bundle.Bundle) error { + // If `--force-lock` is specified, force acquisition of the deployment lock. + b.Config.Bundle.Deployment.Lock.Force = forceDestroy - // If `--auto-approve`` is specified, we skip confirmation checks - b.AutoApprove = autoApprove + // If `--auto-approve`` is specified, we skip confirmation checks + b.AutoApprove = autoApprove + + return nil + }) // we require auto-approve for non tty terminals since interactive consent // is not possible diff --git a/cmd/bundle/utils/utils.go b/cmd/bundle/utils/utils.go index f68ab06b01..e900f47c38 100644 --- a/cmd/bundle/utils/utils.go +++ b/cmd/bundle/utils/utils.go @@ -1,6 +1,8 @@ package utils import ( + "context" + "github.com/databricks/cli/bundle" "github.com/databricks/cli/cmd/root" "github.com/spf13/cobra" @@ -20,5 +22,7 @@ func ConfigureBundleWithVariables(cmd *cobra.Command, args []string) error { // Initialize variables by assigning them values passed as command line flags b := bundle.Get(cmd.Context()) - return b.Config.InitializeVariables(variables) + return bundle.ApplyFunc(cmd.Context(), b, func(ctx context.Context, b *bundle.Bundle) error { + return b.Config.InitializeVariables(variables) + }) } diff --git a/cmd/bundle/validate.go b/cmd/bundle/validate.go index 01b8c18acc..f235e097b5 100644 --- a/cmd/bundle/validate.go +++ b/cmd/bundle/validate.go @@ -6,6 +6,7 @@ import ( "github.com/databricks/cli/bundle" "github.com/databricks/cli/bundle/phases" "github.com/databricks/cli/cmd/bundle/utils" + "github.com/databricks/cli/libs/log" "github.com/spf13/cobra" ) @@ -25,6 +26,12 @@ func newValidateCommand() *cobra.Command { return err } + // Until we change up the output of this command to be a text representation, + // we'll just output all diagnostics as debug logs. + for _, diag := range b.Config.Diagnostics() { + log.Debugf(cmd.Context(), "[%s]: %s", diag.Location, diag.Summary) + } + buf, err := json.MarshalIndent(b.Config, "", " ") if err != nil { return err diff --git a/cmd/root/bundle.go b/cmd/root/bundle.go index 3f9d90db6b..edfc1f4315 100644 --- a/cmd/root/bundle.go +++ b/cmd/root/bundle.go @@ -64,7 +64,13 @@ func loadBundle(cmd *cobra.Command, args []string, load func(ctx context.Context profile := getProfile(cmd) if profile != "" { - b.Config.Workspace.Profile = profile + err = bundle.ApplyFunc(ctx, b, func(ctx context.Context, b *bundle.Bundle) error { + b.Config.Workspace.Profile = profile + return nil + }) + if err != nil { + return nil, err + } } err = bundle.Apply(ctx, b, bundle.Seq(mutator.DefaultMutators()...)) diff --git a/go.mod b/go.mod index 4aaecd1d07..9fd37e6e0b 100644 --- a/go.mod +++ b/go.mod @@ -12,7 +12,6 @@ require ( github.com/hashicorp/hc-install v0.6.3 // MPL 2.0 github.com/hashicorp/terraform-exec v0.20.0 // MPL 2.0 github.com/hashicorp/terraform-json v0.21.0 // MPL 2.0 - github.com/imdario/mergo v0.3.15 // BSD-3-Clause github.com/manifoldco/promptui v0.9.0 // BSD-3-Clause github.com/mattn/go-isatty v0.0.20 // MIT github.com/nwidger/jsoncolor v0.3.2 // MIT diff --git a/go.sum b/go.sum index 545ff9e353..3826f15daa 100644 --- a/go.sum +++ b/go.sum @@ -106,8 +106,6 @@ github.com/hashicorp/terraform-exec v0.20.0 h1:DIZnPsqzPGuUnq6cH8jWcPunBfY+C+M8J github.com/hashicorp/terraform-exec v0.20.0/go.mod h1:ckKGkJWbsNqFKV1itgMnE0hY9IYf1HoiekpuN0eWoDw= github.com/hashicorp/terraform-json v0.21.0 h1:9NQxbLNqPbEMze+S6+YluEdXgJmhQykRyRNd+zTI05U= github.com/hashicorp/terraform-json v0.21.0/go.mod h1:qdeBs11ovMzo5puhrRibdD6d2Dq6TyE/28JiU4tIQxk= -github.com/imdario/mergo v0.3.15 h1:M8XP7IuFNsqUx6VPK2P9OSmsYsI/YFaGil0uD21V3dM= -github.com/imdario/mergo v0.3.15/go.mod h1:WBLT9ZmE3lPoWsEzCh9LPo3TiwVN+ZKEjmz+hD27ysY= github.com/inconshreveable/mousetrap v1.1.0 h1:wN+x4NVGpMsO7ErUn/mUI3vEoE6Jt13X2s0bqwp9tc8= github.com/inconshreveable/mousetrap v1.1.0/go.mod h1:vpF70FUmC8bwa3OWnCshd2FqLfsEA9PFc4w1p2J65bw= github.com/jbenet/go-context v0.0.0-20150711004518-d14ea06fba99 h1:BQSFePA1RWJOlocH6Fxy8MmwDt+yVQYULKfN0RoTN8A= diff --git a/internal/bundle/artifacts_test.go b/internal/bundle/artifacts_test.go index 549b393d2b..0f3769ece3 100644 --- a/internal/bundle/artifacts_test.go +++ b/internal/bundle/artifacts_test.go @@ -33,15 +33,6 @@ func TestAccUploadArtifactFileToCorrectRemotePath(t *testing.T) { whlPath := filepath.Join(dir, "dist", "test.whl") touchEmptyFile(t, whlPath) - artifact := &config.Artifact{ - Type: "whl", - Files: []config.ArtifactFile{ - { - Source: whlPath, - }, - }, - } - wsDir := internal.TemporaryWorkspaceDir(t, w) b := &bundle.Bundle{ @@ -54,7 +45,14 @@ func TestAccUploadArtifactFileToCorrectRemotePath(t *testing.T) { ArtifactPath: wsDir, }, Artifacts: config.Artifacts{ - "test": artifact, + "test": &config.Artifact{ + Type: "whl", + Files: []config.ArtifactFile{ + { + Source: whlPath, + }, + }, + }, }, Resources: config.Resources{ Jobs: map[string]*resources.Job{ @@ -80,9 +78,14 @@ func TestAccUploadArtifactFileToCorrectRemotePath(t *testing.T) { require.NoError(t, err) // The remote path attribute on the artifact file should have been set. - require.Regexp(t, regexp.MustCompile(path.Join(regexp.QuoteMeta(wsDir), `.internal/test\.whl`)), artifact.Files[0].RemotePath) + require.Regexp(t, + regexp.MustCompile(path.Join(regexp.QuoteMeta(wsDir), `.internal/test\.whl`)), + b.Config.Artifacts["test"].Files[0].RemotePath, + ) // The task library path should have been updated to the remote path. - lib := b.Config.Resources.Jobs["test"].JobSettings.Tasks[0].Libraries[0] - require.Regexp(t, regexp.MustCompile(path.Join("/Workspace", regexp.QuoteMeta(wsDir), `.internal/test\.whl`)), lib.Whl) + require.Regexp(t, + regexp.MustCompile(path.Join("/Workspace", regexp.QuoteMeta(wsDir), `.internal/test\.whl`)), + b.Config.Resources.Jobs["test"].JobSettings.Tasks[0].Libraries[0].Whl, + ) } diff --git a/libs/dyn/merge/elements_by_key.go b/libs/dyn/merge/elements_by_key.go new file mode 100644 index 0000000000..3ce571bf7e --- /dev/null +++ b/libs/dyn/merge/elements_by_key.go @@ -0,0 +1,67 @@ +package merge + +import "github.com/databricks/cli/libs/dyn" + +type elementsByKey struct { + key string + keyFunc func(dyn.Value) string +} + +func (e elementsByKey) Map(v dyn.Value) (dyn.Value, error) { + // We know the type of this value is a sequence. + // For additional defence, return self if it is not. + elements, ok := v.AsSequence() + if !ok { + return v, nil + } + + seen := make(map[string]dyn.Value, len(elements)) + keys := make([]string, 0, len(elements)) + + // Iterate in natural order. For a given key, we first see the + // base definition and merge instances that come after it. + for i := range elements { + kv := elements[i].Get(e.key) + key := e.keyFunc(kv) + + // Register element with key if not yet seen before. + ref, ok := seen[key] + if !ok { + keys = append(keys, key) + seen[key] = elements[i] + continue + } + + // Merge this instance into the reference. + nv, err := Merge(ref, elements[i]) + if err != nil { + return v, err + } + + // Overwrite reference. + seen[key] = nv + } + + // Gather resulting elements in natural order. + out := make([]dyn.Value, 0, len(keys)) + for _, key := range keys { + nv, err := dyn.Set(seen[key], e.key, dyn.V(key)) + if err != nil { + return dyn.InvalidValue, err + } + out = append(out, nv) + } + + return dyn.NewValue(out, v.Location()), nil +} + +// ElementsByKey returns a [dyn.MapFunc] that operates on a sequence +// where each element is a map. It groups elements by a key and merges +// elements with the same key. +// +// The function that extracts the key from an element is provided as +// a parameter. The resulting elements get their key field overwritten +// with the value as returned by the key function. +func ElementsByKey(key string, keyFunc func(dyn.Value) string) dyn.MapFunc { + return elementsByKey{key, keyFunc}.Map +} diff --git a/libs/dyn/merge/elements_by_key_test.go b/libs/dyn/merge/elements_by_key_test.go new file mode 100644 index 0000000000..c61f834e5f --- /dev/null +++ b/libs/dyn/merge/elements_by_key_test.go @@ -0,0 +1,52 @@ +package merge + +import ( + "strings" + "testing" + + "github.com/databricks/cli/libs/dyn" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestElementByKey(t *testing.T) { + vin := dyn.V([]dyn.Value{ + dyn.V(map[string]dyn.Value{ + "key": dyn.V("foo"), + "value": dyn.V(42), + }), + dyn.V(map[string]dyn.Value{ + "key": dyn.V("bar"), + "value": dyn.V(43), + }), + dyn.V(map[string]dyn.Value{ + // Use upper case key to test that the resulting element has its + // key field assigned to the output of the key function. + // The key function in this test returns the lower case version of the key. + "key": dyn.V("FOO"), + "value": dyn.V(44), + }), + }) + + keyFunc := func(v dyn.Value) string { + return strings.ToLower(v.MustString()) + } + + vout, err := dyn.MapByPath(vin, dyn.EmptyPath, ElementsByKey("key", keyFunc)) + require.NoError(t, err) + assert.Len(t, vout.MustSequence(), 2) + assert.Equal(t, + vout.Index(0).AsAny(), + map[string]any{ + "key": "foo", + "value": 44, + }, + ) + assert.Equal(t, + vout.Index(1).AsAny(), + map[string]any{ + "key": "bar", + "value": 43, + }, + ) +} diff --git a/libs/dyn/value.go b/libs/dyn/value.go index e9c22bfbea..ecf21abbe8 100644 --- a/libs/dyn/value.go +++ b/libs/dyn/value.go @@ -42,6 +42,15 @@ func NewValue(v any, loc Location) Value { } } +// WithLocation returns a new Value with its location set to the given value. +func (v Value) WithLocation(loc Location) Value { + return Value{ + v: v.v, + k: v.k, + l: loc, + } +} + func (v Value) Kind() Kind { return v.k } diff --git a/libs/template/renderer_test.go b/libs/template/renderer_test.go index 8d0c21010b..e541259e00 100644 --- a/libs/template/renderer_test.go +++ b/libs/template/renderer_test.go @@ -66,7 +66,11 @@ func assertBuiltinTemplateValid(t *testing.T, settings map[string]any, target st require.NoError(t, err) // Apply initialize / validation mutators - b.Config.Workspace.CurrentUser = &bundleConfig.User{User: cachedUser} + bundle.ApplyFunc(ctx, b, func(ctx context.Context, b *bundle.Bundle) error { + b.Config.Workspace.CurrentUser = &bundleConfig.User{User: cachedUser} + return nil + }) + b.Tagging = tags.ForCloud(w.Config) b.WorkspaceClient() b.Config.Bundle.Terraform = &bundleConfig.Terraform{