diff --git a/.nextchanges/bundles/snapshot-force-plan.md b/.nextchanges/bundles/snapshot-force-plan.md new file mode 100644 index 00000000000..c92b0cf788d --- /dev/null +++ b/.nextchanges/bundles/snapshot-force-plan.md @@ -0,0 +1 @@ +* `bundle deploy --plan --force` now fails instead of staying on a snapshot that was modified in the workspace after the plan was created; create a new plan with `bundle plan --force`. ([#7336](https://github.com/databricks/cli/pull/7336)) diff --git a/acceptance/bundle/deploy/immutable/breakglass/output.txt b/acceptance/bundle/deploy/immutable/breakglass/output.txt index 39f62c3b457..5646552b909 100644 --- a/acceptance/bundle/deploy/immutable/breakglass/output.txt +++ b/acceptance/bundle/deploy/immutable/breakglass/output.txt @@ -54,6 +54,29 @@ Resources: 1 created, 1 changed, 1 deleted, 0 unchanged Fourth snapshot path: /Workspace/Users/[UUID]/.snapshots/[SNAPSHOT_HASH]/[SNAPSHOT_HASH]-[UUID]/snapshot/files/src/main.py OK: the saved plan moved the deployment to a new snapshot +=== Save a plan without changes, then break the glass before deploying it +>>> [CLI] bundle plan -o json + +>>> [CLI] api post /api/2.0/snapshots:breakGlass --json {"snapshot_content_path": "/Workspace/Users/[UUID]/.snapshots/[SNAPSHOT_HASH]/[SNAPSHOT_HASH]-[UUID]/snapshot"} +true + +=== Deploying the saved plan with --force fails, since it keeps the modified snapshot +>>> musterr [CLI] bundle deploy --plan plan.json --force +Error: the last deployment was modified in the workspace since the plan was created; create a new plan with 'bundle plan --force' to move to a new snapshot + +Files: 0 uploaded, 0 deleted + +=== A plan saved with --force moves to a new snapshot +>>> [CLI] bundle plan --force -o json + +>>> [CLI] bundle deploy --plan plan.json --force +Recreated internal_immutable_snapshots.immutable +Updated jobs.my_job +Files: 0 uploaded, 0 deleted +Resources: 1 created, 1 changed, 1 deleted, 0 unchanged +Fifth snapshot path: /Workspace/Users/[UUID]/.snapshots/[SNAPSHOT_HASH]/[SNAPSHOT_HASH]-[UUID]/snapshot/files/src/main.py +OK: the plan saved with --force moved the deployment to a new snapshot + >>> [CLI] bundle destroy --auto-approve The following resources will be deleted: delete resources.jobs.my_job diff --git a/acceptance/bundle/deploy/immutable/breakglass/script b/acceptance/bundle/deploy/immutable/breakglass/script index 26768a89612..7cb00bdab00 100644 --- a/acceptance/bundle/deploy/immutable/breakglass/script +++ b/acceptance/bundle/deploy/immutable/breakglass/script @@ -60,3 +60,22 @@ if [ "$SNAPSHOT_PATH_3" = "$SNAPSHOT_PATH_4" ]; then exit 1 fi echo "OK: the saved plan moved the deployment to a new snapshot" + +title "Save a plan without changes, then break the glass before deploying it" +trace $CLI bundle plan -o json > plan.json +CONTENT_PATH_4="${SNAPSHOT_PATH_4%/files/src/main.py}" +trace $CLI api post /api/2.0/snapshots:breakGlass --json "{\"snapshot_content_path\": \"$CONTENT_PATH_4\"}" | jq .status.dirty + +title "Deploying the saved plan with --force fails, since it keeps the modified snapshot" +trace musterr $CLI bundle deploy --plan plan.json --force + +title "A plan saved with --force moves to a new snapshot" +trace $CLI bundle plan --force -o json > plan.json +trace $CLI bundle deploy --plan plan.json --force +SNAPSHOT_PATH_5=$($CLI jobs get "$JOB_ID" | jq -r '.settings.tasks[0].spark_python_task.python_file') +echo "Fifth snapshot path: $SNAPSHOT_PATH_5" +if [ "$SNAPSHOT_PATH_4" = "$SNAPSHOT_PATH_5" ]; then + echo "ERROR: snapshot path did not change after deploying the plan saved with --force" + exit 1 +fi +echo "OK: the plan saved with --force moved the deployment to a new snapshot" diff --git a/bundle/deploy/snapshot/upload.go b/bundle/deploy/snapshot/upload.go index cdfb6d538d3..fcd1c49ba97 100644 --- a/bundle/deploy/snapshot/upload.go +++ b/bundle/deploy/snapshot/upload.go @@ -82,7 +82,7 @@ func (m *snapshotUpload) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagn // Check the previous snapshot before doing any work, so a deploy onto a snapshot that was // modified outside the bundle fails before building a zip. A saved plan is checked as well: // the snapshot may have been modified after the plan was produced. - prev, err := checkPrevSnapshot(ctx, b, uploader, remoteRoot) + prev, modified, err := checkPrevSnapshot(ctx, b, uploader, remoteRoot) if err != nil { return diag.FromErr(err) } @@ -90,6 +90,10 @@ func (m *snapshotUpload) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagn var zipPath string var diags diag.Diagnostics if m.clean { + // --force moves the deployment off a modified snapshot, so don't let it reuse that name. + if modified { + prev = nil + } zipPath, diags, err = stageZip(ctx, b, snap, prev) } else { zipPath, err = stagedZipPath(ctx, b) @@ -99,6 +103,11 @@ func (m *snapshotUpload) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagn } snap.ZipPath = filepath.ToSlash(zipPath) + // A plan produced while the snapshot was untouched may keep it in place. Applying it would + // leave the deployment on the modified snapshot, which --force promises to move off. + if !m.clean && modified && snap.RelativePath() == prev.RelativePath { + return diag.Errorf("the last deployment was modified in the workspace since the plan was created; create a new plan with 'bundle plan --force' to move to a new snapshot") + } b.Config.Resources.Snapshots[resources.SnapshotResourceKey] = snap return diags @@ -231,10 +240,11 @@ func BuildCanManage(b *bundle.Bundle) []snapshot.ManagePrincipal { } // checkPrevSnapshot asks the backend whether the snapshot deployed last time was modified outside -// of the bundle, and returns its state when the deployment may keep using it: +// of the bundle, and returns its state: // - the previous state while the snapshot is untouched; -// - nil when there is no previous snapshot, or it was modified and --force was given, which -// moves the deployment to a new snapshot; +// - nil when there is no previous snapshot; +// - the previous state and modified=true when it was modified and --force was given; the +// caller must then not keep the deployment on it; // - an error when it was modified and --force was not given. // // The check has to happen here rather than in the resource's DoRead: it decides the snapshot's @@ -242,32 +252,32 @@ func BuildCanManage(b *bundle.Bundle) []snapshot.ManagePrincipal { // before any resource is read (see LookupReferencePreDeploy). Erroring unless --force also has no // equivalent in the resource layer. CheckDashboardsModifiedRemotely is the same shape: read state, // ask the API, refuse unless forced. -func checkPrevSnapshot(ctx context.Context, b *bundle.Bundle, uploader *snapshot.SnapshotClient, remoteRoot string) (*dresources.SnapshotState, error) { +func checkPrevSnapshot(ctx context.Context, b *bundle.Bundle, uploader *snapshot.SnapshotClient, remoteRoot string) (prev *dresources.SnapshotState, modified bool, err error) { // The deploy/plan pipeline opens the state DB for read before this runs, but callers that // exercise PlanUpload in isolation (unit tests) may not. No open state means no previous // snapshot to check, which is the first-deploy case. if !b.DeploymentBundle.StateDB.IsOpen() { - return nil, nil + return nil, false, nil } entry, ok := b.DeploymentBundle.StateDB.GetResourceEntry(resources.SnapshotKey) if !ok || len(entry.State) == 0 { - return nil, nil + return nil, false, nil } // Decode into the state type the engine persists, so the field names cannot drift apart. - var prev dresources.SnapshotState - if err := json.Unmarshal(entry.State, &prev); err != nil { - return nil, fmt.Errorf("reading previous snapshot state: %w", err) + var state dresources.SnapshotState + if err := json.Unmarshal(entry.State, &state); err != nil { + return nil, false, fmt.Errorf("reading previous snapshot state: %w", err) } - if prev.RelativePath == "" { - return nil, nil + if state.RelativePath == "" { + return nil, false, nil } // Compose the content path from relative_path rather than reading full_path: relative_path // means the same thing in every CLI version, while full_path from a CLI that predates the // content subfolder points one level too high. - contentPath := path.Join(remoteRoot, prev.RelativePath, snapshot.ContentSubdir) + contentPath := path.Join(remoteRoot, state.RelativePath, snapshot.ContentSubdir) status, err := uploader.InspectSnapshot(ctx, contentPath) // Nothing to protect when no snapshot is there: the deployment predates the content @@ -275,16 +285,16 @@ func checkPrevSnapshot(ctx context.Context, b *bundle.Bundle, uploader *snapshot // Failing here would also be a dead end, since this runs before the --force check and so // could not be forced past. if errors.Is(err, apierr.ErrNotFound) { - return nil, nil + return nil, false, nil } if err != nil { - return nil, err + return nil, false, err } if !status.Dirty { - return &prev, nil + return &state, false, nil } if !b.Config.Bundle.Force { - return nil, errors.New("the last deployment was modified in the workspace. Changes might be overwritten by this deployment. To confirm it, use the --force flag") + return nil, false, errors.New("the last deployment was modified in the workspace. Changes might be overwritten by this deployment. To confirm it, use the --force flag") } - return nil, nil + return &state, true, nil }