Skip to content

Commit

Permalink
Fixed variable override in target with full variable syntax
Browse files Browse the repository at this point in the history
  • Loading branch information
andrewnester committed Sep 4, 2024
1 parent ca6332a commit 9298634
Show file tree
Hide file tree
Showing 4 changed files with 54 additions and 25 deletions.
47 changes: 31 additions & 16 deletions bundle/config/root.go
Original file line number Diff line number Diff line change
Expand Up @@ -406,6 +406,22 @@ func (r *Root) MergeTargetOverrides(name string) error {
return r.updateWithDynamicValue(root)
}

var variableKeywords = []string{"default", "lookup"}

// isFullVariableDef checks if the given value is a variable definition.
// A variable definition is a map with one of the following
// keys: "default", "value", "lookup".
func isFullVariableDef(v dyn.Value) bool {
for _, keyword := range variableKeywords {
vv, err := dyn.Get(v, keyword)
if err == nil && vv.Kind() != dyn.KindInvalid {
return true
}
}

return false
}

// 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) {
Expand Down Expand Up @@ -433,30 +449,29 @@ func rewriteShorthands(v dyn.Value) (dyn.Value, error) {
}, variable.Locations()), nil

case dyn.KindMap, dyn.KindSequence:
lookup, err := dyn.Get(variable, "lookup")
// If lookup is set, we don't want to rewrite the variable and return it as is.
if err == nil && lookup.Kind() != dyn.KindInvalid {
return variable, nil
}

// Check if the original definition of variable has a type field.
// If it has a type field, it means the shorthand is a value of a complex type.
// Type might not be found if the variable overriden in a separate file
// and configuration is not merged yet.
typeV, err := dyn.GetByPath(v, p.Append(dyn.Key("type")))
if err != nil {
return dyn.NewValue(map[string]dyn.Value{
"default": variable,
}, variable.Locations()), nil
if err == nil && typeV.MustString() == "complex" {
if typeV.MustString() == "complex" {
return dyn.NewValue(map[string]dyn.Value{
"type": typeV,
"default": variable,
}, variable.Locations()), nil
}
}

if typeV.MustString() == "complex" {
return dyn.NewValue(map[string]dyn.Value{
"type": typeV,
"default": variable,
}, variable.Locations()), nil
// If it's a full variable definition, leave it as is.
if isFullVariableDef(variable) {
return variable, nil
}

return variable, nil
// If it's a shorthand, rewrite it to a full variable definition.
return dyn.NewValue(map[string]dyn.Value{
"default": variable,
}, variable.Locations()), nil

default:
return variable, nil
Expand Down
11 changes: 6 additions & 5 deletions bundle/tests/complex_variables_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -81,9 +81,10 @@ func TestComplexVariablesOverrideWithMultipleFiles(t *testing.T) {
),
))
require.NoError(t, diags.Error())

require.Equal(t, "14.2.x-scala2.11", b.Config.Resources.Jobs["my_job"].JobClusters[0].NewCluster.SparkVersion)
require.Equal(t, "Standard_DS3_v2", b.Config.Resources.Jobs["my_job"].JobClusters[0].NewCluster.NodeTypeId)
require.Equal(t, 4, b.Config.Resources.Jobs["my_job"].JobClusters[0].NewCluster.NumWorkers)
require.Equal(t, "false", b.Config.Resources.Jobs["my_job"].JobClusters[0].NewCluster.SparkConf["spark.speculation"])
for i, cluster := range b.Config.Resources.Jobs["my_job"].JobClusters {
require.Equalf(t, "14.2.x-scala2.11", cluster.NewCluster.SparkVersion, "cluster: %v", i)
require.Equalf(t, "Standard_DS3_v2", cluster.NewCluster.NodeTypeId, "cluster: %v", i)
require.Equalf(t, 4, cluster.NewCluster.NumWorkers, "cluster: %v", i)
require.Equalf(t, "false", cluster.NewCluster.SparkConf["spark.speculation"], "cluster: %v", i)
}
}
11 changes: 8 additions & 3 deletions bundle/tests/variables/complex_multiple_files/databricks.yml
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,16 @@ resources:
jobs:
my_job:
job_clusters:
- job_cluster_key: key
new_cluster: ${var.cluster}
- job_cluster_key: key§
new_cluster: ${var.cluster1}
- job_cluster_key: key1
new_cluster: ${var.cluster2}

variables:
cluster:
cluster1:
type: complex
description: "A cluster definition"
cluster2:
type: complex
description: "A cluster definition"

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,18 @@ targets:
default:
dev:
variables:
cluster:
cluster1:
spark_version: "14.2.x-scala2.11"
node_type_id: "Standard_DS3_v2"
num_workers: 4
spark_conf:
spark.speculation: false
spark.databricks.delta.retentionDurationCheck.enabled: false
cluster2:
default:
spark_version: "14.2.x-scala2.11"
node_type_id: "Standard_DS3_v2"
num_workers: 4
spark_conf:
spark.speculation: false
spark.databricks.delta.retentionDurationCheck.enabled: false

0 comments on commit 9298634

Please sign in to comment.