always validate all graphs

Complete the removal of the Validate option for graph building. There is
no case where we want to allow an invalid graph, as the primary reason
for validation is to ensure we have no cycles, and we can't walk a graph
with cycles. The only code which specifically relied on there being no
validation was a test to ensure the Validate flag prevented it.

internal/terraform/context_apply.go

@@ -110,7 +110,6 @@ func (c *Context) applyGraph(plan *plans.Plan, config *configs.Config, validate
 		Plugins:            c.plugins,
 		Targets:            plan.TargetAddrs,
 		ForceReplace:       plan.ForceReplaceAddrs,
-		Validate:           validate,
 	}).Build(addrs.RootModuleInstance)
 	diags = diags.Append(moreDiags)
 	if moreDiags.HasErrors() {

internal/terraform/graph_builder.go

@@ -21,8 +21,7 @@ type GraphBuilder interface {
 // series of transforms and (optionally) validates the graph is a valid
 // structure.
 type BasicGraphBuilder struct {
-	Steps    []GraphTransformer
+	Steps []GraphTransformer
-	Validate bool
 	// Optional name to add to the graph debug log
 	Name string
 }
@@ -56,13 +55,10 @@ func (b *BasicGraphBuilder) Build(path addrs.ModuleInstance) (*Graph, tfdiags.Di
 		}
 	}
 
-	// Validate the graph structure
+	if err := g.Validate(); err != nil {
-	if b.Validate {
+		log.Printf("[ERROR] Graph validation failed. Graph:\n\n%s", g.String())
-		if err := g.Validate(); err != nil {
+		diags = diags.Append(err)
-			log.Printf("[ERROR] Graph validation failed. Graph:\n\n%s", g.String())
+		return nil, diags
-			diags = diags.Append(err)
-			return nil, diags
-		}
 	}
 
 	return g, diags

internal/terraform/graph_builder_apply.go

@@ -46,17 +46,13 @@ type ApplyGraphBuilder struct {
 	// The apply step refers to these as part of verifying that the planned
 	// actions remain consistent between plan and apply.
 	ForceReplace []addrs.AbsResourceInstance
-
-	// Validate will do structural validation of the graph.
-	Validate bool
 }
 
 // See GraphBuilder
 func (b *ApplyGraphBuilder) Build(path addrs.ModuleInstance) (*Graph, tfdiags.Diagnostics) {
 	return (&BasicGraphBuilder{
-		Steps:    b.Steps(),
+		Steps: b.Steps(),
-		Validate: b.Validate,
+		Name:  "ApplyGraphBuilder",
-		Name:     "ApplyGraphBuilder",
 	}).Build(path)
 }
 

internal/terraform/graph_builder_destroy_plan.go

@@ -35,9 +35,6 @@ type DestroyPlanGraphBuilder struct {
 	// Targets are resources to target
 	Targets []addrs.Targetable
 
-	// Validate will do structural validation of the graph.
-	Validate bool
-
 	// If set, skipRefresh will cause us stop skip refreshing any existing
 	// resource instances as part of our planning. This will cause us to fail
 	// to detect if an object has already been deleted outside of Terraform.
@@ -47,9 +44,8 @@ type DestroyPlanGraphBuilder struct {
 // See GraphBuilder
 func (b *DestroyPlanGraphBuilder) Build(path addrs.ModuleInstance) (*Graph, tfdiags.Diagnostics) {
 	return (&BasicGraphBuilder{
-		Steps:    b.Steps(),
+		Steps: b.Steps(),
-		Validate: b.Validate,
+		Name:  "DestroyPlanGraphBuilder",
-		Name:     "DestroyPlanGraphBuilder",
 	}).Build(path)
 }
 

internal/terraform/graph_builder_eval.go

@@ -43,9 +43,8 @@ type EvalGraphBuilder struct {
 // See GraphBuilder
 func (b *EvalGraphBuilder) Build(path addrs.ModuleInstance) (*Graph, tfdiags.Diagnostics) {
 	return (&BasicGraphBuilder{
-		Steps:    b.Steps(),
+		Steps: b.Steps(),
-		Validate: true,
+		Name:  "EvalGraphBuilder",
-		Name:     "EvalGraphBuilder",
 	}).Build(path)
 }
 

internal/terraform/graph_builder_import.go

@@ -30,9 +30,8 @@ type ImportGraphBuilder struct {
 // Build builds the graph according to the steps returned by Steps.
 func (b *ImportGraphBuilder) Build(path addrs.ModuleInstance) (*Graph, tfdiags.Diagnostics) {
 	return (&BasicGraphBuilder{
-		Steps:    b.Steps(),
+		Steps: b.Steps(),
-		Validate: true,
+		Name:  "ImportGraphBuilder",
-		Name:     "ImportGraphBuilder",
 	}).Build(path)
 }
 

internal/terraform/graph_builder_test.go

@@ -42,7 +42,6 @@ func TestBasicGraphBuilder_validate(t *testing.T) {
 			&testBasicGraphBuilderTransform{1},
 			&testBasicGraphBuilderTransform{2},
 		},
-		Validate: true,
 	}
 
 	_, err := b.Build(addrs.RootModuleInstance)
@@ -51,21 +50,6 @@ func TestBasicGraphBuilder_validate(t *testing.T) {
 	}
 }
 
-func TestBasicGraphBuilder_validateOff(t *testing.T) {
-	b := &BasicGraphBuilder{
-		Steps: []GraphTransformer{
-			&testBasicGraphBuilderTransform{1},
-			&testBasicGraphBuilderTransform{2},
-		},
-		Validate: false,
-	}
-
-	_, err := b.Build(addrs.RootModuleInstance)
-	if err != nil {
-		t.Fatalf("expected no error, got: %s", err)
-	}
-}
-
 type testBasicGraphBuilderTransform struct {
 	V dag.Vertex
 }

internal/terraform/node_resource_plan.go

@@ -390,9 +390,8 @@ func (n *NodePlannableResource) DynamicExpand(ctx EvalContext) (*Graph, error) {
 
 	// Build the graph
 	b := &BasicGraphBuilder{
-		Steps:    steps,
+		Steps: steps,
-		Validate: true,
+		Name:  "NodePlannableResource",
-		Name:     "NodePlannableResource",
 	}
 	graph, diags := b.Build(ctx.Path())
 	return graph, diags.ErrWithWarnings()