Conversation
Collaborator
Integration test reportCommit: aa942be
Top 4 slowest tests (at least 2 minutes):
|
denik
force-pushed
the
denik/migration-before-deploy
branch
from
September 22, 2026 11:08
32fe6bb to
14e8c46
Compare
denik
force-pushed
the
denik/migration-before-deploy
branch
from
September 22, 2026 12:21
6c1fd6c to
7b7aabe
Compare
Vivek1106-04
pushed a commit
to Vivek1106-04/cli
that referenced
this pull request
Sep 23, 2026
## Changes Reorder the steps in `ProcessBundleRet`. The single pre-build `shouldReadState` block is split in two: the state **pull** (which determines the engine) stays up front, while opening the resolved state moves to after Build. **Old:** pull-state → open-state → FastValidate → Validate → Build → deploy **New:** pull-state → FastValidate → Validate → Build → open-state → deploy where: - **pull-state** = `PullResourcesState` — determine the engine, set `MigratingToDirect`, print the deploy notice, and validate `--select`. Needs only the pulled engine descriptor, so it stays before FastValidate (unchanged). - **open-state** = open the direct state, DMS fetch/open, deployment-history enforcement, `InitIDs`, and the `--plan` file load. Needs the opened/resolved state, so it moves to after Build. The reorder is safe because the two sides are independent: - validate/build do not read the deployment state, and - open-state does not depend on validate/build output. (`Validate` may issue workspace API calls — that's why it is separate from `FastValidate` — but it does not need the state, so running it before open-state is fine.) The auto-migration still runs post-deploy, so behavior is unchanged. ## Why Prep for databricks#6749 — move the terraform→direct migration before deploy: that migration converts the bundle config into the direct state and must run **after Build**, because library and `${artifacts.*}` references (e.g. `whl: ./dist/*.whl`) are only resolved during Build. Running migration before Build would bake unexpanded globs into the migrated state instead of resolved remote paths. Landing this reorder on its own keeps databricks#6749's diff to the actual behavior change. This pull request and its description were written by Isaac. --------- Co-authored-by: Isaac <no-reply@databricks.com>
When the direct engine is requested (the default) and the existing state
still uses terraform, convert the state to the direct engine before the
run proceeds, instead of after deploy. deploy/destroy commit the migration
(resources.json written and pushed, terraform.tfstate backed up); plan and
summary keep it in memory. The migration runs after phases.Build so its
conversion and plan check see library and ${artifacts.*} references
resolved (matching what a direct deploy records), and it falls back to the
terraform engine if the plan check fails. Terraform-state cleanup after the
commit is best-effort: resources.json already outranks the terraform state
by serial, so a failed backup/delete only warns.
Co-authored-by: Isaac <no-reply@databricks.com>
Now that the migration runs before deploy, a hard error blocks a deploy that would otherwise succeed on terraform. Treat every failure before resources.json is pushed (parse, conversion, empty-state sweep) as non-fatal: warn and deploy on terraform this time, retrying the migration next run - matching what plan-check and push failures already did. Only a failure after the push (placing/opening the local state) stays an error, since the workspace is already committed to direct; reword those messages to say the migration succeeded and re-running recovers. Co-authored-by: Isaac <no-reply@databricks.com>
The pre-deploy migration plan-checks the converted state. If that plan would recreate (destroy + create) an existing resource, do not commit the migration: a recreate risks data loss, whether it comes from a conversion that did not faithfully reproduce an immutable field or from a real pending config change (which terraform would recreate too). Fall back to terraform this run and retry the migration next run, once the recreate is applied. Records direct_migrate_recreate_planned. checkPlanOnTempState now returns the plan so the caller can inspect the planned actions. Co-authored-by: Isaac <no-reply@databricks.com>
The pre-deploy migration flips the engine terraform->direct, but the SDK user agent only appends dimensions, so tagging engine/terraform in PullResourcesState and then engine/direct after a successful migration left both on a migrated deploy's requests. PullResourcesState now skips the tag on the auto-migration path (direct requested, state still terraform); the caller sets engine from the resolved stateDesc.Engine once the migration has run, been skipped, or fallen back - so a migrated deploy is engine/direct, a fallback is engine/terraform, and there is never a stale second tag. The two generate commands, which never migrate, tag the resolved engine too. auto-migrate-envvar now records the deploy's engine tags to lock this in. Co-authored-by: Isaac <no-reply@databricks.com>
Records (no fix) how the pre-deploy migration treats a schema's named id fields when they change or are backend-normalized: - name backend-lowercased (config MySchema vs deployed myschema): spurious warnOnIDFieldRename warning (also emitted by the terraform deploy's dry-run telemetry), but the follow-up plan converges. - catalog_name legitimately changed (immutable id): only a warning, no recreate - the migration records the config value and the moved-catalog drift is silently stable. - storage_root trailing slash normalized: absorbed by normalize_slash, no warning, converges. Co-authored-by: Isaac <no-reply@databricks.com>
Its golden had captured the buggy engine/terraform engine/direct double tag on the migrating plan/deploy commands; the user-agent fix makes those a single engine/direct. Regenerated (the only golden that recorded the double tag). Co-authored-by: Isaac <no-reply@databricks.com>
…odels, pipelines Extends the schema characterization (no fix) across more resources: - volume name backend-lowercased: spurious rename warning, converges (same as schema name). - volume_type changed (both a provided id field AND recreate_on_changes): the recreate classification wins - the recreate guard fires and the run stays on terraform (contrast with catalog_name, a pure provided id field, which only warns). - registered_model catalog_name changed (immutable id): warn + silently stable drift, no recreate (same as schema catalog_name). - pipeline storage changed (pure recreate_on_changes): recreate guard fires. Catalogs are direct-only (no terraform converter), so they are never a migration scenario and are intentionally not covered. Co-authored-by: Isaac <no-reply@databricks.com>
The migration seeded id-composing fields (provided_id_fields, updatable_id_fields) from config, which snapshotted a pending id change as already applied: a genuinely-moved/renamed resource then plan-converged and silently drifted from the backend, while a backend-normalized value (identifier case, trailing slash) produced a spurious "rename not applied" warning - also leaked onto plain terraform deploys via the dry-run telemetry. reconcileIDFields now seeds each id field from the deployed terraform value unless it differs from config only by backend normalization (case-insensitive + trailing-slash), in which case the config value is kept. So a real id change surfaces in the plan (recreate for provided_id -> caught by the recreate guard; rename update for updatable_id), and a normalized value converges with no warning. warnOnIDFieldRename is removed (its genuine case is now the recreate guard, its false-positive case is gone). Regenerated the id-field characterization goldens, which now assert the fixed behavior (recreate on genuine change, clean converge on normalization). Co-authored-by: Isaac <no-reply@databricks.com>
The terraform fallback leaves two terraform.tfstate* files, and find lists them in filesystem order, which differs on Windows. Pipe the finds through sort so the golden is deterministic across platforms. Co-authored-by: Isaac <no-reply@databricks.com>
First cloud coverage for the pre-deploy migration: renaming a schema (an immutable provided id field) must recreate. Verifies against a real workspace that the recreate guard detects the recreate, falls back to terraform (which recreates the schema under the new name), and the following deploy migrates the now-matching state to direct. Passed on aws-cli. Co-authored-by: Isaac <no-reply@databricks.com>
Companion to cloud-recreate-schema. A volume name is an updatable id field, so unlike a schema (which recreates) it is renamed in place. Verifies against a real workspace that the migration seeds the deployed name, the plan renames the volume (UpdateWithID) rather than snapshotting the new name as applied, the migration succeeds (no fallback), and the schema alongside it is untouched. Passed on aws-cli. Co-authored-by: Isaac <no-reply@databricks.com>
Extend the two schema recreate tests (local auto-migrate-recreate and cloud cloud-recreate-schema) with a step that deploys the recreate WITHOUT --auto-approve first: the migration guard falls back to terraform, which refuses the destructive recreate (exit 1) and changes nothing. The following --auto-approve step then applies it. The cloud step passed on aws-cli. Co-authored-by: Isaac <no-reply@databricks.com>
Third cloud resource type. A registered model name is a provided id field, so a rename recreates - the migration guard detects it against the real backend and falls back to terraform, which recreates the model; the retry then migrates. Unlike schemas/volumes, registered models are not in the destructive-approval group, so the recreate applies without --auto-approve. Passed on aws-cli. Co-authored-by: Isaac <no-reply@databricks.com>
…nario> Drop the redundant cloud-/idfield-/recreate-/auto-migrate- prefixes: migration and running on both testserver and cloud are the defaults, and Cloud=true only adds a cloud run. Name by resource and scenario instead (schema-rename, volume-type-change, ...). Remove auto-migrate-rename, now fully covered by the more thorough schema-rename (which also runs on cloud). Co-authored-by: Isaac <no-reply@databricks.com>
…lume) Upgrade schema-name-normalized and volume-name-normalized to Cloud=true (unique names + cleanup trap). This covers the last behavior class on cloud: a backend-normalized id (UC lowercasing) converges through the migration with no warning or recreate. Verified on aws-cli. Together with schema-rename, volume-rename and registered-model-rename, all id-field/recreate classes now run against a real workspace. Co-authored-by: Isaac <no-reply@databricks.com>
…alog catalog_name is an immutable provided id field; moving a schema to another catalog recreates it. Create the destination catalog out of band (catalogs are direct-only, so they cannot be a bundle resource here), then verify on a real workspace: the recreate is refused without --auto-approve and, with it, moves the schema to the new catalog; the retry migrates. Passed on aws-cli. Co-authored-by: Isaac <no-reply@databricks.com>
…f-band catalog Moving a registered model to another catalog recreates it (catalog_name is a provided id field). Create the destination catalog out of band (it auto-creates a default schema the moved model reuses). Registered models are not in the destructive-approval group, so the recreate applies without --auto-approve; the retry migrates. Passed on aws-cli. Co-authored-by: Isaac <no-reply@databricks.com>
storage is a recreate_on_changes field on pipelines. Give the pipeline a notebook library and a DBFS storage path (no external location needed) so it deploys on a real workspace, then verify: changing storage plans a recreate; pipelines are in the destructive-approval group, so it is refused without --auto-approve and applied with it; the retry migrates. Passed on aws-cli. Co-authored-by: Isaac <no-reply@databricks.com>
…only schema-storage-normalized, schema-storage-change and volume-type-change need a registered external location on a real workspace, so they are not Cloud-enabled. Add a note in each pointing to the cloud-enabled tests that cover the same class. Co-authored-by: Isaac <no-reply@databricks.com>
migrate/command holds the explicit "bundle deployment migrate" command tests; migrate/auto holds the auto-migration-on-deploy tests. Drop the now-redundant auto-migrate- prefix from the latter. The shared script.prepare and test.toml stay at migrate/ and are inherited by both subtrees. Co-authored-by: Isaac <no-reply@databricks.com>
…esource A recreate in the migrated state's first plan is a genuine pending config change (the same one terraform would apply), and the deploy's approval flow already gates it behind --auto-approve. So stop refusing the migration on a planned recreate: commit the migrated state and let the normal approval gate the recreate on direct. direct_migrate_recreate_planned is kept purely as an observability metric. The migrate/auto recreate tests are updated to the new flow (migrate commits, then the deploy refuses/applies the recreate on direct). Co-authored-by: Isaac <no-reply@databricks.com>
denik
force-pushed
the
denik/migration-before-deploy
branch
from
September 23, 2026 11:02
edc9b68 to
8b1f858
Compare
…ne-storage-change On a real workspace a direct-engine pipeline recreate leaves a field the next plan re-updates (1 changed), while the testserver converges (1 unchanged); a single golden can't match both. The migrate + recreate-gating flow (the test's subject) is unaffected and still covered on cloud; the clean-redeploy path is covered by the schema/registered-model recreate tests. Co-authored-by: Isaac <no-reply@databricks.com>
…ng id compare reconcileIDFields is a no-op for permissions/grants sub-nodes (their adapters declare no id fields), so the len(parts)==3 guard was redundant - call it unconditionally. Every id-composing field is a string, so assert both values are strings (erroring otherwise) and inline the case- and trailing-slash-insensitive compare, dropping the speculative non-string structdiff fallback and the idFieldNormalizedEqual helper. Co-authored-by: Isaac <no-reply@databricks.com>
…migrate test The metric fires whenever a committing migration's plan would recreate a resource, but only schema-storage-change asserted it. Add print_recreate_planned_metric (a stable, single-key telemetry assertion) at the recreate step of the remaining recreate tests. The cloud tests (schema/pipeline/registered-model) need RecordRequests=true to read telemetry; assert only the recreate-planned key so backend-varying metrics (e.g. conversion warnings) don't destabilize the golden. Co-authored-by: Isaac <no-reply@databricks.com>
Use print_migration_telemetry (all direct_* metrics) instead of the recreate-only helper, matching schema-storage-change. At the recreate step the set is direct_migrate_recreate_planned + direct_migrated_via_env, both CLI-side and stable across engines/clouds. Drops the print_recreate_planned_metric helper. Co-authored-by: Isaac <no-reply@databricks.com>
When an id-composing field differs between config and the deployed terraform state (beyond backend normalization), reconcileIDFields now emits a warning naming the resource, field, and both values, and whether the resource will be recreated (provided-id) or renamed (updatable-id) - so the plan that follows the migration is not a surprise. Only fires on a genuine pending change; the dry-run telemetry path runs post-deploy where config already matches the state, so it stays quiet. Co-authored-by: Isaac <no-reply@databricks.com>
Co-authored-by: Isaac <no-reply@databricks.com>
A deploy that auto-migrates terraform→direct now prepares the converted state (plan-check, recreate metric, local state file) but does not commit it to the workspace until the deploy is approved: deployCore pushes it and FinalizeDeferredMigration then backs up the terraform state. A declined deploy discards the local state (DiscardDeferredMigration) and stays on terraform, and prints "Migration not committed, staying on terraform state". Destroy keeps committing the migration up front (MigrateCommit) since it has no deployCore to defer to; plan/run migrate in memory only (MigratePlan). This also moves the commit under the deploy lock. Because the state push now happens after deployCore applies changes, a push failure is a hard deploy error (self-healing on retry) rather than the old graceful terraform fallback; push-failure is rewritten accordingly. Co-authored-by: Isaac <no-reply@databricks.com>
…t path Destroy now uses the same deferred migration as deploy (MigrateDeferred): it writes the local direct state, and once the destroy is approved destroyCore runs on it and the terraform state is backed up (CleanupTerraformStateAfterMigration). A declined destroy discards the migration and stays on terraform. Backing up the local terraform.tfstate matters here: destroy removes the local resources.json, so a lingering terraform state would otherwise win the next deploy. With both deploy and destroy deferring, the early-commit path is unused, so MigrateCommit and its pushMigrationToRemote/finalizeLocalMigration/pushDirectState helpers are removed; the remaining modes are MigratePlan (plan, run) and MigrateDeferred (deploy, destroy). Co-authored-by: Isaac <no-reply@databricks.com>
This branch has not been deployed
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
2 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
Run the terraform→direct state migration before plan/deploy (after the build, so
${artifacts.*}and library paths are resolved) instead of after a terraform deploy, so the deploy itself runs on the direct engine. The migration:reconcileIDFields);deployCorepushes it and the terraform state is then backed up. A declined deploy discards it and stays on terraform (Migration not committed, staying on terraform state), so a refused recreate changes nothing. A genuine recreate is gated by the deploy's--auto-approvelike any other (direct_migrate_recreate_planned);engine: terraformstill opts out. Destroy defers the same way (its migration commits only once the destroy is approved). The terraform-engine removal is stacked on top of this PR.Tests
Acceptance tests under
acceptance/bundle/migrate/cover rename, cross-catalog move, normalization-converge, recreate-refused-without---auto-approve(then applied with it, staying on terraform when refused), and push/plan/backup failure handling, across schemas, volumes, registered models and pipelines. The rename/converge/catalog-move tests also run against a real workspace (Cloud = true, verified on aws-cli); the storage/external-location cases run on the testserver.This pull request and its description were written by Isaac.