-
Notifications
You must be signed in to change notification settings - Fork 72
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Reading variables from file #2171
Changes from all commits
a794490
3f0b5f8
a75c6af
66dae81
d0db1d7
2c9adcb
15c9c5c
01a6a66
c5109cd
f52423c
2a1c4d1
bdb8f1e
19b5018
44aba7e
e4164c1
bfec3c4
6dd8fc2
1656bf5
422585c
47c8fb3
6f20e88
ad6343a
e0c81f0
38b7d27
3e1a335
229fdb2
6e8f5f3
cf42459
d771ce7
f1ab4f0
8e8287b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
{ | ||
"cluster_key": { | ||
"node_type_id": "Standard_DS3_v2" | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
{ | ||
"cluster": { | ||
"node_type_id": "Standard_DS3_v2" | ||
}, | ||
"cluster_key": "mlops_stacks-cluster", | ||
"cluster_workers": 2 | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
foo |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
{ | ||
"cluster": "mlops_stacks-cluster" | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
{ | ||
"cluster_key": "mlops_stacks-cluster-from-file" | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
{ | ||
"cluster_key": "mlops_stacks-cluster", | ||
"cluster_workers": 2 | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
[ | ||
"foo" | ||
] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
!.databricks |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,53 @@ | ||
bundle: | ||
name: TestResolveVariablesFromFile | ||
|
||
variables: | ||
cluster: | ||
type: "complex" | ||
cluster_key: | ||
cluster_workers: | ||
|
||
resources: | ||
jobs: | ||
job1: | ||
job_clusters: | ||
- job_cluster_key: ${var.cluster_key} | ||
new_cluster: | ||
node_type_id: "${var.cluster.node_type_id}" | ||
num_workers: ${var.cluster_workers} | ||
|
||
targets: | ||
default: | ||
default: true | ||
variables: | ||
cluster_workers: 1 | ||
cluster: | ||
node_type_id: "default" | ||
cluster_key: "default" | ||
|
||
without_defaults: | ||
|
||
complex_to_string: | ||
variables: | ||
cluster_workers: 1 | ||
cluster: | ||
node_type_id: "default" | ||
cluster_key: "default" | ||
|
||
string_to_complex: | ||
variables: | ||
cluster_workers: 1 | ||
cluster: | ||
node_type_id: "default" | ||
cluster_key: "default" | ||
|
||
wrong_file_structure: | ||
|
||
invalid_json: | ||
|
||
with_value: | ||
variables: | ||
cluster_workers: 1 | ||
cluster: | ||
node_type_id: "default" | ||
cluster_key: cluster_key_value |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,82 @@ | ||
|
||
=== variable file | ||
>>> $CLI bundle validate -o json | ||
{ | ||
"job_cluster_key": "mlops_stacks-cluster", | ||
"new_cluster": { | ||
"node_type_id": "Standard_DS3_v2", | ||
"num_workers": 2 | ||
} | ||
} | ||
|
||
=== variable file and variable flag | ||
>>> $CLI bundle validate -o json --var=cluster_key=mlops_stacks-cluster-overriden | ||
{ | ||
"job_cluster_key": "mlops_stacks-cluster-overriden", | ||
"new_cluster": { | ||
"node_type_id": "Standard_DS3_v2", | ||
"num_workers": 2 | ||
} | ||
} | ||
|
||
=== variable file and environment variable | ||
>>> BUNDLE_VAR_cluster_key=mlops_stacks-cluster-overriden $CLI bundle validate -o json | ||
{ | ||
"job_cluster_key": "mlops_stacks-cluster-overriden", | ||
"new_cluster": { | ||
"node_type_id": "Standard_DS3_v2", | ||
"num_workers": 2 | ||
} | ||
} | ||
|
||
=== variable has value in config file | ||
>>> $CLI bundle validate -o json --target with_value | ||
{ | ||
"job_cluster_key": "mlops_stacks-cluster-from-file", | ||
"new_cluster": { | ||
"node_type_id": "default", | ||
"num_workers": 1 | ||
} | ||
} | ||
|
||
=== file has variable that is complex but default is string | ||
>>> errcode $CLI bundle validate -o json --target complex_to_string | ||
Error: variable cluster_key is not of type complex, but the value in the variable file is a complex type | ||
|
||
|
||
Exit code: 1 | ||
{ | ||
"job_cluster_key": "${var.cluster_key}", | ||
"new_cluster": { | ||
"node_type_id": "${var.cluster.node_type_id}", | ||
"num_workers": "${var.cluster_workers}" | ||
} | ||
} | ||
|
||
=== file has variable that is string but default is complex | ||
>>> errcode $CLI bundle validate -o json --target string_to_complex | ||
Error: variable cluster is of type complex, but the value in the variable file is not a complex type | ||
|
||
|
||
Exit code: 1 | ||
{ | ||
"job_cluster_key": "${var.cluster_key}", | ||
"new_cluster": { | ||
"node_type_id": "${var.cluster.node_type_id}", | ||
"num_workers": "${var.cluster_workers}" | ||
} | ||
} | ||
|
||
=== variable is required but it's not provided in the file | ||
>>> errcode $CLI bundle validate -o json --target without_defaults | ||
Error: no value assigned to required variable cluster. Assignment can be done using "--var", by setting the BUNDLE_VAR_cluster environment variable, or in .databricks/bundle/<target>/variable-overrides.json file | ||
|
||
|
||
Exit code: 1 | ||
{ | ||
"job_cluster_key": "${var.cluster_key}", | ||
"new_cluster": { | ||
"node_type_id": "${var.cluster.node_type_id}", | ||
"num_workers": "${var.cluster_workers}" | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
cluster_expr=".resources.jobs.job1.job_clusters[0]" | ||
|
||
# defaults from variable file, see .databricks/bundle/<target>/variable-overrides.json | ||
|
||
title "variable file" | ||
trace $CLI bundle validate -o json | jq $cluster_expr | ||
|
||
title "variable file and variable flag" | ||
trace $CLI bundle validate -o json --var="cluster_key=mlops_stacks-cluster-overriden" | jq $cluster_expr | ||
|
||
title "variable file and environment variable" | ||
trace BUNDLE_VAR_cluster_key=mlops_stacks-cluster-overriden $CLI bundle validate -o json | jq $cluster_expr | ||
|
||
title "variable has value in config file" | ||
trace $CLI bundle validate -o json --target with_value | jq $cluster_expr | ||
|
||
# title "file cannot be parsed" | ||
# trace errcode $CLI bundle validate -o json --target invalid_json | jq $cluster_expr | ||
|
||
# title "file has wrong structure" | ||
# trace errcode $CLI bundle validate -o json --target wrong_file_structure | jq $cluster_expr | ||
|
||
title "file has variable that is complex but default is string" | ||
trace errcode $CLI bundle validate -o json --target complex_to_string | jq $cluster_expr | ||
|
||
title "file has variable that is string but default is complex" | ||
trace errcode $CLI bundle validate -o json --target string_to_complex | jq $cluster_expr | ||
|
||
title "variable is required but it's not provided in the file" | ||
trace errcode $CLI bundle validate -o json --target without_defaults | jq $cluster_expr |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,11 +3,14 @@ package mutator | |
import ( | ||
"context" | ||
"fmt" | ||
"os" | ||
"path/filepath" | ||
|
||
"github.com/databricks/cli/bundle" | ||
"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/jsonloader" | ||
"github.com/databricks/cli/libs/env" | ||
) | ||
|
||
|
@@ -23,7 +26,11 @@ func (m *setVariables) Name() string { | |
return "SetVariables" | ||
} | ||
|
||
func setVariable(ctx context.Context, v dyn.Value, variable *variable.Variable, name string) (dyn.Value, error) { | ||
func getDefaultVariableFilePath(target string) string { | ||
return ".databricks/bundle/" + target + "/variable-overrides.json" | ||
} | ||
|
||
func setVariable(ctx context.Context, v dyn.Value, variable *variable.Variable, name string, fileDefault dyn.Value) (dyn.Value, error) { | ||
// case: variable already has value initialized, so skip | ||
if variable.HasValue() { | ||
return v, nil | ||
|
@@ -49,6 +56,26 @@ func setVariable(ctx context.Context, v dyn.Value, variable *variable.Variable, | |
return v, nil | ||
} | ||
|
||
// case: Set the variable to the default value from the variable file | ||
if fileDefault.Kind() != dyn.KindInvalid && fileDefault.Kind() != dyn.KindNil { | ||
hasComplexType := variable.IsComplex() | ||
hasComplexValue := fileDefault.Kind() == dyn.KindMap || fileDefault.Kind() == dyn.KindSequence | ||
|
||
if hasComplexType && !hasComplexValue { | ||
return dyn.InvalidValue, fmt.Errorf(`variable %s is of type complex, but the value in the variable file is not a complex type`, name) | ||
} | ||
if !hasComplexType && hasComplexValue { | ||
return dyn.InvalidValue, fmt.Errorf(`variable %s is not of type complex, but the value in the variable file is a complex type`, name) | ||
} | ||
|
||
v, err := dyn.Set(v, "value", fileDefault) | ||
if err != nil { | ||
return dyn.InvalidValue, fmt.Errorf(`failed to assign default value from variable file to variable %s with error: %v`, name, err) | ||
} | ||
|
||
return v, nil | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The value from the file won't be observed if the value already has a value. We can deal with this in a follow-up. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You mean in case of errors in var file? If variable has value defined already (--var flag) that it should be fine to skip defaults from file There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added test case for value from target which is overriden by file because file has higher in priority list Do we have other cases other than --var where value is defined already? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nevermind, I thought that setting a variable in targets would set the value directly, but it sets the default. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah I also started to doubt and added test to make it clear |
||
// case: Set the variable to its default value | ||
if variable.HasDefault() { | ||
vDefault, err := dyn.Get(v, "default") | ||
|
@@ -64,10 +91,43 @@ func setVariable(ctx context.Context, v dyn.Value, variable *variable.Variable, | |
} | ||
|
||
// We should have had a value to set for the variable at this point. | ||
return dyn.InvalidValue, fmt.Errorf(`no value assigned to required variable %s. Assignment can be done through the "--var" flag or by setting the %s environment variable`, name, bundleVarPrefix+name) | ||
return dyn.InvalidValue, fmt.Errorf(`no value assigned to required variable %s. Assignment can be done using "--var", by setting the %s environment variable, or in %s file`, name, bundleVarPrefix+name, getDefaultVariableFilePath("<target>")) | ||
} | ||
|
||
func readVariablesFromFile(b *bundle.Bundle) (dyn.Value, diag.Diagnostics) { | ||
var diags diag.Diagnostics | ||
|
||
filePath := filepath.Join(b.BundleRootPath, getDefaultVariableFilePath(b.Config.Bundle.Target)) | ||
if _, err := os.Stat(filePath); err != nil { | ||
return dyn.InvalidValue, nil | ||
} | ||
|
||
f, err := os.ReadFile(filePath) | ||
if err != nil { | ||
return dyn.InvalidValue, diag.FromErr(fmt.Errorf("failed to read variables file: %w", err)) | ||
} | ||
|
||
val, err := jsonloader.LoadJSON(f, filePath) | ||
if err != nil { | ||
return dyn.InvalidValue, diag.FromErr(fmt.Errorf("failed to parse variables file %s: %w", filePath, err)) | ||
} | ||
|
||
if val.Kind() != dyn.KindMap { | ||
return dyn.InvalidValue, diags.Append(diag.Diagnostic{ | ||
Severity: diag.Error, | ||
Summary: fmt.Sprintf("failed to parse variables file %s: invalid format", filePath), | ||
Detail: "Variables file must be a JSON object with the following format:\n{\"var1\": \"value1\", \"var2\": \"value2\"}", | ||
}) | ||
} | ||
|
||
return val, nil | ||
} | ||
|
||
func (m *setVariables) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { | ||
defaults, diags := readVariablesFromFile(b) | ||
if diags.HasError() { | ||
return diags | ||
} | ||
err := b.Config.Mutate(func(v dyn.Value) (dyn.Value, error) { | ||
return dyn.Map(v, "variables", dyn.Foreach(func(p dyn.Path, variable dyn.Value) (dyn.Value, error) { | ||
name := p[1].Key() | ||
|
@@ -76,9 +136,10 @@ func (m *setVariables) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnos | |
return dyn.InvalidValue, fmt.Errorf(`variable "%s" is not defined`, name) | ||
} | ||
|
||
return setVariable(ctx, variable, v, name) | ||
fileDefault, _ := dyn.Get(defaults, name) | ||
return setVariable(ctx, variable, v, name, fileDefault) | ||
})) | ||
}) | ||
|
||
return diag.FromErr(err) | ||
return diags.Extend(diag.FromErr(err)) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Temporary disabled because it has tmpDir value in output
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The expectation is that paths to test-supplied directories are replaced by
$TMPDIR
and friends.cc @denik
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean in test runner or in bash script?