From 785cc7b78aedda6994f14c99bdaf6d46c4769afd Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Thu, 10 Nov 2016 07:54:39 -0800 Subject: [PATCH] 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 | 1 - command/apply.go | 46 ++------- command/apply_destroy_test.go | 2 +- helper/experiment/experiment.go | 10 +- terraform/context.go | 171 ++++++++------------------------ terraform/debug_test.go | 10 +- 6 files changed, 61 insertions(+), 179 deletions(-) diff --git a/.travis.yml b/.travis.yml index b39854fb1..6a77ce953 100644 --- a/.travis.yml +++ b/.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 diff --git a/command/apply.go b/command/apply.go index 128ea5e75..55f44f0b3 100644 --- a/command/apply.go +++ b/command/apply.go @@ -98,44 +98,14 @@ func (c *ApplyCommand) Run(args []string) int { terraform.SetDebugInfo(DefaultDataDir) - // Check for the new apply - if experiment.Enabled(experiment.X_newApply) && !experiment.Force() { - desc := "Experimental new apply 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-apply", - Query: "Experimental feature enabled: new apply 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 - } - } - - // 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 - } + // Check for the legacy graph + if experiment.Enabled(experiment.X_legacyGraph) { + c.Ui.Output(c.Colorize().Color( + "[reset][bold][yellow]" + + "Legacy graph enabled! This will use the graph from Terraform 0.7.x\n" + + "to execute this operation. This will be removed in the future so\n" + + "please report any issues causing you to use this to the Terraform\n" + + "project.\n\n")) } // This is going to keep track of shadow errors diff --git a/command/apply_destroy_test.go b/command/apply_destroy_test.go index 1d7fe4c54..b82ffdf47 100644 --- a/command/apply_destroy_test.go +++ b/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 diff --git a/helper/experiment/experiment.go b/helper/experiment/experiment.go index 1a49c4d27..991011018 100644 --- a/helper/experiment/experiment.go +++ b/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. - X_newApply = newBasicID("new-apply", "NEW_APPLY", false) - - // New destroy graph. This will be reomved and be the default in 0.8.0. - X_newDestroy = newBasicID("new-destroy", "NEW_DESTROY", false) + // Reuse the old graphs from TF 0.7.x. These will be removed at some point. + X_legacyGraph = newBasicID("legacy-graph", "LEGACY_GRAPH", 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_newDestroy, + X_legacyGraph, X_shadow, x_force, } diff --git a/terraform/context.go b/terraform/context.go index 3cef50945..7720bd8ac 100644 --- a/terraform/context.go +++ b/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) - X_newDestroy := experiment.Enabled(experiment.X_newDestroy) - newGraphEnabled := (c.destroy && X_newDestroy) || (!c.destroy && X_newApply) + // Enable the new graph by default + X_legacyGraph := experiment.Enabled(experiment.X_legacyGraph) - // Build the original graph. This is before the new graph builders - // coming in 0.8. We do this for shadow graphing. - oldGraph, err := c.Graph(&ContextGraphOpts{Validate: true}) - if err != nil && X_newApply { - // If we had an error graphing but we're using the new graph, - // just set it to nil and let it go. There are some features that - // may work with the new graph that don't with the old. - oldGraph = nil - err = nil - } - if err != nil { - return nil, err - } - - // 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 + // Build the graph. + var graph *Graph + var err error + if !X_legacyGraph { + graph, err = (&ApplyGraphBuilder{ + Module: c.module, + Diff: c.diff, + State: c.state, + Providers: c.components.ResourceProviders(), + Provisioners: c.components.ResourceProvisioners(), + Destroy: c.destroy, + }).Build(RootModulePath) } 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_newDestroy := experiment.Enabled(experiment.X_newDestroy) - newGraphEnabled := (c.destroy && X_newDestroy) || (!c.destroy && X_newApply) + X_legacyGraph := experiment.Enabled(experiment.X_legacyGraph) - // Build the original graph. This is before the new graph builders - // coming in 0.8. We do this for shadow graphing. - oldGraph, err := c.Graph(&ContextGraphOpts{Validate: true}) - if err != nil && newGraphEnabled { - // If we had an error graphing but we're using the new graph, - // just set it to nil and let it go. There are some features that - // may work with the new graph that don't with the old. - oldGraph = nil - err = nil + // Build the graph. + var graph *Graph + var err error + if !X_legacyGraph { + if c.destroy { + graph, err = (&DestroyPlanGraphBuilder{ + Module: c.module, + State: c.state, + 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 { diff --git a/terraform/debug_test.go b/terraform/debug_test.go index cd105f238..5f8cbb18b 100644 --- a/terraform/debug_test.go +++ b/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