acc: add BundleConfig setting by denik · Pull Request #2809 · databricks/cli

denik · 2025-05-02T14:03:42Z

Changes

New setting BundleConfig, to define portions of bundle config in test.toml which is then propagated to all child configs (like any other config setting).

Why

This feature allows structuring tests better, by having common section defined at the parent level and re-used by all children. This is also a foundation for BundleConfigMatrix (similar to EnvMatrix) which will allow to test variations on the config without duplicating test.

Some use cases:

Today we have to copy bundle.name boilerplate in every test, this becomes a one time thing at the parent level (acc: Use BundleConfig to define bundle.name once #2845)
Today many artifact tests need to be test with both dynamic_version and without, this would allow to reuse config definition. We also have other variations for config, those can be matrix-tested with BundleConfigMatrix.
A number of tests use envsubst to parametrize. Using BundleConfig is a replacement for that is declarative, inheritable and supports complex structures where envsubst only works with strings.

Tests

New acceptance selftest.
Testing in practice: acc: Use BundleConfig to define bundle.name once #2845

pietern · 2025-05-15T07:45:15Z

acceptance/acceptance_test.go

 	return latestWheel
 }
+
+func applyBundleConfig(t *testing.T, tmpDir string, bundleConfig map[string]any) string {


Can you include a docstring saying what this function returns?

Done. Also changed it to return bool. 3a991f4

pietern · 2025-05-15T07:45:58Z

acceptance/acceptance_test.go

+		if configValue == "" {
+			continue
+		}
+		// either "" or a map are allowed


What is the significance of the empty string?

Disables the setting. Added a comment.

pietern · 2025-05-15T07:47:25Z

acceptance/acceptance_test.go

+	}
+
+	var configPath, configData string
+	filenames := []string{"databricks.yml", "databricks.yml.tmpl"}


Why the tmpl file?

Also, the configuration could include the filename directly to make this not bundle-specific (or specific to databricks.yml but applicable to other YAML files in the test directory).

Great idea, added BundleConfigTarget for this: 3a991f4

pietern · 2025-05-15T07:48:07Z

acceptance/acceptance_test.go

