remove the need for destroyRootOutputTransformer

Since root outputs can now use the planned changes, we can directly
insert the correct applyable or destroyable node into the graph during
plan and apply, and it will remove the outputs if they are being
destroyed.

terraform/graph_builder_apply.go

@@ -150,17 +150,6 @@ func (b *ApplyGraphBuilder) Steps() []GraphTransformer {
 			Schemas: b.Schemas,
 		},
 
-		// Create a destroy node for root outputs to remove them from the
-		// state.  This does nothing unless invoked via the destroy command
-		// directly.  A destroy is identical to a normal apply, except for the
-		// fact that we also have configuration to evaluate. While the rest of
-		// the unused nodes can be programmatically pruned (via
-		// pruneUnusedNodesTransformer), root module outputs always have an
-		// implied dependency on remote state. This means that if they exist in
-		// the configuration, the only signal to remove them is via the destroy
-		// command itself.
-		&destroyRootOutputTransformer{Destroy: b.Destroy},
-
 		// We need to remove configuration nodes that are not used at all, as
 		// they may not be able to evaluate, especially during destroy.
 		// These include variables, locals, and instance expanders.

terraform/graph_builder_plan_test.go

@@ -288,10 +288,10 @@ local.instance_id (expand)
   aws_instance.web (expand)
 meta.count-boundary (EachMode fixup)
   aws_load_balancer.weblb (expand)
-  output.instance_id (expand)
+  output.instance_id
 openstack_floating_ip.random (expand)
   provider["registry.terraform.io/hashicorp/openstack"]
-output.instance_id (expand)
+output.instance_id
   local.instance_id (expand)
 provider["registry.terraform.io/hashicorp/aws"]
   openstack_floating_ip.random (expand)

terraform/node_output.go

@@ -15,8 +15,8 @@ import (
 	"github.com/zclconf/go-cty/cty"
 )
 
