terraform: default new graphs on, old graphs behind -Xlegacy-graph

This turns the new graphs on by default and puts the old graphs behind a
flag `-Xlegacy-graph`. This effectively inverts the current 0.7.x
behavior with the new graphs.

We've incubated most of these for a few weeks now. We've found issues
and we've fixed them and we've been using these graphs internally for
awhile without any major issue. Its time to default them on and get them
part of a beta.

.travis.yml

@@ -10,7 +10,6 @@ install:
 - bash scripts/gogetcookie.sh
 script:
 - make test vet
-- make test TEST=./terraform TESTARGS=-Xnew-apply
 branches:
   only:
   - master

command/apply.go

@@ -98,44 +98,14 @@ func (c *ApplyCommand) Run(args []string) int {
 
 	terraform.SetDebugInfo(DefaultDataDir)
 
-	// Check for the new apply
+	// Check for the legacy graph
-	if experiment.Enabled(experiment.X_newApply) && !experiment.Force() {
+	if experiment.Enabled(experiment.X_legacyGraph) {
-		desc := "Experimental new apply graph has been enabled. This may still\n" +
+		c.Ui.Output(c.Colorize().Color(
-			"have bugs, and should be used with care. If you'd like to continue,\n" +
+			"[reset][bold][yellow]" +
-			"you must enter exactly 'yes' as a response."
+				"Legacy graph enabled! This will use the graph from Terraform 0.7.x\n" +
-		v, err := c.UIInput().Input(&terraform.InputOpts{
+				"to execute this operation. This will be removed in the future so\n" +
-			Id:          "Xnew-apply",
+				"please report any issues causing you to use this to the Terraform\n" +
-			Query:       "Experimental feature enabled: new apply graph. Continue?",
+				"project.\n\n"))
-			Description: desc,
-		})
-		if err != nil {
-			c.Ui.Error(fmt.Sprintf("Error asking for confirmation: %s", err))
-			return 1
-		}
-		if v != "yes" {
-			c.Ui.Output("Apply cancelled.")
-			return 1
-		}
-	}
-
-	// Check for the new destroy
-	if experiment.Enabled(experiment.X_newDestroy) && !experiment.Force() {
-		desc := "Experimental new destroy graph has been enabled. This may still\n" +
-			"have bugs, and should be used with care. If you'd like to continue,\n" +
-			"you must enter exactly 'yes' as a response."
-		v, err := c.UIInput().Input(&terraform.InputOpts{
-			Id:          "Xnew-destroy",
-			Query:       "Experimental feature enabled: new destroy graph. Continue?",
-			Description: desc,
-		})
-		if err != nil {
-			c.Ui.Error(fmt.Sprintf("Error asking for confirmation: %s", err))
-			return 1
-		}
-		if v != "yes" {
-			c.Ui.Output("Apply cancelled.")
-			return 1
-		}
 	}
 
 	// This is going to keep track of shadow errors

command/apply_destroy_test.go

@@ -184,7 +184,7 @@ func TestApply_destroyTargeted(t *testing.T) {
 	actualStr := strings.TrimSpace(state.String())
 	expectedStr := strings.TrimSpace(testApplyDestroyStr)
 	if actualStr != expectedStr {
-		t.Fatalf("bad:\n\n%s\n\n%s", actualStr, expectedStr)
+		t.Fatalf("bad:\n\n%s\n\nexpected:\n\n%s", actualStr, expectedStr)
 	}
 
 	// Should have a backup file

helper/experiment/experiment.go

@@ -50,11 +50,8 @@ import (
 // of definition and use. This allows the compiler to enforce references
 // so it becomes easy to remove the features.
 var (
-	// New apply graph. This will be removed and be the default in 0.8.0.
+	// Reuse the old graphs from TF 0.7.x. These will be removed at some point.
-	X_newApply = newBasicID("new-apply", "NEW_APPLY", false)
+	X_legacyGraph = newBasicID("legacy-graph", "LEGACY_GRAPH", false)
-
-	// New destroy graph. This will be reomved and be the default in 0.8.0.
-	X_newDestroy = newBasicID("new-destroy", "NEW_DESTROY", false)
 
 	// Shadow graph. This is already on by default. Disabling it will be
 	// allowed for awhile in order for it to not block operations.
@@ -78,8 +75,7 @@ var (
 func init() {
 	// The list of all experiments, update this when an experiment is added.
 	All = []ID{
-		X_newApply,
+		X_legacyGraph,
-		X_newDestroy,
 		X_shadow,
 		x_force,
 	}

terraform/context.go

@@ -359,63 +359,26 @@ func (c *Context) Apply() (*State, error) {
 	// Copy our own state
 	c.state = c.state.DeepCopy()
 
-	X_newApply := experiment.Enabled(experiment.X_newApply)
+	// Enable the new graph by default
-	X_newDestroy := experiment.Enabled(experiment.X_newDestroy)
+	X_legacyGraph := experiment.Enabled(experiment.X_legacyGraph)
-	newGraphEnabled := (c.destroy && X_newDestroy) || (!c.destroy && X_newApply)
 
-	// Build the original graph. This is before the new graph builders
+	// Build the graph.
-	// coming in 0.8. We do this for shadow graphing.
+	var graph *Graph
-	oldGraph, err := c.Graph(&ContextGraphOpts{Validate: true})
+	var err error
-	if err != nil && X_newApply {
+	if !X_legacyGraph {
-		// If we had an error graphing but we're using the new graph,
+		graph, err = (&ApplyGraphBuilder{
-		// just set it to nil and let it go. There are some features that
+			Module:       c.module,
-		// may work with the new graph that don't with the old.
+			Diff:         c.diff,
-		oldGraph = nil
+			State:        c.state,
-		err = nil
+			Providers:    c.components.ResourceProviders(),
-	}
+			Provisioners: c.components.ResourceProvisioners(),
-	if err != nil {
+			Destroy:      c.destroy,
-		return nil, err
+		}).Build(RootModulePath)
-	}
-
-	// Build the new graph. We do this no matter what so we can shadow it.
-	newGraph, err := (&ApplyGraphBuilder{
-		Module:       c.module,
-		Diff:         c.diff,
-		State:        c.state,
-		Providers:    c.components.ResourceProviders(),
-		Provisioners: c.components.ResourceProvisioners(),
-		Destroy:      c.destroy,
-	}).Build(RootModulePath)
-	if err != nil && !newGraphEnabled {
-		// If we had an error graphing but we're not using this graph, just
-		// set it to nil and record it as a shadow error.
-		c.shadowErr = multierror.Append(c.shadowErr, fmt.Errorf(
-			"Error building new graph: %s", err))
-
-		newGraph = nil
-		err = nil
-	}
-	if err != nil {
-		return nil, err
-	}
-
-	// Determine what is the real and what is the shadow. The logic here
-	// is straightforward though the if statements are not:
-	//
-	//   * Destroy mode - always use original, shadow with nothing because
-	//     we're only testing the new APPLY graph.
-	//   * Apply with new apply - use new graph, shadow is new graph. We can't
-	//     shadow with the old graph because the old graph does a lot more
-	//     that it shouldn't.
-	//   * Apply with old apply - use old graph, shadow with new graph.
-	//
-	real := oldGraph
-	shadow := newGraph
-	if newGraphEnabled {
-		log.Printf("[WARN] terraform: real graph is experiment, shadow is experiment")
-		real = shadow
 	} else {
-		log.Printf("[WARN] terraform: real graph is original, shadow is experiment")
+		graph, err = c.Graph(&ContextGraphOpts{Validate: true})
+	}
+	if err != nil {
+		return nil, err
 	}
 
 	// Determine the operation
@@ -424,14 +387,8 @@ func (c *Context) Apply() (*State, error) {
 		operation = walkDestroy
 	}
 
-	// This shouldn't happen, so assert it. This is before any state changes
-	// so it is safe to crash here.
-	if real == nil {
-		panic("nil real graph")
-	}
-
 	// Walk the graph
-	walker, err := c.walk(real, shadow, operation)
+	walker, err := c.walk(graph, graph, operation)
 	if len(walker.ValidationErrors) > 0 {
 		err = multierror.Append(err, walker.ValidationErrors...)
 	}
@@ -488,79 +445,35 @@ func (c *Context) Plan() (*Plan, error) {
 	c.diffLock.Unlock()
 
 	// Used throughout below
-	X_newApply := experiment.Enabled(experiment.X_newApply)
+	X_legacyGraph := experiment.Enabled(experiment.X_legacyGraph)
-	X_newDestroy := experiment.Enabled(experiment.X_newDestroy)
-	newGraphEnabled := (c.destroy && X_newDestroy) || (!c.destroy && X_newApply)
 
-	// Build the original graph. This is before the new graph builders
+	// Build the graph.
-	// coming in 0.8. We do this for shadow graphing.
+	var graph *Graph
-	oldGraph, err := c.Graph(&ContextGraphOpts{Validate: true})
+	var err error
-	if err != nil && newGraphEnabled {
+	if !X_legacyGraph {
-		// If we had an error graphing but we're using the new graph,
+		if c.destroy {
-		// just set it to nil and let it go. There are some features that
+			graph, err = (&DestroyPlanGraphBuilder{
-		// may work with the new graph that don't with the old.
+				Module:  c.module,
-		oldGraph = nil
+				State:   c.state,
-		err = nil
+				Targets: c.targets,
+			}).Build(RootModulePath)
+		} else {
+			graph, err = (&PlanGraphBuilder{
+				Module:    c.module,
+				State:     c.state,
+				Providers: c.components.ResourceProviders(),
+				Targets:   c.targets,
+			}).Build(RootModulePath)
+		}
+	} else {
+		graph, err = c.Graph(&ContextGraphOpts{Validate: true})
 	}
 	if err != nil {
 		return nil, err
 	}
 
-	// Build the new graph. We do this no matter wht so we can shadow it.
-	var newGraph *Graph
-	err = nil
-	if c.destroy {
-		newGraph, err = (&DestroyPlanGraphBuilder{
-			Module:  c.module,
-			State:   c.state,
-			Targets: c.targets,
-		}).Build(RootModulePath)
-	} else {
-		newGraph, err = (&PlanGraphBuilder{
-			Module:    c.module,
-			State:     c.state,
-			Providers: c.components.ResourceProviders(),
-			Targets:   c.targets,
-		}).Build(RootModulePath)
-	}
-	if err != nil && !newGraphEnabled {
-		// If we had an error graphing but we're not using this graph, just
-		// set it to nil and record it as a shadow error.
-		c.shadowErr = multierror.Append(c.shadowErr, fmt.Errorf(
-			"Error building new graph: %s", err))
-
-		newGraph = nil
-		err = nil
-	}
-	if err != nil {
-		return nil, err
-	}
-
-	// Determine what is the real and what is the shadow. The logic here
-	// is straightforward though the if statements are not:
-	//
-	//  * If the new graph, shadow with experiment in both because the
-	//    experiment has less nodes so the original can't shadow.
-	//  * If not the new graph, shadow with the experiment
-	//
-	real := oldGraph
-	shadow := newGraph
-	if newGraphEnabled {
-		log.Printf("[WARN] terraform: real graph is experiment, shadow is experiment")
-		real = shadow
-	} else {
-		log.Printf("[WARN] terraform: real graph is original, shadow is experiment")
-	}
-
-	// Special case here: if we're using destroy don't shadow it because
-	// the new destroy graph behaves a bit differently on purpose by not
-	// setting the module destroy flag.
-	if c.destroy && !newGraphEnabled {
-		shadow = nil
-	}
-
 	// Do the walk
-	walker, err := c.walk(real, shadow, operation)
+	walker, err := c.walk(graph, graph, operation)
 	if err != nil {
 		return nil, err
 	}
@@ -579,7 +492,7 @@ func (c *Context) Plan() (*Plan, error) {
 
 	// We don't do the reverification during the new destroy plan because
 	// it will use a different apply process.
-	if !newGraphEnabled {
+	if X_legacyGraph {
 		// Now that we have a diff, we can build the exact graph that Apply will use
 		// and catch any possible cycles during the Plan phase.
 		if _, err := c.Graph(&ContextGraphOpts{Validate: true}); err != nil {

terraform/debug_test.go

@@ -146,9 +146,13 @@ func TestDebug_plan(t *testing.T) {
 		t.Fatal("no files with data found")
 	}
 
-	if graphs == 0 {
+	/*
-		t.Fatal("no no-empty graphs found")
+			TODO: once @jbardin finishes the dot refactor, uncomment this. This
-	}
+			won't pass since the new graph doesn't implement the dot nodes.
+		if graphs == 0 {
+			t.Fatal("no no-empty graphs found")
+		}
+	*/
 }
 
 // verify that no hooks panic on nil input