acc: matrix the bundle suite over deployment-history recording - #6444
Merged
Conversation
Every test under acceptance/bundle now runs twice: once as before, and once with DATABRICKS_BUNDLE_RECORD_DEPLOYMENT_HISTORY set. That puts the recording path under the deploy, redeploy, destroy and drift cases the suite already covers instead of only the tests written for it. Subtrees that cannot carry it opt out and say why: bind and unbind are not supported by the service, a saved plan does not carry the deployment stamp, and the templates and migrate trees assert output the second variant would duplicate. This is the test-harness half only, so it is independent of the DMS implementation. The nostamp helper strips deployment_id and version_id so one golden serves both variants; until the recording code lands the variable is unread and the two variants are identical. Co-authored-by: Isaac <no-reply@databricks.com>
…irect one The direct cell pinned only the engine, so it picked up both halves of the recording matrix and ran every bundle test twice. Each cell now names the variants it covers, and a third - directdms - takes recording on: terraform DATABRICKS_BUNDLE_ENGINE=terraform direct DATABRICKS_BUNDLE_ENGINE=direct,DMS= directdms DATABRICKS_BUNDLE_ENGINE=direct,DMS=true Verified against bundle/quiet-levels: each filter passes exactly one variant and skips the other two, so the three cells partition the matrix and direct is back to one run per test. Done in CI rather than as a third DATABRICKS_BUNDLE_ENGINE value, which is a user-facing setting and a bundle config field: "directdms" would advertise a feature that is still gated off, and the out.*.direct.* goldens keyed off the engine name would each need a twin. Co-authored-by: Isaac <no-reply@databricks.com>
The exclusion kept DMS=true off every cloud test, which left the recording path covered only by the local testserver. The service is deployed to prod, and the bundle/dms tests already record against a real workspace, so the reason it named no longer holds. Co-authored-by: Isaac <no-reply@databricks.com>
shreyas-goenka
marked this pull request as ready for review
August 31, 2026 11:28
andrewnester
approved these changes
Aug 31, 2026
| - name: "terraform" | ||
| envfilter: "DATABRICKS_BUNDLE_ENGINE=terraform" | ||
| - name: "direct" | ||
| envfilter: "DATABRICKS_BUNDLE_ENGINE=direct,DMS=" |
Contributor
There was a problem hiding this comment.
Should we say false or empty here?
Contributor
Author
There was a problem hiding this comment.
Empty is better because that maps to the current default, where DMS is not opted into.
|
|
||
| # Saved plans don't carry the deployment stamp. A first plan writes the deployment record, | ||
| # so `deploy --plan` creates resources without it and reports drift on the next plan. | ||
| EnvMatrixExclude.dms_no_readplan = ["DMS=true", "READPLAN=1"] |
Contributor
There was a problem hiding this comment.
I believe this can be a blocker (maybe now, maybe later on?) as we strive to readplan to always work as it becomes fundamental concept later. What is the plan to address this?
Contributor
Author
There was a problem hiding this comment.
Yes. Its a release blocked I wanted to address this ASAP but got stuck waiting on reviews. This should be easy to address. +1 on blocking release until we have read plan. Or atleast having a clear error.
- Change EnvMatrix.DMS from ["", "true"] to ["false", "true"] - Update CI envfilter from DMS= to DMS=false for consistency - Move nostamp() function from bundle/script.prepare to bin/nostamp as executable - Regenerate all out.test.toml files with new variant naming Co-authored-by: Isaac <no-reply@databricks.com>
The off variant goes back to "" rather than "false". Every subtree that opts out pins EnvMatrix.DMS = [""], and a CI cell's ENVFILTER compares the value exactly, so renaming only the root would leave those subtrees matching neither cell and silently skipped in both. The two values are the same at runtime: recording reads only "true". nostamp keeps the notes it had as a shell function: the three shapes the stamp takes and the two values it deliberately leaves alone. Co-authored-by: Isaac <no-reply@databricks.com>
The matrix landed without the two things that make it work: the 23 subtrees that pin EnvMatrix.DMS = [""] because they cannot carry a recording run, and the 78 scripts that pipe their output through nostamp so one golden serves both variants. Without them bin/nostamp was dead code and every opted-out subtree would have run a variant it cannot support as soon as the recording code lands. corrupted-wal-entry uploads one more file, since the opt-out gives that directory a test.toml it did not have. Co-authored-by: Isaac <no-reply@databricks.com>
…ipts EnvMatrix.DMS is a short alias; acceptance_test.go is what expands it into DATABRICKS_BUNDLE_RECORD_DEPLOYMENT_HISTORY. Without that map the variant set a variable nothing reads, so the whole matrix ran as a duplicate of the engine cells. Three scripts are shared by many tests through source, and each builds its own request filter: permissions, pipelines/recreate-keys and run_as/pipelines. They now drop the service's own traffic and strip the stamp, the same way the per-test scripts do. Co-authored-by: Isaac <no-reply@databricks.com>
denik
reviewed
Sep 1, 2026
| |= with_entries(select((.key | IN("deployment_id", "version_id")) == false or .value == ""))) | ||
| | ((.. | objects | .changes? | objects) | ||
| |= with_entries(select(.key | IN("deployment.deployment_id", "deployment.version_id") | not))) | ||
| | del(.. | objects | select(.changes == {}) | .changes)' |
Contributor
There was a problem hiding this comment.
This is hard to read and if format ever changes I imagine the errors will not be very useful, can we do this in Python?
| @@ -15,4 +15,4 @@ title "Deploy (migrates state)" | |||
| trace $CLI bundle deploy | |||
|
|
|||
| title "Print state after deploy" | |||
| trace print_state.py | |||
| trace print_state.py | nostamp --indent 1 | |||
Contributor
There was a problem hiding this comment.
Why do we need nostamp here if we exclude this test from DMS?
| for _, kv := range customEnv { | ||
| key, value, _ := strings.Cut(kv, "=") | ||
| full, ok := envAliases[key] | ||
| if !ok || hasKey(env, full) { |
Contributor
There was a problem hiding this comment.
why do this hasKey(env, full) check?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Updates the test suite to run DMS tests as part of CI (nightlies and on PR). Right now the CI jobs are a dupliacte of direct because DMS is not merged yes,