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 1/2] 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{

From 191124e9c9482bd99fbe229f7bc2fb67c3a6178a Mon Sep 17 00:00:00 2001
From: James Bardin <j.bardin@gmail.com>
Date: Thu, 14 Jan 2021 09:24:47 -0500
Subject: [PATCH 2/2] detect new outputs and plans as Create actions

Rather than using a prior value of null to indicate create, which is
imprecise because null is a valid output value, only plan values that
didn't exist in the prior state as Create changes.
---
 terraform/context_plan_test.go | 26 ++++++++++++++++++++++++++
 terraform/node_output.go       | 10 ++++++++--
 2 files changed, 34 insertions(+), 2 deletions(-)

diff --git a/terraform/context_plan_test.go b/terraform/context_plan_test.go
index f4dede18a..cab642b28 100644
--- a/terraform/context_plan_test.go
+++ b/terraform/context_plan_test.go
@@ -6714,3 +6714,29 @@ output "planned" {
 		}
 	}
 }
+
+func TestContext2Plan_createOutput(t *testing.T) {
+	// this should always plan a NoOp change for the output
+	m := testModuleInline(t, map[string]string{
+		"main.tf": `
+output "planned" {
+  value = 1
+}
+`,
+	})
+
+	ctx := testContext2(t, &ContextOpts{
+		Config: m,
+		State:  states.NewState(),
+	})
+	plan, diags := ctx.Plan()
+	if diags.HasErrors() {
+		t.Fatal(diags.Err())
+	}
+
+	for _, c := range plan.Changes.Outputs {
+		if c.Action != plans.Create {
+			t.Fatalf("expected Create change, got %s for %q", c.Action, c.Addr)
+		}
+	}
+}
diff --git a/terraform/node_output.go b/terraform/node_output.go
index d928a796b..c70c76e9c 100644
--- a/terraform/node_output.go
+++ b/terraform/node_output.go
@@ -432,12 +432,17 @@ func (n *NodeApplyableOutput) setValue(state *states.SyncState, changes *plans.C
 		// the diff
 		sensitiveBefore := false
 		before := cty.NullVal(cty.DynamicPseudoType)
+
+		// is this output new to our state?
+		newOutput := true
+
 		mod := state.Module(n.Addr.Module)
 		if n.Addr.Module.IsRoot() && mod != nil {
 			for name, o := range mod.OutputValues {
 				if name == n.Addr.OutputValue.Name {
 					before = o.Value
 					sensitiveBefore = o.Sensitive
+					newOutput = false
 					break
 				}
 			}
@@ -455,10 +460,11 @@ func (n *NodeApplyableOutput) setValue(state *states.SyncState, changes *plans.C
 		switch {
 		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.
+			// sensitivity here when there are only null values.
 			action = plans.NoOp
 
-		case before.IsNull():
+		case newOutput:
+			// This output was just added to the configuration
 			action = plans.Create
 
 		case val.IsWhollyKnown() &&