+
+	for _, filename := range filenames {
+		path := filepath.Join(tmpDir, filename)
+		exists := false


We need to know whether to exclude this file from the comparison to avoid test failing because there is a new file that is unaccounted for.

pietern · 2025-05-15T07:49:17Z

acceptance/acceptance_test.go

+	newConfigData := configData
+	var applied []string
+
+	for _, configName := range utils.SortedKeys(validConfig) {


The keys are the "path" in YAML to apply the override to (e.g. config2.resources.jobs.example_job)?

If so, please include comments or examples of the shape of the input to this function to clarify.

config2 is not part of yaml, it is the name of the config (for override purposes). There is selftest that shows how it works.

pietern · 2025-05-15T07:54:22Z

acceptance/internal/config.go

+		if len(key) > 0 && key[0] == "BundleConfig" {
+			continue
+		}
+		t.Errorf("Undecoded key in %s[%d]: %#v", path, ind, key)


What does "undecoded" mean? How do these keys now end up in the config struct?

It's means a key in TOML file was not mapped to any struct field.

But how does the "BundleConfig" map get populated if it is not decoded?

I would expect either:

The "BundleConfig" field to be populated with the config and this code to never execute

This code to execute and the "BundleConfig" field to remain empty

Good question! This seems to be an issue with toml parser's handling of map[string]any type.

Somehow it decodes it but also complains about it, This is what happens if I remove that check:

--- FAIL: TestAccept/selftest/bundleconfig/override (0.02s) config.go:221: Undecoded key in selftest/bundleconfig/test.toml[0]: toml.Key{"BundleConfig", "config1", "bundle"} config.go:221: Undecoded key in selftest/bundleconfig/test.toml[1]: toml.Key{"BundleConfig", "config1", "bundle", "name"} config.go:221: Undecoded key in selftest/bundleconfig/test.toml[2]: toml.Key{"BundleConfig", "config2", "resources", "jobs", "example_job"} config.go:221: Undecoded key in selftest/bundleconfig/test.toml[3]: toml.Key{"BundleConfig", "config2", "resources", "jobs", "example_job", "name"} config.go:221: Undecoded key in selftest/bundleconfig/test.toml[4]: toml.Key{"BundleConfig", "config2", "resources", "jobs", "example_job", "new_string"} config.go:221: Undecoded key in selftest/bundleconfig/test.toml[5]: toml.Key{"BundleConfig", "config2", "resources", "jobs", "example_job", "new_list"} config.go:221: Undecoded key in selftest/bundleconfig/test.toml[6]: toml.Key{"BundleConfig", "config2", "resources", "jobs", "example_job", "new_map"} config.go:221: Undecoded key in selftest/bundleconfig/test.toml[7]: toml.Key{"BundleConfig", "config2", "resources", "jobs", "example_job", "new_map", "key"} config.go:221: Undecoded key in selftest/bundleconfig/test.toml[8]: toml.Key{"BundleConfig", "config2", "resources", "jobs", "example_job", "list2"} config.go:221: Undecoded key in selftest/bundleconfig/test.toml[9]: toml.Key{"BundleConfig", "config2", "resources", "jobs", "example_job", "string2"}

pietern · 2025-05-15T07:57:00Z

acceptance/internal/bundle_config_test.go

+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			out, err := MergeBundleConfig(tt.initialYaml, tt.bundleConfig)


Can this function take a map[string]any as input instead?

it already does?

bundle_config.go:func MergeBundleConfig(source string, bundleConfig map[string]any) (string, error) {

I mean for "source". The mix of unmarshalling and receiving an object is confusing.

I see what you mean, but there is some logic to it: the function receives config as a string and returns new updated config as a string. This allows it to encapsulate (and potentially have test cases for) details related to yaml parsing, such as whether it is strict or not, whether it preserves comments. The test runner on the other hand knows which file we're reading and writing and handles all I/O.

The fact that it also receives unmarshalled bundleConfig is just because we do all of unmarshalling of config in one place and there is no other way to receive bundleConfig.

There is a lot of unnecessary marshalling / unmarshalling going on with this implementation though. It works for now and does not cause noticeable overhead, but worth rewriting a bit (for later).

andrewnester

I think we discussed it offline that it's important for this change to also include materialised test.toml configuration which will contain actual bundle config set up for this test (e.g. all parent BundleConfig merged). Is it something you plan to work on?

denik · 2025-05-16T13:17:29Z

I think we discussed it offline that it's important for this change to also include materialised test.toml configuration which will contain actual bundle config set up for this test (e.g. all parent BundleConfig merged). Is it something you plan to work on?

Not in this PR, that's separate issue.

pietern

Minor remaining comments.

pietern · 2025-05-19T08:38:40Z

acceptance/acceptance_test.go

+			continue
+		}
+		// either "" or a map are allowed
+		// Empty string can be used to disable an update that was defined in parent config


Comment out of date; empty string will cause fatal now.

Not sure what you mean, empty string is explicitly handled below to skip this setting:

configValue := bundleConfig[configName] if configValue == "" { continue }

Let me move the comment about it to where the code is, to make it more obvious: ed731f4

pietern · 2025-05-19T08:42:44Z

acceptance/internal/bundle_config_test.go

+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			out, err := MergeBundleConfig(tt.initialYaml, tt.bundleConfig)


I mean for "source". The mix of unmarshalling and receiving an object is confusing.

pietern · 2025-05-19T08:45:49Z

acceptance/internal/config.go

+		if len(key) > 0 && key[0] == "BundleConfig" {
+			continue
+		}
+		t.Errorf("Undecoded key in %s[%d]: %#v", path, ind, key)


But how does the "BundleConfig" map get populated if it is not decoded?

I would expect either:

The "BundleConfig" field to be populated with the config and this code to never execute

This code to execute and the "BundleConfig" field to remain empty

## Changes Using BundleConfig option (#2809), specify boilerplate bundle.name once and clean up individual test cases. The tests with diagnostics (lineno, column) are left alone. Test acceptance/bundle/volume_path is split into invidual tests, one databricks.yml per test (since BundleConfig only updates databricks.yml[.tmpl] in the root of the test. ## Why Simpler to read and write, make each test focussed on what's important. ## Tests Existing tests. Benchmarked acceptance test suite (local) to ensure this does not add a lot of overhead (due to parsing/marshalling every config): ``` This branch: Time (mean ± σ): 18.364 s ± 0.857 s [User: 32.707 s, System: 27.281 s] Range (min … max): 17.002 s … 19.922 s 10 runs Main: Time (mean ± σ): 18.337 s ± 0.789 s [User: 32.613 s, System: 27.235 s] Range (min … max): 17.078 s … 19.342 s 10 runs ```

## Changes - Remove BundleConfig setting (#2809) which allows post-processing of databricks.yml - Add bundle.name section to all config that need it explicitly. ## Why It is not used outside of original 'name' use case and I don't think that use case alone warrants the complexity. This makes test runner simpler and understanding the test output simpler. In particular line numbers in the output become correct. There is also log noise that is removed. Dynamically generated configs are useful sometimes, but that can be done by a script, no need to support it on test runner level. Test runner can provide input to configuration via EnvMatrix.

denik temporarily deployed to test-trigger-is May 2, 2025 14:03 — with GitHub Actions Inactive

denik force-pushed the denik/acc-bundle-config branch from c882225 to 66a318b Compare May 2, 2025 14:36

denik temporarily deployed to test-trigger-is May 2, 2025 14:37 — with GitHub Actions Inactive

denik force-pushed the denik/acc-bundle-config branch from 66a318b to 2f36255 Compare May 2, 2025 14:59

denik temporarily deployed to test-trigger-is May 2, 2025 14:59 — with GitHub Actions Inactive

denik force-pushed the denik/acc-bundle-config branch from 2f36255 to b01cadd Compare May 6, 2025 09:10

denik temporarily deployed to test-trigger-is May 6, 2025 09:10 — with GitHub Actions Inactive

denik force-pushed the denik/acc-bundle-config branch from b01cadd to 7d3810d Compare May 7, 2025 18:36

denik temporarily deployed to test-trigger-is May 7, 2025 18:36 — with GitHub Actions Inactive

denik force-pushed the denik/acc-bundle-config branch from 7d3810d to 92a3b4c Compare May 7, 2025 18:49

denik temporarily deployed to test-trigger-is May 7, 2025 18:49 — with GitHub Actions Inactive

denik force-pushed the denik/acc-bundle-config branch from 92a3b4c to 7c82324 Compare May 7, 2025 21:12

denik temporarily deployed to test-trigger-is May 7, 2025 21:12 — with GitHub Actions Inactive

denik force-pushed the denik/acc-bundle-config branch from 7c82324 to 31b42d0 Compare May 9, 2025 14:28

denik temporarily deployed to test-trigger-is May 9, 2025 14:28 — with GitHub Actions Inactive

denik mentioned this pull request May 9, 2025

acc: Use BundleConfig to define bundle.name once #2845

Merged

denik marked this pull request as ready for review May 9, 2025 14:46

denik requested review from andrewnester, anton-107, pietern and shreyas-goenka as code owners May 9, 2025 14:46

denik force-pushed the denik/acc-bundle-config branch from 31b42d0 to c4484b3 Compare May 13, 2025 19:20

denik temporarily deployed to test-trigger-is May 13, 2025 19:20 — with GitHub Actions Inactive

denik force-pushed the denik/acc-bundle-config branch from c4484b3 to bfa5241 Compare May 15, 2025 07:42

denik temporarily deployed to test-trigger-is May 15, 2025 07:42 — with GitHub Actions Inactive

denik force-pushed the denik/acc-bundle-config branch from bfa5241 to d3eb299 Compare May 15, 2025 07:46

denik temporarily deployed to test-trigger-is May 15, 2025 07:46 — with GitHub Actions Inactive

pietern reviewed May 15, 2025

View reviewed changes

denik temporarily deployed to test-trigger-is May 15, 2025 08:14 — with GitHub Actions Inactive

denik requested a review from pietern May 15, 2025 08:19

denik temporarily deployed to test-trigger-is May 15, 2025 08:39 — with GitHub Actions Inactive

andrewnester reviewed May 16, 2025

View reviewed changes

denik requested a review from andrewnester May 16, 2025 13:17

pietern approved these changes May 19, 2025

View reviewed changes

denik temporarily deployed to test-trigger-is May 19, 2025 09:19 — with GitHub Actions Inactive

denik temporarily deployed to test-trigger-is May 19, 2025 09:34 — with GitHub Actions Inactive

denik temporarily deployed to test-trigger-is May 19, 2025 09:37 — with GitHub Actions Inactive

denik temporarily deployed to test-trigger-is May 19, 2025 09:39 — with GitHub Actions Inactive

denik temporarily deployed to test-trigger-is May 19, 2025 09:41 — with GitHub Actions Inactive

denik added 9 commits May 19, 2025 11:45

acc: add BundleConfig setting

6b23bd9

Add BundleConfigTarget

e74b58a

add selftest/bundleconfig/disabled

8e46c19

add shortcut in isSameYAMLContent

8a4c0a1

Move comment about "" up

4626264

add disabled2 test

c072c8f

add differnet_target test

86a4bae

rewrite bundleConfigTarget init to make it more readable

b3beab9

fix comments

17f103c

denik force-pushed the denik/acc-bundle-config branch from af48f8b to 17f103c Compare May 19, 2025 09:45

denik temporarily deployed to test-trigger-is May 19, 2025 09:45 — with GitHub Actions Inactive

denik enabled auto-merge May 19, 2025 09:51

denik disabled auto-merge May 19, 2025 09:52

denik added this pull request to the merge queue May 19, 2025

Merged via the queue into main with commit f5d1580 May 19, 2025
10 checks passed

denik deleted the denik/acc-bundle-config branch May 19, 2025 13:21

denik mentioned this pull request Dec 30, 2025

acc: remove BundleConfig feature #4182

Merged

Conversation

denik commented May 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Why

Tests

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewnester left a comment

Choose a reason for hiding this comment

Uh oh!

denik commented May 16, 2025

Uh oh!

pietern left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

denik commented May 2, 2025 •

edited

Loading