From 57f004e0ef6f23368cc40f57235380b41fb04aed Mon Sep 17 00:00:00 2001
From: James Bardin <j.bardin@gmail.com>
Date: Thu, 14 Jan 2021 08:59:31 -0500
Subject: [PATCH] existing outputs can only be Updated

The planning logic here was inspired by resources, but unlike resources
a null value is valid for an output and does not necessarily indicate it
is being removed. If both before and after are null, the change should
be NoOp. When an output is removed from the configuration, we have a
separate node to create a Delete action to remove it from the state.
---
 terraform/context_plan_test.go | 29 +++++++++++++++++++++++++++++
 terraform/node_output.go       | 11 +++++------
 2 files changed, 34 insertions(+), 6 deletions(-)

diff --git a/terraform/context_plan_test.go b/terraform/context_plan_test.go
index 64167c520..f4dede18a 100644
--- a/terraform/context_plan_test.go
+++ b/terraform/context_plan_test.go
@@ -6685,3 +6685,32 @@ func TestContext2Plan_variableCustomValidationsSensitive(t *testing.T) {
 		t.Fatalf("wrong error:\ngot:  %s\nwant: message containing %q", got, want)
 	}
 }
+
+func TestContext2Plan_nullOutputNoOp(t *testing.T) {
+	// this should always plan a NoOp change for the output
+	m := testModuleInline(t, map[string]string{
+		"main.tf": `
+output "planned" {
+  value = false ? 1 : null
+}
+`,
+	})
+
+	ctx := testContext2(t, &ContextOpts{
+		Config: m,
+		State: states.BuildState(func(s *states.SyncState) {
+			r := s.Module(addrs.RootModuleInstance)
+			r.SetOutputValue("planned", cty.NullVal(cty.DynamicPseudoType), false)
+		}),
+	})
+	plan, diags := ctx.Plan()
+	if diags.HasErrors() {
+		t.Fatal(diags.Err())
+	}
+
+	for _, c := range plan.Changes.Outputs {
+		if c.Action != plans.NoOp {
+			t.Fatalf("expected no changes, got %s for %q", c.Action, c.Addr)
+		}
+	}
+}
diff --git a/terraform/node_output.go b/terraform/node_output.go
index 9f64db82d..d928a796b 100644
--- a/terraform/node_output.go
+++ b/terraform/node_output.go
@@ -451,10 +451,12 @@ func (n *NodeApplyableOutput) setValue(state *states.SyncState, changes *plans.C
 		// strip any marks here just to be sure we don't panic on the True comparison
 		val, _ = val.UnmarkDeep()
 
-		var action plans.Action
+		action := plans.Update
 		switch {
-		case val.IsNull():
-			action = plans.Delete
+		case val.IsNull() && before.IsNull():
+			// This is separate from the NoOp case below, since we can ignore
+			// sensitivity here if there are only null values.
+			action = plans.NoOp
 
 		case before.IsNull():
 			action = plans.Create
@@ -467,9 +469,6 @@ func (n *NodeApplyableOutput) setValue(state *states.SyncState, changes *plans.C
 			// only one we can act on, and the state will have been loaded
 			// without any marks to consider.
 			action = plans.NoOp
-
-		default:
-			action = plans.Update
 		}
 
 		change := &plans.OutputChange{