Refuse to apply a plan that keeps a modified snapshot under --force

.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))

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

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"

bundle/deploy/snapshot/upload.go

@@ -82,14 +82,18 @@ 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)
 	}
 
 	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,60 +240,61 @@ 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
 // path, and resources referencing ${...full_path} resolve it from the desired state computed
 // 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
 	// subfolder, or the snapshot was deleted out of band, or an earlier deploy never created it.
 	// 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
 }