Auto-migrate to direct engine when opted in and dry-run is clean#5858
Merged
Conversation
Collaborator
Integration test reportCommit: 66ac14d
11 interesting tests: 4 SKIP, 3 BUG, 3 RECOVERED, 1 KNOWN
|
denik
force-pushed
the
denik/engine-migrate-optin
branch
from
f385901 to
6c09183
Compare
denik
force-pushed
the
denik/engine-migrate-optin
branch
from
6c09183 to
135afd7
Compare
The previous synchronize event did not fire the build workflow for some reason; nudge it with an empty commit. Co-authored-by: Isaac
- Warning when direct is requested and state is terraform now reads "Using X engine for deployment, then will try to migrate your state to direct engine" so the user knows the state-wins outcome is only temporary. Regenerated the golden in state/engine_mismatch. - "Migrated N resource(s) to direct deployment engine." now reports the resource count. - Emit new bool telemetry keys migrated-via-config / migrated-via-env when auto-migration actually rewrites the state, so we can slice by opt-in source. - Test coverage: put the new auto-migrate tests under EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["direct"] instead of [] so they participate in the CI engine-filter; extend auto-migrate-clean to end with a no-op deploy, a config edit, and a with/without --plan deploy of the resulting update (READPLAN matrix). Co-authored-by: Isaac
- Rename bundle/statemgmt/check_direct_migration.go to direct_migration.go
and the top-level function CheckDirectMigration → MigrateToDirect. The
old name suggested a passive check, but the function now can rewrite
state when the user opts in.
- Split metrics: DirectDryMigrate{Success,Warnings} now fire ONLY when
the conversion is truly a dry run (no opt-in). When the user opted in,
emit the new direct_migrate_error / direct_migrate_commit_error /
direct_migrate_warnings keys instead, matching the two failure paths
(state conversion vs. commit) and the warnings case.
Co-authored-by: Isaac
…t.toml Main switched to fragment-per-PR changelog files under .nextchanges/; move the auto-migrate entry from NEXT_CHANGELOG.md to a fragment and regenerate the two new out.test.toml snapshots to pick up the inherited GOOSOnPR skip. Co-authored-by: Isaac
- commitMigration: push the direct state to the workspace FIRST, then rename local files. Prevents this machine going direct while the workspace still has terraform state (which would diverge from other clients). - commitMigration: distinguish "file missing" from a stat error like permission denied — only fs.ErrNotExist means we can proceed. - dryRunMigrate: also SetCollect on the isolated logdiag context so transient diagnostics from SecretScopeFixups/BuildStateFromTF don't render as user-visible "Error:" lines. Same pattern as YAML sync. - Move the "will try to migrate" hint out of PullResourcesState and into MigrateToDirect. PullResourcesState is shared with commands like `bundle debug states` that don't migrate; those were incorrectly promising migration. Now the hint appears only when a deploy is about to attempt the migration, and includes the opt-in source (config vs. env var). - Auto-migrate tests: assert remote state after migration via `bundle debug states --force-pull`, so a broken pushDirectState would surface as a test failure instead of hiding behind local files. Co-authored-by: Isaac
- pushDirectState: after uploading resources.json, back up and delete the remote terraform.tfstate inline and return an error on failure. The previous code called BackupRemoteTerraformState() which swallows errors via log.Warnf, so a workspace could end up with both files authoritative and validateStates would later reject that combination. - commitMigration: distinguish "remote push failed" from "local cache update failed after remote push succeeded". Only the first is a real commit error; local rename failures are logged as warnings and don't break the commit, because the next deploy will pull the direct state from the workspace anyway. - dryRunMigrate: short-circuit when tfState.IDs is empty. A terraform state file with no databricks_* resources produces no SaveState calls, so Finalize writes nothing to disk, and the subsequent os.Open of the temp state fails. Treat that identically to "no terraform state file". - MigrateToDirect: move the "Migrating state..." announce to right before commitMigration is called, so the user only sees it when a commit is actually about to happen (not when there's nothing to migrate). Co-authored-by: Isaac
Three new tests under acceptance/bundle/migrate/: - auto-migrate-empty-tfstate: seed a terraform.tfstate with no databricks_* resources, deploy with bundle.engine=direct, and assert auto-migration is skipped (no "Migrating..." message, no resources.json produced). Locks in the empty-IDs short-circuit fix — previously this would fail at os.Open(tempStatePath). - auto-migrate-push-failure: fault-inject a 403 on the resources.json push and assert commitMigration fails with a clear warning, local state stays terraform, and a subsequent deploy recovers cleanly. Locks in the push-first ordering — a regression that renamed local files before verifying the push would leave a stale local direct state after the failure. - auto-migrate-tfbackup-failure: fault-inject a 403 on the DELETE of remote terraform.tfstate that runs after resources.json is pushed. Assert the migration surfaces a commit_error warning and doesn't rewrite local files. Locks in the "backup fully or not at all" contract — previously BackupRemoteTerraformState swallowed errors via log.Warnf, so a divergent workspace state would ship silently. Co-authored-by: Isaac
- When bundle.engine=direct is set but the local terraform state has no databricks_* resources, remove the terraform state (locally + workspace) instead of trying to write an empty resources.json. With no state anywhere, the next deploy falls back to engine.Default (direct), so the user's opt-in takes effect immediately and the permanent "engine mismatch" warning goes away. - Move EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["direct"] from each new auto-migrate-*/test.toml into the parent acceptance/bundle/migrate/ test.toml (was []). The scripts already set the engine inline; the outer matrix just controls CI's engine filter, so one value suffices and the leaf overrides are redundant. All existing migrate/* tests' out.test.toml snapshots regenerate to reflect the new inherited value. - Refactor MigrateToDirect to inspect tfState directly and route the three cases (no state, empty state, populated state) to distinct code paths; dryRunMigrate now takes the parsed tfState. Co-authored-by: Isaac
- Gate the empty-state sweep on tfState.Attrs also being empty, not just tfState.IDs. IDs is filtered through TerraformToGroupName so an unknown managed resource type produces empty IDs even though the state has real content; sweeping in that case would destroy state the CLI doesn't recognize. Only sweep when the terraform state is truly empty (no managed resources at all). - sweepEmptyTerraformState: return an error on local rename failure instead of a warning. Best-effort left a stale local terraform.tfstate behind, which the next deploy's PullResourcesState would pick and re-enter the mismatch loop. ErrNotExist stays fine — the local file may have never been synced. - Extract recordDryRunNoop helper to remove the duplicate telemetry block across the tfState==nil and empty-Attrs branches. - Assert workspace/delete + terraform.tfstate.backup import in the empty-tfstate acceptance test; previously the test only inspected local files, so a broken remote sweep would pass silently. Co-authored-by: Isaac
The old warning "Deployment engine ... does not match the existing state. Using X engine from the existing state." with a full state-files list was technically correct but misleading in the auto-migration case: it made the migration sound like a lost opt-in, when actually we're about to attempt it. And the state-files list is diagnostic noise for a deploy warning. - PullResourcesState: suppress the generic mismatch warning for the auto-migration case (direct opt-in against terraform state); the message and the state-files list belonged to `bundle debug states`, not to deploy. - process.go: after PullResourcesState, print a dedicated one-liner when opts.Deploy is set and the auto-migration path applies. Non-deploy commands like `bundle debug states` no longer see any warning at all — they just show the state files, which is what the command is for. - engine_mismatch acceptance test: update titles/script to reflect the new "debug states is quiet" contract. Co-authored-by: Isaac
The auto-migrate-push-failure and auto-migrate-tfbackup-failure tests care about the failure path, not about which opt-in mechanism triggered it. Config-mode opt-in is already covered by auto-migrate-clean; env-var mode is already covered by auto-migrate-envvar. Switch the fault tests to DATABRICKS_BUNDLE_ENGINE=direct so their goldens don't carry the update_file.py yml edit — the opt-in signal is visible on the deploy command itself. Co-authored-by: Isaac
- Rename direct auto-migrate success keys from kebab-case to snake_case
so all seven migration keys share the direct_* prefix:
migrated-via-config → direct_migrated_via_config
migrated-via-env → direct_migrated_via_env
A single `grep '^direct_'` now catches every migration bool.
- Add a per-test print_migration_telemetry helper (wrapping the shared
print_telemetry_bool_values) that reports migration bools and clears
out.requests.txt so each call sees only the just-completed deploy's
telemetry. Call it after every deploy in the five auto-migrate tests.
The goldens now visibly track each transition: dry-run success →
direct_migrate_commit_error on a faulted push → direct_migrated_via_*
on the retry.
Co-authored-by: Isaac
- commitMigration: hard-fail (return commit_error) when local rename fails after remote push succeeds. Previously we logged a warning and reported direct_migrated_via_* success telemetry even though local was left with a stale terraform.tfstate — telemetry over-reported. AlwaysPull deploys self-heal on the next run, but this machine's next `bundle plan` (which also uses AlwaysPull) could see confusing output before that. Aligning telemetry with what the user actually sees on this checkout is more honest. - BackupRemoteTerraformState: use filer.OverwriteIfExists when writing the .backup path. Without it, a retry after a failed delete leaves the .backup already there → Write bails → delete never runs, leaving the remote terraform.tfstate to linger forever. The tfbackup-failure test's retry now cleans up remote state without the "file already exists" warning (which was previously visible in the golden). - print_migration_telemetry: extract as a shared helper into acceptance/bundle/migrate/script.prepare so it's scoped to migrate tests only, not global. Callers explicitly `rm -f out.requests.txt` after each call because some tests (empty-tfstate) want to keep the log for a subsequent print_requests.py assertion. Co-authored-by: Isaac
The old name implied the "empty state" case was special, but the helper just backs up terraform state (remote + local) — nothing about it depends on whether the state is empty. Rename accordingly so a future caller that needs to back up populated state (e.g. as part of consolidating commitMigration's inline logic) doesn't feel awkward reusing it. Also clarify recordAutoMigrateSource: requestedEngine.Type can resolve to direct from either the config or the env var (config wins in ResolveEngineSetting); ConfigType is the correct signal for "was this a durable opt-in?" — env-only opt-ins have ConfigType == EngineNotSet. Doc-only clarification on the metric-key comment. Co-authored-by: Isaac
…file - Rename: the function does a real conversion into a separate location, it's not "dry" — the caller decides whether to commit or discard. - Write the temp state to `resources.migrating.json` alongside the final `resources.json` instead of `os.TempDir()/…`. Same directory means same filesystem, so commitMigration's os.Rename can't fail with EXDEV (tmpfs vs. project disk is the common failure case). Clean up stale leftovers from a crashed previous run so UpgradeToWrite's O_EXCL open of the .wal doesn't fail. Co-authored-by: Isaac
Previously ResolveEngineSetting was gated on shouldReadState (Deploy / AlwaysPull / InitIDs / …). That let commands like `bundle sync`, `bundle open`, and `bundle validate` silently accept a garbage DATABRICKS_BUNDLE_ENGINE value because they never called the resolver — only state-touching commands would surface the error. Move the call out of the guarded block so every bundle command that goes through ProcessBundleRet validates the env var. Cost is negligible (config lookup + env read, no I/O), and the outer-scope declaration that was there for the same reason is gone too. Co-authored-by: Isaac
After the sweep, print_requests already proves the workspace API calls happened, but `bundle debug states --force-pull` shows the full state landscape (local + remote, terraform + direct). Its empty output positively asserts that no active terraform.tfstate exists anywhere and no resources.json was written — a stronger end-to-end proof than just checking local files or the request log. Co-authored-by: Isaac
denik
force-pushed
the
denik/engine-migrate-optin
branch
from
e2ea3c2 to
66ac14d
Compare
pietern
approved these changes
Jul 15, 2026
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.
Changes
After a successful terraform deploy, if the user opted in via
bundle.engine: directorDATABRICKS_BUNDLE_ENGINE=direct, the CLI now migrates terraform state to direct. The direct state is pushed to the workspace first; local files are renamed only after the workspace has the new state. Failure surfaces as a warning and gets recorded in telemetry; the deploy itself is never affected.A pre-deploy warning announces the upcoming migration attempt so the user isn't surprised. When the terraform state has no databricks_* resources, the state is swept away (locally + remotely) instead of round-tripped through a conversion, so opt-in bundles with empty state stop hitting the mismatch warning on every deploy.
Split post-deploy telemetry:
direct_drymigrate_*still records dry-run outcomes when the user hasn't opted in. Newdirect_migrate_{error,commit_error,warnings}anddirect_migrated_via_{config,env}cover the opt-in path.Tests
Five new acceptance tests under
acceptance/bundle/migrate/:auto-migrate-clean: config opt-in, edit +--planvariant.auto-migrate-envvar: env-var opt-in.auto-migrate-empty-tfstate: empty terraform state gets swept, not converted.auto-migrate-push-failure: fault-injected 403 on the resources.json push leaves local state intact and recordsdirect_migrate_commit_error.auto-migrate-tfbackup-failure: fault-injected 403 on the remote terraform.tfstate delete surfaces a commit error; retry recovers idempotently.This pull request and its description were written by Isaac.