rely solely on the plan changes for outputs

Now that outputs changes are tracked in full, we can remove the
comparisons with the prior state and use the planned changes directly.

backend/local/backend_apply.go

@@ -66,16 +66,6 @@ func (b *Local) opApply(
 		}
 	}()
 
-	// Before we do anything else we'll take a snapshot of the prior state
-	// so we can use it for some fixups to our detection of whether the plan
-	// includes externally-visible side-effects that need to be applied.
-	// (We should be able to remove this once we complete the planned work
-	// described in the comment for func planHasSideEffects in backend_plan.go .)
-	// We go directly to the state manager here because the state inside
-	// tfCtx was already implicitly changed by a validation walk inside
-	// the b.context method.
-	priorState := opState.State().DeepCopy()
-
 	runningOp.State = tfCtx.State()
 
 	// If we weren't given a plan, then we refresh/plan
@@ -89,7 +79,7 @@ func (b *Local) opApply(
 			return
 		}
 
-		trivialPlan := !planHasSideEffects(priorState, plan.Changes)
+		trivialPlan := plan.Changes.Empty()
 		hasUI := op.UIOut != nil && op.UIIn != nil
 		mustConfirm := hasUI && ((op.Destroy && (!op.DestroyForce && !op.AutoApprove)) || (!op.Destroy && !op.AutoApprove && !trivialPlan))
 		if mustConfirm {
@@ -114,7 +104,7 @@ func (b *Local) opApply(
 
 			if !trivialPlan {
 				// Display the plan of what we are going to apply/destroy.
-				b.renderPlan(plan, runningOp.State, priorState, tfCtx.Schemas())
+				b.renderPlan(plan, runningOp.State, tfCtx.Schemas())
 				b.CLI.Output("")
 			}
 

backend/local/backend_plan.go

@@ -10,13 +10,11 @@ import (
 
 	"github.com/mitchellh/cli"
 	"github.com/mitchellh/colorstring"
-	"github.com/zclconf/go-cty/cty"
 
 	"github.com/hashicorp/terraform/addrs"
 	"github.com/hashicorp/terraform/backend"
 	"github.com/hashicorp/terraform/command/format"
 	"github.com/hashicorp/terraform/plans"
-	"github.com/hashicorp/terraform/plans/objchange"
 	"github.com/hashicorp/terraform/plans/planfile"
 	"github.com/hashicorp/terraform/states"
 	"github.com/hashicorp/terraform/states/statemgr"
@@ -85,16 +83,6 @@ func (b *Local) opPlan(
 		}
 	}()
 
-	// Before we do anything else we'll take a snapshot of the prior state
-	// so we can use it for some fixups to our detection of whether the plan
-	// includes externally-visible side-effects that need to be applied.
-	// (We should be able to remove this once we complete the planned work
-	// described in the comment for func planHasSideEffects below.)
-	// We go directly to the state manager here because the state inside
-	// tfCtx was already implicitly changed by a validation walk inside
-	// the b.context method.
-	priorState := opState.State().DeepCopy()
-
 	runningOp.State = tfCtx.State()
 
 	// Perform the plan in a goroutine so we can be interrupted
@@ -123,7 +111,7 @@ func (b *Local) opPlan(
 	}
 
 	// Record whether this plan includes any side-effects that could be applied.
-	runningOp.PlanEmpty = !planHasSideEffects(priorState, plan.Changes)
+	runningOp.PlanEmpty = plan.Changes.Empty()
 
 	// Save the plan to disk
 	if path := op.PlanOutPath; path != "" {
@@ -167,7 +155,7 @@ func (b *Local) opPlan(
 			return
 		}
 
-		b.renderPlan(plan, plan.State, priorState, schemas)
+		b.renderPlan(plan, plan.State, schemas)
 
 		// If we've accumulated any warnings along the way then we'll show them
 		// here just before we show the summary and next steps. If we encountered
@@ -194,8 +182,8 @@ func (b *Local) opPlan(
 	}
 }
 
-func (b *Local) renderPlan(plan *plans.Plan, baseState *states.State, priorState *states.State, schemas *terraform.Schemas) {
+func (b *Local) renderPlan(plan *plans.Plan, baseState *states.State, schemas *terraform.Schemas) {
-	RenderPlan(plan, baseState, priorState, schemas, b.CLI, b.Colorize())
+	RenderPlan(plan, baseState, schemas, b.CLI, b.Colorize())
 }
 
 // RenderPlan renders the given plan to the given UI.
@@ -218,7 +206,7 @@ func (b *Local) renderPlan(plan *plans.Plan, baseState *states.State, priorState
 // output values will not currently be rendered because their prior values
 // are currently stored only in the prior state. (see the docstring for
 // func planHasSideEffects for why this is and when that might change)
-func RenderPlan(plan *plans.Plan, baseState *states.State, priorState *states.State, schemas *terraform.Schemas, ui cli.Ui, colorize *colorstring.Colorize) {
+func RenderPlan(plan *plans.Plan, baseState *states.State, schemas *terraform.Schemas, ui cli.Ui, colorize *colorstring.Colorize) {
 	counts := map[plans.Action]int{}
 	var rChanges []*plans.ResourceInstanceChangeSrc
 	for _, change := range plan.Changes.Resources {
@@ -325,159 +313,12 @@ func RenderPlan(plan *plans.Plan, baseState *states.State, priorState *states.St
 	)))
 
 	// If there is at least one planned change to the root module outputs
-	// then we'll render a summary of those too. This is easier said than done
+	// then we'll render a summary of those too.
-	// because currently output changes are not accurately recorded in
+	if len(plan.Changes.Outputs) > 0 {
-	// plan.Changes.Outputs (see the func planHasSideEffects docstring for why)
+		ui.Output(colorize.Color("[reset]\n[bold]Changes to Outputs:[reset]" + format.OutputChanges(plan.Changes.Outputs, colorize)))
-	// and so we must use priorState to produce an actually-accurate changeset
-	// to display.
-	//
-	// Some callers (i.e. "terraform show") only have the plan and therefore
-	// can't provide the prior state. In that case we'll skip showing the
-	// outputs for now, until we can make plan.Changes.Outputs itself be
-	// accurate and self-contained.
-	if priorState != nil {
-		var synthOutputChanges []*plans.OutputChangeSrc
-		outputChangeCount := 0
-		for _, addr := range allRootModuleOutputs(priorState, plan.Changes) {
-			before := cty.NullVal(cty.DynamicPseudoType)
-			after := cty.NullVal(cty.DynamicPseudoType)
-			sensitive := false
-			if changeSrc := plan.Changes.OutputValue(addr); changeSrc != nil {
-				sensitive = sensitive || changeSrc.Sensitive
-				change, err := changeSrc.Decode()
-				if err != nil {
-					// It would be very strange to get here because changeSrc was
-					// presumably just created by Terraform Core and so should never
-					// be invalid.
-					panic(fmt.Sprintf("failed to decode change for %s: %s", addr, err))
-				}
-				after = change.After
-			}
-			if priorOutputState := priorState.OutputValue(addr); priorOutputState != nil {
-				sensitive = sensitive || priorOutputState.Sensitive
-				before = priorOutputState.Value
-			}
-
-			// We'll now construct ourselves a new, accurate change.
-			change := &plans.OutputChange{
-				Addr:      addr,
-				Sensitive: sensitive,
-				Change: plans.Change{
-					Action: objchange.ActionForChange(before, after),
-					Before: before,
-					After:  after,
-				},
-			}
-			if change.Action == plans.NoOp {
-				continue // ignore non-changes
-			}
-			outputChangeCount++
-			newChangeSrc, err := change.Encode()
-			if err != nil {
-				// Again, it would be very strange to see an error here because
-				// we've literally just created this value in memory above.
-				panic(fmt.Sprintf("failed to encode change for %s: %s", addr, err))
-			}
-			synthOutputChanges = append(synthOutputChanges, newChangeSrc)
-		}
-		if outputChangeCount > 0 {
-			ui.Output(colorize.Color("[reset]\n[bold]Changes to Outputs:[reset]" + format.OutputChanges(synthOutputChanges, colorize)))
-		}
 	}
 }
 
-// planHasSideEffects determines whether the given planned changeset has
-// externally-visible side-effects that warrant giving the user an opportunity
-// to apply the plan. If planHasSideEffects returns false, the caller should
-// return a "No changes" message and not offer to apply the plan.
-//
-// This is currently implemented here, rather than in the "terraform" package,
-// because with the current separation of the refresh vs. plan walks there is
-// never any single point in the "terraform" package where both the prior and
-// planned new values for outputs are available at once. We have this out here
-// as a temporary workaround for that design problem, with the intent of moving
-// this down into the "terraform" package once we've completed some work to
-// combine the refresh and plan walks together into a single walk and thus
-// that walk will be able to see both the prior and new values for outputs.
-func planHasSideEffects(priorState *states.State, changes *plans.Changes) bool {
-	if !changes.Empty() {
-		// At the time of writing, changes.Empty considers only resource
-		// changes because the planned changes for outputs are inaccurate.
-		// If we have at least one resource change then we know we have
-		// side-effects though, regardless of outputs.
-		return true
-	}
-
-	// If we get here then there are definitely no resource changes in the plan
-	// but we may have some changes to outputs that "changes" hasn't properly
-	// captured, because it treats all outputs as being either created or
-	// deleted regardless of their prior values. To work around that for now,
-	// we'll use priorState to see if those planned changes really are changes.
-	for _, addr := range allRootModuleOutputs(priorState, changes) {
-		before := cty.NullVal(cty.DynamicPseudoType)
-		after := cty.NullVal(cty.DynamicPseudoType)
-		if changeSrc := changes.OutputValue(addr); changeSrc != nil {
-			change, err := changeSrc.Decode()
-			if err != nil {
-				// It would be very strange to get here because changeSrc was
-				// presumably just created by Terraform Core and so should never
-				// be invalid. In this unlikely event, we'll just conservatively
-				// assume there is a change.
-				return true
-			}
-			after = change.After
-		}
-		if priorState != nil {
-			if priorOutputState := priorState.OutputValue(addr); priorOutputState != nil {
-				before = priorOutputState.Value
-			}
-		}
-		if objchange.ActionForChange(before, after) != plans.NoOp {
-			return true
-		}
-	}
-
-	// If we fall out here then we didn't find any effective changes in the
-	// outputs, and we already showed that there were no resource changes, so
-	// this plan has no side-effects.
-	return false
-}
-
-// allRootModuleOutputs is a helper function to produce the union of all
-// root module output values across both the given prior state and the given
-// changeset. This is to compensate for the fact that the outputs portion of
-// a plans.Changes is currently incomplete and inaccurate due to limitations of
-// Terraform Core's design; we need to use information from the prior state
-// to compensate for those limitations when making decisions based on the
-// effective output changes.
-func allRootModuleOutputs(priorState *states.State, changes *plans.Changes) []addrs.AbsOutputValue {
-	m := make(map[string]addrs.AbsOutputValue)
-	if priorState != nil {
-		for _, os := range priorState.RootModule().OutputValues {
-			m[os.Addr.String()] = os.Addr
-		}
-	}
-	if changes != nil {
-		for _, oc := range changes.Outputs {
-			if !oc.Addr.Module.IsRoot() {
-				continue
-			}
-			m[oc.Addr.String()] = oc.Addr
-		}
-	}
-	if len(m) == 0 {
-		return nil
-	}
-	ret := make([]addrs.AbsOutputValue, 0, len(m))
-	for _, addr := range m {
-		ret = append(ret, addr)
-	}
-	sort.Slice(ret, func(i, j int) bool {
-		return ret[i].OutputValue.Name < ret[j].OutputValue.Name
-	})
-	return ret
-}
-
 const planHeaderIntro = `
 An execution plan has been generated and is shown below.
 Resource actions are indicated with the following symbols:

command/show.go

@@ -163,10 +163,7 @@ func (c *ShowCommand) Run(args []string) int {
 		// package rather than in the backends themselves, but for now we're
 		// accepting this oddity because "terraform show" is a less commonly
 		// used way to render a plan than "terraform plan" is.
-		// We're setting priorState to null because a saved plan file only
+		localBackend.RenderPlan(plan, stateFile.State, schemas, c.Ui, c.Colorize())
-		// records the base state (possibly updated by refresh), not the
-		// prior state (direct result of the previous apply).
-		localBackend.RenderPlan(plan, stateFile.State, nil, schemas, c.Ui, c.Colorize())
 		return 0
 	}
 

plans/changes.go

@@ -37,6 +37,13 @@ func (c *Changes) Empty() bool {
 			return false
 		}
 	}
+
+	for _, out := range c.Outputs {
+		if out.Action != NoOp {
+			return false
+		}
+	}
+
 	return true
 }