Reading variables from file #2171

ilyakuz-db · 2025-01-16T17:11:58Z

Changes

New source of default values for variables - variable file .databricks/bundle/<target>/variable-overrides.json

CLI tries to stat and read that file every time during variable initialisation phase

Tests

Acceptance tests

pietern

Nice work, thanks!

pietern · 2025-01-17T07:48:52Z

cmd/bundle/variables.go

@@ -6,4 +6,5 @@ import (

 func initVariableFlag(cmd *cobra.Command) {
 	cmd.PersistentFlags().StringSlice("var", []string{}, `set values for variables defined in bundle config. Example: --var="foo=bar"`)
+	cmd.PersistentFlags().String("vars-file-path", "", `file path to a JSON file containing variables. Example: --vars-file-path="/path/to/vars.json"`)


Let's call this --var-file, shorter and same meaning.

+1 or --varfile, don't have to remember to put dash.

pietern · 2025-01-17T07:54:35Z

cmd/bundle/utils/utils.go

+		vars := map[string]any{}
+		err = json.Unmarshal(f, &vars)
+		if err != nil {
+			return diag.FromErr(fmt.Errorf("failed to parse variables file: %w", err))


If you pass a file with "foo" in there, or a list, this will return an error without suggesting what the right format is. Could you look into using jsonloader here? It would be nice to return a diag.Error with details suggesting what the correct format is, if the contents doesn't match what we expect.

denik · 2025-01-16T22:45:01Z

cmd/bundle/utils/utils.go

-	// Initialize variables by assigning them values passed as command line flags
-	diags = diags.Extend(configureVariables(cmd, b, variables))
+	if len(variables) > 0 && variableFilePath != "" {
+		return b, diag.Errorf("cannot specify both --var and --vars-file-path flags")


That seems like unnecessary constraint? Why not process one (e.g. file first) and then the other?

IIRC we discussed on meeting that it may cause confusion in some edge cases but with low profit:

is it be obvious to give priority to second arg?

since both flags might be partial - should we merge them or not

non-obvious type mismatch between values of these flags (--var is always string)

is it be obvious to give priority to second arg?

What do you mean by second? Simple thing would be to have fixed priority: --var takes over --varfile. Complex but better thing would be to take into account order on the command line --varfile file1 --var ... --varfile file2, but we can leave that out of scope.

since both flags might be partial - should we merge them or not

Yes. Same as we do with env and command line parameters currently, we merge them. Command line take priority of both specify the same var.

non-obvious type mismatch between values of these flags (--var is always string)

Does not matter, we do not do any type of merging or other logic based on type. We just select the value.

What do you mean by second?

Yeah I meant order. I'm not sure whether it's clear to user, they should follow documentation to understand this priority

If it's not too hard to implement following command-line arguments order that would be great.

Otherwise fixed order is much better that complete disable --var.

So in general my concern is whether it brings value to user because the use case of using both flags might be not so common but it may add some confusion and code becomes more complex

Variable resolution might feel non-obvious already and it will add more uncertainty

Also if we add this feature now it won't be possible to remove later because of backward-compatibility but if we restrict now we can always allow later if customers ask for that

BTW, passing --var twice for the same argument raises an error: #2176

So to be consistent with that I change my suggestion to raise an error if variable is defined in both --var and --varfile. But I still think it should be per-variable check.

Let's discuss in the slack thread? I don't mind adding merge but at the same time I don't want logic to be too complex because we already have multiple layers of overrides and with files added we will also have some type inconsistency between values

Discussed offline - we can limit the scope of this PR by erroring in case both --var and --var-file is used and we'll revisit that later.

denik · 2025-01-16T22:49:10Z

acceptance/bundle/variables/flag_overrides/script

+$CLI bundle validate -o json --vars-file-path=vars.json | jq .resources.jobs.job1.job_clusters[0]
+
+# variable flag
+$CLI bundle validate -o json --var="cluster_key=mlops_stacks-cluster" | jq .resources.jobs.job1.job_clusters[0].job_cluster_key


How about testing

both --var and --vars-file-path?

file not found

file cannot be parsed

file has variable name that is not defined

file has variable name that is complex but default is string and vice versa

variable is required but it's not provided in the file

file has variable name that is not defined
file has variable name that is complex but default is string and vice versa
variable is required but it's not provided in the file

@denik Not sure about these, they are part of generic variable handling, should they be covered here? It feels like we starting to cover edge cases in place where it shouldn't be tested, WDYT?

Added tests, but would be nice to hear your opinion on testing edge cases

What I don't like in that approach is that snapshot tests are very easy to add/update but at the same time hard to debug and I've seen many times how such tests became write-only thing when people didn't want to investigate the failure and just updated the snapshot

denik · 2025-01-16T22:50:32Z

bundle/config/root.go

+// Variables can have any type of value, including complex types
+func (r *Root) InitializeAnyTypeVariables(vars map[string]any) error {
+	for name, val := range vars {
+		if _, ok := r.Variables[name]; !ok {


nit: instead of assigning to _, let's give it a name and use below to call Set() on it.

denik · 2025-01-16T22:51:23Z

bundle/config/root_test.go

+	assert.Equal(t, "value", (root.Variables["string"].Value))
+	assert.Equal(t, 1, (root.Variables["int"].Value))
+	assert.Equal(t, []map[string]int{{"c": 3}}, (root.Variables["complex"].Value))
+}


What does it add on top of acceptance test? I think it can be removed/merged into acceptance test.

Would it be better to also have simple localised check? It also serve showcase purpose

Acceptance test is nice because it's top-level and is really easy to add but at the same time they are harder to debug in case of failure

Added acceptance tests, let me know what you think of keeping these as well

If acceptance tests cover the same functionality, I'd remove this ones, because they have non-zero maintenance cost.

denik · 2025-01-16T22:52:04Z

bundle/config/root_test.go

+	assert.Equal(t, []map[string]int{{"c": 3}}, (root.Variables["complex"].Value))
+}
+
+func TestInitializeAnyTypeVariablesUndeclared(t *testing.T) {


Same here, it would be good to see full output, so acceptance test is preferred. That shows whether you use diagnostics correctly.

denik · 2025-01-16T22:53:15Z

bundle/config/variable/variable.go

+	//   a. Variable value obtained from arguments, example: `--var="foo=bar"`
+	//   b. Variable value obtained from the file, example: `--vars-file-path="/path/to/file"`
+	// 2. Environment variable. eg: BUNDLE_VAR_foo=bar
+	// 3. Default value as defined in the applicable targets block


Let's keep it in simple and with ordered rule:

Command line flag. For example: `--var="foo=bar"-2

Command line flag with json variables: --varfile

Target variable. eg: BUNDLE_VAR_foo=bar and the test

...

Let's discuss here whether we need to merge values from flags

denik · 2025-01-17T08:23:04Z

cmd/bundle/variables.go

@@ -6,4 +6,5 @@ import (

 func initVariableFlag(cmd *cobra.Command) {
 	cmd.PersistentFlags().StringSlice("var", []string{}, `set values for variables defined in bundle config. Example: --var="foo=bar"`)
+	cmd.PersistentFlags().String("vars-file-path", "", `file path to a JSON file containing variables. Example: --vars-file-path="/path/to/vars.json"`)


+1 or --varfile, don't have to remember to put dash.

…iles

ilyakuz-db · 2025-01-21T17:26:42Z

cmd/bundle/utils/utils.go

 	"github.com/spf13/cobra"
 )

+func GetDefaultVariableFilePath(target string) string {
+	return ".databricks/bundle/" + target + "/vars.json"


cc @pietern @fjakobs to confirm the path / filename

since we will live with that name for centuries :)

I'd use variable-overrides.json.

I like mentioning "overrides"

Updated

denik · 2025-01-22T13:23:55Z

acceptance/bundle/variables/var_file_overrides/output.txt

+
+=== file cannot be parsed
+>>> errcode $CLI bundle validate -o json --var-file=var_files/invalid_json.json
+Error: failed to parse variables file: error decoding JSON at :0:0: invalid character 'o' in literal false (expecting 'a')


Error: failed to parse variables file:

would be very helpful to print the file name there

Error: failed to parse variables file var_files/invalid_json.json:

Good suggestion! Updated

denik · 2025-01-22T13:29:51Z

acceptance/bundle/variables/var_file_overrides/databricks.yml

+  without-defaults:
+
+  # see .databricks/bundle/default_target/ for variable values
+  with-default-variable-file:


Why do you need a target here? The default variables are read for any target, right? so it's the same as without-defaults.

I made separate target to avoid affecting other tests by this default

E.g. I put .databrick/bundle/without-defaults/variable-overrides.json file and previous test implicitly has this additional variable sources which may lead to confusion if anything goes wrong

ilyakuz-db · 2025-01-23T13:08:27Z

acceptance/bundle/variables/file-defaults/script

+trace BUNDLE_VAR_cluster_key=mlops_stacks-cluster-overriden $CLI bundle validate -o json | jq $cluster_expr
+
+# title "file cannot be parsed"
+# trace errcode $CLI bundle validate -o json --target invalid_json | jq $cluster_expr


Temporary disabled because it has tmpDir value in output

The expectation is that paths to test-supplied directories are replaced by $TMPDIR and friends.

cc @denik

Do you mean in test runner or in bash script?

pietern · 2025-01-23T13:14:30Z

acceptance/bundle/variables/file-defaults/script

+trace BUNDLE_VAR_cluster_key=mlops_stacks-cluster-overriden $CLI bundle validate -o json | jq $cluster_expr
+
+# title "file cannot be parsed"
+# trace errcode $CLI bundle validate -o json --target invalid_json | jq $cluster_expr


The expectation is that paths to test-supplied directories are replaced by $TMPDIR and friends.

cc @denik

pietern · 2025-01-23T13:22:16Z

bundle/config/mutator/set_variables.go

+
+		return v, nil
+	}
+


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.

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

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?

Nevermind, I thought that setting a variable in targets would set the value directly, but it sets the default.

Yeah I also started to doubt and added test to make it clear

CLI: * Added text output templates for apps list and list-deployments ([#2175](#2175)). * Fix duplicate "apps" entry in help output ([#2191](#2191)). Bundles: * Allow yaml-anchors in schema ([#2200](#2200)). * Show an error when non-yaml files used in include section ([#2201](#2201)). * Set WorktreeRoot to sync root outside git repo ([#2197](#2197)). * fix: Detailed message for using source-linked deployment with file_path specified ([#2119](#2119)). * Allow using variables in enum fields ([#2199](#2199)). * Add experimental-jobs-as-code template ([#2177](#2177)). * Reading variables from file ([#2171](#2171)). * Fixed an apps message order and added output test ([#2174](#2174)). * Default to forward slash-separated paths for path translation ([#2145](#2145)). * Include a materialized copy of built-in templates ([#2146](#2146)).

ilyakuz-db added 2 commits January 16, 2025 16:42

feat: Read variables from file

a794490

test: Acceptance test for flag overrides

3f0b5f8

ilyakuz-db requested review from pietern, andrewnester, shreyas-goenka and denik as code owners January 16, 2025 17:11

ilyakuz-db temporarily deployed to test-trigger-is January 16, 2025 17:12 — with GitHub Actions Inactive

pietern reviewed Jan 17, 2025

View reviewed changes

denik reviewed Jan 17, 2025

View reviewed changes

ilyakuz-db added 4 commits January 17, 2025 11:40

fix: Rename to --var-file

a75c6af

fix: More detailed error for json parsing

66dae81

fix: Cleanup

d0db1d7

fix: More acceptance tests

2c9adcb

ilyakuz-db temporarily deployed to test-trigger-is January 17, 2025 18:37 — with GitHub Actions Inactive

fix: Flaky test

15c9c5c

ilyakuz-db temporarily deployed to test-trigger-is January 18, 2025 11:58 — with GitHub Actions Inactive

ilyakuz-db added 3 commits January 20, 2025 12:05

fix: Remove extra tests

01a6a66

Merge branch 'main' of github.com:databricks/cli into feat/variable-f…

c5109cd

…iles

fix: Update help snapshots

f52423c

ilyakuz-db temporarily deployed to test-trigger-is January 20, 2025 11:14 — with GitHub Actions Inactive

fix: Message in test

2a1c4d1

ilyakuz-db temporarily deployed to test-trigger-is January 20, 2025 11:20 — with GitHub Actions Inactive

fix: Message in acceptance tests

bdb8f1e

ilyakuz-db temporarily deployed to test-trigger-is January 20, 2025 11:38 — with GitHub Actions Inactive

ilyakuz-db added 2 commits January 20, 2025 12:47

fix: Flaky test

19b5018

fix: Remove platform specific part from output

44aba7e

ilyakuz-db temporarily deployed to test-trigger-is January 20, 2025 13:40 — with GitHub Actions Inactive

fix: Try jq -b to preserve window output

e4164c1

ilyakuz-db temporarily deployed to test-trigger-is January 20, 2025 15:30 — with GitHub Actions Inactive

ilyakuz-db requested a review from denik January 20, 2025 15:35

ilyakuz-db commented Jan 21, 2025

View reviewed changes

ilyakuz-db temporarily deployed to test-trigger-is January 21, 2025 17:28 — with GitHub Actions Inactive

ilyakuz-db requested review from pietern and removed request for andrewnester and shreyas-goenka January 21, 2025 17:29

Use variable-overrides.json name

229fdb2

ilyakuz-db temporarily deployed to test-trigger-is January 22, 2025 12:29 — with GitHub Actions Inactive

Fix message in test

6e8f5f3

ilyakuz-db temporarily deployed to test-trigger-is January 22, 2025 13:20 — with GitHub Actions Inactive

denik reviewed Jan 22, 2025

View reviewed changes

denik self-requested a review January 22, 2025 13:28

denik reviewed Jan 22, 2025

View reviewed changes

Add file path to parsing error

cf42459

ilyakuz-db temporarily deployed to test-trigger-is January 22, 2025 14:46 — with GitHub Actions Inactive

ilyakuz-db added 2 commits January 23, 2025 14:05

Remove --var-file

d771ce7

Regenerate snapshots

f1ab4f0

ilyakuz-db temporarily deployed to test-trigger-is January 23, 2025 13:06 — with GitHub Actions Inactive

ilyakuz-db commented Jan 23, 2025

View reviewed changes

pietern approved these changes Jan 23, 2025

View reviewed changes

Adds test case with value from target specified

8e8287b

ilyakuz-db temporarily deployed to test-trigger-is January 23, 2025 14:07 — with GitHub Actions Inactive

pietern approved these changes Jan 23, 2025

View reviewed changes

ilyakuz-db enabled auto-merge January 23, 2025 14:25

ilyakuz-db added this pull request to the merge queue Jan 23, 2025

Merged via the queue into main with commit 0487e81 Jan 23, 2025
9 checks passed

ilyakuz-db deleted the feat/variable-files branch January 23, 2025 14:47

andrewnester mentioned this pull request Jan 23, 2025

[Release] Release v0.239.1 #2218

Merged

Reading variables from file #2171

Reading variables from file #2171

Conversation

ilyakuz-db commented Jan 16, 2025 • edited Loading

Changes

Tests

pietern left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

denik Jan 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilyakuz-db Jan 23, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilyakuz-db commented Jan 16, 2025 •

edited

Loading

denik Jan 17, 2025 •

edited

Loading

ilyakuz-db Jan 23, 2025 •

edited

Loading