From 7946e4a88a065153884718a622f92a8e2651942b Mon Sep 17 00:00:00 2001
From: James Bardin <j.bardin@gmail.com>
Date: Fri, 18 Nov 2022 08:48:15 -0500
Subject: [PATCH] a deleted orphan should have no plan

If when refreshing an orphaned instance the provider indicates it has
already been deleted, there is no reason to create a change for that
instance. A NoOp change should only represent an object that exists and
is not changing.

This was likely left in before in order to try and provide a record of
the change for external consumers of the plan, but newer plans also
contain all changes made outside of Terraform which better accounts for
the difference. The NoOp change now can cause problems, because it may
represent an instance with conditions to check even though that instance
does not exist.
---
 .../terraform/node_resource_plan_orphan.go    | 50 +++++++++----------
 .../node_resource_plan_orphan_test.go         |  9 +---
 2 files changed, 27 insertions(+), 32 deletions(-)

diff --git a/internal/terraform/node_resource_plan_orphan.go b/internal/terraform/node_resource_plan_orphan.go
index 6002a2f25d74..aab78696fa29 100644
--- 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
index f46c7a7091c1..deed93bb9b6c 100644
--- 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)
 	}
-
 }