Reading variables from file #2171

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

Changes

New flag --var-file which accepts JSON file with variables values. Variable values can have any type including complex type

--var-file has default value: .databricks/bundle/<target>/variable-overrides.json, i.e. CLI tries to stat and read that file every time during variable initialisation phase

It is possible to use this flag together with --var flag, priority of the value will be given to --var.

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

pietern

Nice work, thanks.

pietern · 2025-01-21T15:27:13Z

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("var-file", "", `file path to a JSON file containing variables. Example: --var-file="/path/to/vars.json"`)


Suggested change

cmd.PersistentFlags().String("var-file", "", `file path to a JSON file containing variables. Example: --var-file="/path/to/vars.json"`)

cmd.PersistentFlags().String("var-file", "", `path to a JSON file containing variables. Example: --var-file="/path/to/vars.json"`)

pietern · 2025-01-21T15:35:27Z

acceptance/bundle/variables/var_file_overrides/script

+trace errcode $CLI bundle validate -o json --var-file=var_files/string_to_complex.json | jq $cluster_expr
+
+# variable is required but it's not provided in the file
+trace errcode $CLI bundle validate -o json --target without-defaults --var-file=var_files/without_required.json | jq $cluster_expr


Could you include the comments as echo "..." statements so they appear in the output?

That makes reading the output and understanding the expectations easier.

Added small helper for nice printing which we can further adjust if needed

pietern · 2025-01-21T15:42:36Z

acceptance/bundle/variables/var_file_overrides/output.txt

+
+>>> errcode $CLI bundle validate -o json --var-file=var_files/wrong_file_structure.json
+Error: failed to parse variables file: var_files/wrong_file_structure.json:1:1: expected a map, found a sequence
+  in var_files/wrong_file_structure.json:1:1


This location is always the same because it is taken from the root value. Can be omitted.

pietern · 2025-01-21T15:43:54Z

@ilyakuz-db Please update the PR summary as well to reflect the actual flag name.

ilyakuz-db · 2025-01-21T17:20:16Z

@pietern @denik as discussed offline added default path for --var-path

ilyakuz-db · 2025-01-21T17:22:20Z

acceptance/bundle/help/bundle-deploy/output.txt

+  -p, --profile string    ~/.databrickscfg profile
+  -t, --target string     bundle target to use (if applicable)
+      --var strings       set values for variables defined in bundle config. Example: --var="foo=bar"
+      --var-file string   path to a JSON file containing variables. Example: --var-file="/path/to/vars.json" (default ".databricks/bundle/<target>/vars.json")


This default message is hardcoded to avoid having invalid path value in variable

ilyakuz-db · 2025-01-21T17:23:48Z

acceptance/bundle/variables/empty/output.txt

@@ -1,4 +1,4 @@
-Error: no value assigned to required variable a. Assignment can be done through the "--var" flag or by setting the BUNDLE_VAR_a environment variable
+Error: no value assigned to required variable a. Assignment can be done using "--var" or "--var-file", by setting the BUNDLE_VAR_a environment variable, or in .databricks/bundle/<target>/vars.json file


Any other suggestions for good user message are welcome! It became quite complicated

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 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

fix: Remove redundant test

1656bf5

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

ilyakuz-db changed the title ~~feat: Read variables from file~~ Reading variables from file Jan 21, 2025

pietern reviewed Jan 21, 2025

View reviewed changes

ilyakuz-db added 6 commits January 21, 2025 18:00

feat: Default value for variable path

422585c

chore: Regenerate other snapshots

47c8fb3

Added annotations to output

6f20e88

Cleanup in flag description

ad6343a

Regenerate help snapshots

e0c81f0

Remove unnecessary locations

38b7d27

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

ilyakuz-db commented Jan 21, 2025

View reviewed changes

Fix test with error message

3e1a335

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

	cmd.PersistentFlags().String("var-file", "", `file path to a JSON file containing variables. Example: --var-file="/path/to/vars.json"`)
	cmd.PersistentFlags().String("var-file", "", `path to a JSON file containing variables. Example: --var-file="/path/to/vars.json"`)

		@@ -1,4 +1,4 @@
		Error: no value assigned to required variable a. Assignment can be done through the "--var" flag or by setting the BUNDLE_VAR_a environment variable
		Error: no value assigned to required variable a. Assignment can be done using "--var" or "--var-file", by setting the BUNDLE_VAR_a environment variable, or in .databricks/bundle/<target>/vars.json file

Reading variables from file #2171

Are you sure you want to change the base?

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

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

pietern commented Jan 21, 2025

ilyakuz-db commented Jan 21, 2025

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 commented Jan 16, 2025 •

edited

Loading

denik Jan 17, 2025 •

edited

Loading