Merge pull request #32246 from hashicorp/jbardin/plan-orphan-deleted

A deleted orphan should have no planned change
hashicorp · Nov 22, 2022 · c96da72 · c96da72
2 parents 79175b2 + 7946e4a
commit c96da72
Show file tree

Hide file tree

Showing 2 changed files with 27 additions and 32 deletions.
diff --git a/internal/terraform/node_resource_plan_orphan.go b/internal/terraform/node_resource_plan_orphan.go
@@ -121,37 +121,37 @@ func (n *NodePlannableResourceInstanceOrphan) managedResourceExecute(ctx EvalCon
 		oldState = refreshedState
 	}
 
-	if !n.skipPlanChanges {
-		var change *plans.ResourceInstanceChange
-		change, destroyPlanDiags := n.planDestroy(ctx, oldState, "")
-		diags = diags.Append(destroyPlanDiags)
-		if diags.HasErrors() {
-			return diags
-		}
+	// If we're skipping planning, all we need to do is write the state. If the
+	// refresh indicates the instance no longer exists, there is also nothing
+	// to plan because there is no longer any state and it doesn't exist in the
+	// config.
+	if n.skipPlanChanges || oldState.Value.IsNull() {
+		return diags.Append(n.writeResourceInstanceState(ctx, oldState, workingState))
+	}
 
-		diags = diags.Append(n.checkPreventDestroy(change))
-		if diags.HasErrors() {
-			return diags
-		}
+	var change *plans.ResourceInstanceChange
+	change, destroyPlanDiags := n.planDestroy(ctx, oldState, "")
+	diags = diags.Append(destroyPlanDiags)
+	if diags.HasErrors() {
+		return diags
+	}
 
-		// We might be able to offer an approximate reason for why we are
-		// planning to delete this object. (This is best-effort; we might
-		// sometimes not have a reason.)
-		change.ActionReason = n.deleteActionReason(ctx)
+	diags = diags.Append(n.checkPreventDestroy(change))
+	if diags.HasErrors() {
+		return diags
+	}
 
-		diags = diags.Append(n.writeChange(ctx, change, ""))
-		if diags.HasErrors() {
-			return diags
-		}
+	// We might be able to offer an approximate reason for why we are
+	// planning to delete this object. (This is best-effort; we might
+	// sometimes not have a reason.)
+	change.ActionReason = n.deleteActionReason(ctx)
 
-		diags = diags.Append(n.writeResourceInstanceState(ctx, nil, workingState))
-	} else {
-		// The working state should at least be updated with the result
-		// of upgrading and refreshing from above.
-		diags = diags.Append(n.writeResourceInstanceState(ctx, oldState, workingState))
+	diags = diags.Append(n.writeChange(ctx, change, ""))
+	if diags.HasErrors() {
+		return diags
 	}
 
-	return diags
+	return diags.Append(n.writeResourceInstanceState(ctx, nil, workingState))
 }
 
 func (n *NodePlannableResourceInstanceOrphan) deleteActionReason(ctx EvalContext) plans.ResourceInstanceChangeActionReason {

diff --git a/internal/terraform/node_resource_plan_orphan_test.go b/internal/terraform/node_resource_plan_orphan_test.go
@@ -137,12 +137,7 @@ func TestNodeResourcePlanOrphanExecute_alreadyDeleted(t *testing.T) {
 	if got := refreshState.ResourceInstance(addr); got != nil {
 		t.Errorf("refresh state has entry for %s; should've been removed", addr)
 	}
-	if got := changes.ResourceInstance(addr); got == nil {
-		t.Errorf("no entry for %s in the planned changes; should have a NoOp change", addr)
-	} else {
-		if got, want := got.Action, plans.NoOp; got != want {
-			t.Errorf("planned change for %s has wrong action\ngot:  %s\nwant: %s", addr, got, want)
-		}
+	if got := changes.ResourceInstance(addr); got != nil {
+		t.Errorf("there should be no change for the %s instance, got %s", addr, got.Action)
 	}
-
 }