From f1a3129f114b46f790e72ef9d33388428977be7a 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 3d62125751..a9caa5e2a7 100644
--- a/terraform/context_plan_test.go
+++ b/terraform/context_plan_test.go
@@ -6641,3 +6641,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 00f39c80e3..d13e28b9b2 100644
--- a/terraform/node_output.go
+++ b/terraform/node_output.go
@@ -437,10 +437,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
@@ -453,9 +455,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 785d3de0f233221b5c0fd6bdb9960bf24ca32d03 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 a9caa5e2a7..129e279a3d 100644
--- a/terraform/context_plan_test.go
+++ b/terraform/context_plan_test.go
@@ -6670,3 +6670,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 d13e28b9b2..0a6c1a6156 100644
--- a/terraform/node_output.go
+++ b/terraform/node_output.go
@@ -418,12 +418,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
 				}
 			}
@@ -441,10 +446,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() &&