-// nodeExpandOutput is the placeholder for an output that has not yet had
-// its module path expanded.
+// nodeExpandOutput is the placeholder for a non-root module output that has
+// not yet had its module path expanded.
 type nodeExpandOutput struct {
 	Addr    addrs.OutputValue
 	Module  addrs.Module
@@ -37,17 +37,21 @@ var (
 func (n *nodeExpandOutput) expandsInstances() {}
 
 func (n *nodeExpandOutput) temporaryValue() bool {
-	// this must always be evaluated if it is a root module output
+	// non root outputs are temporary
 	return !n.Module.IsRoot()
 }
 
 func (n *nodeExpandOutput) DynamicExpand(ctx EvalContext) (*Graph, error) {
 	if n.Destroy {
-		return n.planDestroyOutputs(ctx)
+		// if we're planning a destroy, we only need to handle the root outputs.
+		// The destroy plan doesn't evaluate any other config, so we can skip
+		// the rest of the outputs.
+		return n.planDestroyRootOutput(ctx)
 	}
 
-	var g Graph
 	expander := ctx.InstanceExpander()
+
+	var g Graph
 	for _, module := range expander.ExpandModule(n.Module) {
 		absAddr := n.Addr.Absolute(module)
 
@@ -71,32 +75,23 @@ func (n *nodeExpandOutput) DynamicExpand(ctx EvalContext) (*Graph, error) {
 	return &g, nil
 }
 
-// if we're planing a destroy operation, add destroy nodes for all root outputs
-// in the state.
-func (n *nodeExpandOutput) planDestroyOutputs(ctx EvalContext) (*Graph, error) {
-	// we only need to plan destroying root outputs
-	// Other module outputs may be used during destruction by providers that
-	// need to interpolate values.
+// if we're planing a destroy operation, add a destroy node for any root output
+func (n *nodeExpandOutput) planDestroyRootOutput(ctx EvalContext) (*Graph, error) {
 	if !n.Module.IsRoot() {
 		return nil, nil
 	}
-
 	state := ctx.State()
 	if state == nil {
 		return nil, nil
 	}
 
-	ms := state.Module(addrs.RootModuleInstance)
-
 	var g Graph
-	for _, output := range ms.OutputValues {
-		o := &NodeDestroyableOutput{
-			Addr:   output.Addr,
-			Config: n.Config,
-		}
-		log.Printf("[TRACE] Expanding output: adding %s as %T", o.Addr.String(), o)
-		g.Add(o)
+	o := &NodeDestroyableOutput{
+		Addr:   n.Addr.Absolute(addrs.RootModuleInstance),
+		Config: n.Config,
 	}
+	log.Printf("[TRACE] Expanding output: adding %s as %T", o.Addr.String(), o)
+	g.Add(o)
 
 	return &g, nil
 }
@@ -149,6 +144,8 @@ func (n *nodeExpandOutput) ReferenceOutside() (selfPath, referencePath addrs.Mod
 
 // GraphNodeReferencer
 func (n *nodeExpandOutput) References() []*addrs.Reference {
+	// root outputs might be destroyable, and may not reference anything in
+	// that case
 	return referencesForOutput(n.Config)
 }
 
@@ -364,8 +361,6 @@ func (n *NodeDestroyableOutput) Execute(ctx EvalContext, op walkOperation) error
 		change := &plans.OutputChange{
 			Addr: n.Addr,
 			Change: plans.Change{
-				// This is just a weird placeholder delete action since
-				// we don't have an actual prior value to indicate.
 				// FIXME: Generate real planned changes for output values
 				// that include the old values.
 				Action: plans.Delete,
@@ -379,7 +374,7 @@ func (n *NodeDestroyableOutput) Execute(ctx EvalContext, op walkOperation) error
 			// Should never happen, since we just constructed this right above
 			panic(fmt.Sprintf("planned change for %s could not be encoded: %s", n.Addr, err))
 		}
-		log.Printf("[TRACE] planDestroyOutput: Saving %s change for %s in changeset", change.Action, n.Addr)
+		log.Printf("[TRACE] NodeDestroyableOutput: Saving %s change for %s in changeset", change.Action, n.Addr)
 		changes.RemoveOutputChange(n.Addr) // remove any existing planned change, if present
 		changes.AppendOutputChange(cs)     // add the new planned change
 	}

terraform/node_resource_apply.go

@@ -26,7 +26,8 @@ var (
 	_ GraphNodeTargetable           = (*nodeExpandApplyableResource)(nil)
 )
 
-func (n *nodeExpandApplyableResource) expandsInstances() {}
+func (n *nodeExpandApplyableResource) expandsInstances() {
+}
 
 func (n *nodeExpandApplyableResource) References() []*addrs.Reference {
 	return (&NodeApplyableResource{NodeAbstractResource: n.NodeAbstractResource}).References()

terraform/transform_output.go

@@ -43,73 +43,67 @@ func (t *OutputTransformer) transform(g *Graph, c *configs.Config) error {
 		}
 	}
 
-	// Add plannable outputs to the graph, which will be dynamically expanded
+	// Add outputs to the graph, which will be dynamically expanded
 	// into NodeApplyableOutputs to reflect possible expansion
 	// through the presence of "count" or "for_each" on the modules.
 
+	// if this is a root output, we add the apply or destroy node directly, as
+	// the root modules does not expand
 	var changes []*plans.OutputChangeSrc
 	if t.Changes != nil {
 		changes = t.Changes.Outputs
 	}
 
 	for _, o := range c.Module.Outputs {
-		node := &nodeExpandOutput{
-			Addr:    addrs.OutputValue{Name: o.Name},
-			Module:  c.Path,
-			Config:  o,
-			Changes: changes,
-			Destroy: t.Destroy,
+		addr := addrs.OutputValue{Name: o.Name}
+
+		var rootChange *plans.OutputChangeSrc
+		for _, c := range changes {
+			if c.Addr.Module.IsRoot() && c.Addr.OutputValue.Name == o.Name {
+				rootChange = c
+			}
 		}
+
+		var node dag.Vertex
+		switch {
+		case c.Path.IsRoot() && t.Destroy:
+			node = &NodeDestroyableOutput{
+				Addr:   addr.Absolute(addrs.RootModuleInstance),
+				Config: o,
+			}
+		case c.Path.IsRoot():
+			destroy := t.Destroy
+			if rootChange != nil {
+				destroy = rootChange.Action == plans.Delete
+			}
+
+			switch {
+			case destroy:
+				node = &NodeDestroyableOutput{
+					Addr:   addr.Absolute(addrs.RootModuleInstance),
+					Config: o,
+				}
+			default:
+				node = &NodeApplyableOutput{
+					Addr:   addr.Absolute(addrs.RootModuleInstance),
+					Config: o,
+					Change: rootChange,
+				}
+			}
+
+		default:
+			node = &nodeExpandOutput{
+				Addr:    addr,
+				Module:  c.Path,
+				Config:  o,
+				Changes: changes,
+				Destroy: t.Destroy,
+			}
+		}
+
 		log.Printf("[TRACE] OutputTransformer: adding %s as %T", o.Name, node)
 		g.Add(node)
 	}
 
 	return nil
 }
-
-// destroyRootOutputTransformer is a GraphTransformer that adds nodes to delete
-// outputs during destroy. We need to do this to ensure that no stale outputs
-// are ever left in the state.
-type destroyRootOutputTransformer struct {
-	Destroy bool
-}
-
-func (t *destroyRootOutputTransformer) Transform(g *Graph) error {
-	// Only clean root outputs on a full destroy
-	if !t.Destroy {
-		return nil
-	}
-
-	for _, v := range g.Vertices() {
-		output, ok := v.(*nodeExpandOutput)
-		if !ok {
-			continue
-		}
-
-		// We only destroy root outputs
-		if !output.Module.Equal(addrs.RootModule) {
-			continue
-		}
-
-		// create the destroy node for this output
-		node := &NodeDestroyableOutput{
-			Addr:   output.Addr.Absolute(addrs.RootModuleInstance),
-			Config: output.Config,
-		}
-
-		log.Printf("[TRACE] creating %s", node.Name())
-		g.Add(node)
-
-		deps := g.UpEdges(v)
-
-		for _, d := range deps {
-			log.Printf("[TRACE] %s depends on %s", node.Name(), dag.VertexName(d))
-			g.Connect(dag.BasicEdge(node, d))
-		}
-
-		// We no longer need the expand node, since we intend to remove this
-		// output from the state.
-		g.Remove(v)
-	}
-	return nil
-}

terraform/transform_targets_test.go

@@ -122,7 +122,7 @@ module.child.module.grandchild.output.id (expand)
   module.child.module.grandchild.aws_instance.foo
 module.child.output.grandchild_id (expand)
   module.child.module.grandchild.output.id (expand)
-output.grandchild_id (expand)
+output.grandchild_id
   module.child.output.grandchild_id (expand)
 	`)
 	if actual != expected {
@@ -193,7 +193,7 @@ module.child.module.grandchild.output.id (expand)
   module.child.module.grandchild.aws_instance.foo
 module.child.output.grandchild_id (expand)
   module.child.module.grandchild.output.id (expand)
-output.grandchild_id (expand)
+output.grandchild_id
   module.child.output.grandchild_id (expand)
 	`)
 	if actual != expected {