From 9298634fedd990811e29e03082140d45462332ff Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Wed, 4 Sep 2024 16:24:35 +0200 Subject: [PATCH 1/5] Fixed variable override in target with full variable syntax --- bundle/config/root.go | 47 ++++++++++++------- bundle/tests/complex_variables_test.go | 11 +++-- .../complex_multiple_files/databricks.yml | 11 +++-- .../variables/clusters.yml | 10 +++- 4 files changed, 54 insertions(+), 25 deletions(-) diff --git a/bundle/config/root.go b/bundle/config/root.go index 281c5c2a37..9cacb910a2 100644 --- a/bundle/config/root.go +++ b/bundle/config/root.go @@ -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) { @@ -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 diff --git a/bundle/tests/complex_variables_test.go b/bundle/tests/complex_variables_test.go index d46d8d8c10..f65e43cb04 100644 --- a/bundle/tests/complex_variables_test.go +++ b/bundle/tests/complex_variables_test.go @@ -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) + } } diff --git a/bundle/tests/variables/complex_multiple_files/databricks.yml b/bundle/tests/variables/complex_multiple_files/databricks.yml index cb5d243957..5bb408aede 100644 --- a/bundle/tests/variables/complex_multiple_files/databricks.yml +++ b/bundle/tests/variables/complex_multiple_files/databricks.yml @@ -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" diff --git a/bundle/tests/variables/complex_multiple_files/variables/clusters.yml b/bundle/tests/variables/complex_multiple_files/variables/clusters.yml index badd451648..0186c437b7 100644 --- a/bundle/tests/variables/complex_multiple_files/variables/clusters.yml +++ b/bundle/tests/variables/complex_multiple_files/variables/clusters.yml @@ -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 From e144def4a2889add60b1a26b5c0df3f320f45149 Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Wed, 4 Sep 2024 16:53:48 +0200 Subject: [PATCH 2/5] Update bundle/tests/variables/complex_multiple_files/databricks.yml Co-authored-by: Pieter Noordhuis --- bundle/tests/variables/complex_multiple_files/databricks.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bundle/tests/variables/complex_multiple_files/databricks.yml b/bundle/tests/variables/complex_multiple_files/databricks.yml index 5bb408aede..39b493af45 100644 --- a/bundle/tests/variables/complex_multiple_files/databricks.yml +++ b/bundle/tests/variables/complex_multiple_files/databricks.yml @@ -5,9 +5,9 @@ resources: jobs: my_job: job_clusters: - - job_cluster_key: key§ - new_cluster: ${var.cluster1} - job_cluster_key: key1 + new_cluster: ${var.cluster1} + - job_cluster_key: key2 new_cluster: ${var.cluster2} variables: From e884a3d42f88672fcc8b1936a1902c544e1d92e7 Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Wed, 4 Sep 2024 16:53:55 +0200 Subject: [PATCH 3/5] Update bundle/config/root.go Co-authored-by: Pieter Noordhuis --- bundle/config/root.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bundle/config/root.go b/bundle/config/root.go index 9cacb910a2..d0e9f8ca23 100644 --- a/bundle/config/root.go +++ b/bundle/config/root.go @@ -450,7 +450,7 @@ func rewriteShorthands(v dyn.Value) (dyn.Value, error) { case dyn.KindMap, dyn.KindSequence: // 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. + // 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"))) From 1f1ef8c20df0520da113e3659fe6c25275b93cba Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Wed, 4 Sep 2024 17:02:29 +0200 Subject: [PATCH 4/5] fix --- bundle/config/root.go | 30 ++++++++++------- bundle/tests/complex_variables_test.go | 10 +++--- .../complex_multiple_files/databricks.yml | 32 ++++++++++++++++++- 3 files changed, 55 insertions(+), 17 deletions(-) diff --git a/bundle/config/root.go b/bundle/config/root.go index d0e9f8ca23..433bce1614 100644 --- a/bundle/config/root.go +++ b/bundle/config/root.go @@ -408,13 +408,21 @@ func (r *Root) MergeTargetOverrides(name string) error { 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 { +// isFullVariableOverrideDef checks if the given value is a full syntax varaible override. +// A full syntax variable override is a map with only one of the following +// keys: "default", "lookup". +func isFullVariableOverrideDef(v dyn.Value) bool { + mv, ok := v.AsMap() + if !ok { + return false + } + + if mv.Len() != 1 { + return false + } + for _, keyword := range variableKeywords { - vv, err := dyn.Get(v, keyword) - if err == nil && vv.Kind() != dyn.KindInvalid { + if _, ok := mv.GetByString(keyword); ok { return true } } @@ -449,6 +457,11 @@ func rewriteShorthands(v dyn.Value) (dyn.Value, error) { }, variable.Locations()), nil case dyn.KindMap, dyn.KindSequence: + // If it's a full variable definition, leave it as is. + if isFullVariableOverrideDef(variable) { + 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 @@ -463,11 +476,6 @@ func rewriteShorthands(v dyn.Value) (dyn.Value, error) { } } - // If it's a full variable definition, leave it as is. - if isFullVariableDef(variable) { - return variable, nil - } - // If it's a shorthand, rewrite it to a full variable definition. return dyn.NewValue(map[string]dyn.Value{ "default": variable, diff --git a/bundle/tests/complex_variables_test.go b/bundle/tests/complex_variables_test.go index f65e43cb04..6371071ce4 100644 --- a/bundle/tests/complex_variables_test.go +++ b/bundle/tests/complex_variables_test.go @@ -81,10 +81,10 @@ func TestComplexVariablesOverrideWithMultipleFiles(t *testing.T) { ), )) require.NoError(t, diags.Error()) - 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) + for _, cluster := range b.Config.Resources.Jobs["my_job"].JobClusters { + require.Equalf(t, "14.2.x-scala2.11", cluster.NewCluster.SparkVersion, "cluster: %v", cluster.JobClusterKey) + require.Equalf(t, "Standard_DS3_v2", cluster.NewCluster.NodeTypeId, "cluster: %v", cluster.JobClusterKey) + require.Equalf(t, 4, cluster.NewCluster.NumWorkers, "cluster: %v", cluster.JobClusterKey) + require.Equalf(t, "false", cluster.NewCluster.SparkConf["spark.speculation"], "cluster: %v", cluster.JobClusterKey) } } diff --git a/bundle/tests/variables/complex_multiple_files/databricks.yml b/bundle/tests/variables/complex_multiple_files/databricks.yml index 39b493af45..42a82c6157 100644 --- a/bundle/tests/variables/complex_multiple_files/databricks.yml +++ b/bundle/tests/variables/complex_multiple_files/databricks.yml @@ -9,7 +9,10 @@ resources: new_cluster: ${var.cluster1} - job_cluster_key: key2 new_cluster: ${var.cluster2} - + - job_cluster_key: key3 + new_cluster: ${var.cluster3} + - job_cluster_key: key4 + new_cluster: ${var.cluster4} variables: cluster1: type: complex @@ -17,6 +20,33 @@ variables: cluster2: type: complex description: "A cluster definition" + cluster3: + type: complex + description: "A cluster definition" + cluster4: + type: complex + description: "A cluster definition" include: - ./variables/*.yml + + +targets: + default: + dev: + variables: + cluster3: + 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 + cluster4: + 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 From dae3c8f9b0981284958084a71505074c9bcc2fd8 Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Wed, 4 Sep 2024 17:03:08 +0200 Subject: [PATCH 5/5] typo --- bundle/config/root.go | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/bundle/config/root.go b/bundle/config/root.go index 433bce1614..46578769c4 100644 --- a/bundle/config/root.go +++ b/bundle/config/root.go @@ -468,12 +468,10 @@ func rewriteShorthands(v dyn.Value) (dyn.Value, error) { // and configuration is not merged yet. typeV, err := dyn.GetByPath(v, p.Append(dyn.Key("type"))) if err == nil && typeV.MustString() == "complex" { - if typeV.MustString() == "complex" { - return dyn.NewValue(map[string]dyn.Value{ - "type": typeV, - "default": variable, - }, variable.Locations()), nil - } + return dyn.NewValue(map[string]dyn.Value{ + "type": typeV, + "default": variable, + }, variable.Locations()), nil } // If it's a shorthand, rewrite it to a full variable definition.