From f64978b64cc036bdb08ed551bd8ddec445e5bc4e Mon Sep 17 00:00:00 2001 From: Radek Simko Date: Fri, 14 Dec 2018 13:45:47 +0000 Subject: [PATCH] backend/local: Render CBD replacement (+/-) correctly (#19642) * backend/local: Render CBD replacement (+/-) correctly * command/format: Use IsReplace helper function --- backend/local/backend_plan.go | 15 +- backend/local/backend_plan_test.go | 152 +++++++++++++++++++ backend/local/test-fixtures/plan-cbd/main.tf | 13 ++ command/format/plan.go | 74 ++++----- 4 files changed, 198 insertions(+), 56 deletions(-) create mode 100644 backend/local/test-fixtures/plan-cbd/main.tf diff --git a/backend/local/backend_plan.go b/backend/local/backend_plan.go index 19f9e3df42..98185ccf3b 100644 --- a/backend/local/backend_plan.go +++ b/backend/local/backend_plan.go @@ -204,25 +204,22 @@ func (b *Local) renderPlan(plan *plans.Plan, schemas *terraform.Schemas) { headerBuf := &bytes.Buffer{} fmt.Fprintf(headerBuf, "\n%s\n", strings.TrimSpace(planHeaderIntro)) if counts[plans.Create] > 0 { - fmt.Fprintf(headerBuf, "%s create\n", format.DiffActionSymbol(terraform.DiffCreate)) + fmt.Fprintf(headerBuf, "%s create\n", format.DiffActionSymbol(plans.Create)) } if counts[plans.Update] > 0 { - fmt.Fprintf(headerBuf, "%s update in-place\n", format.DiffActionSymbol(terraform.DiffUpdate)) + fmt.Fprintf(headerBuf, "%s update in-place\n", format.DiffActionSymbol(plans.Update)) } if counts[plans.Delete] > 0 { - fmt.Fprintf(headerBuf, "%s destroy\n", format.DiffActionSymbol(terraform.DiffDestroy)) + fmt.Fprintf(headerBuf, "%s destroy\n", format.DiffActionSymbol(plans.Delete)) } if counts[plans.DeleteThenCreate] > 0 { - fmt.Fprintf(headerBuf, "%s destroy and then create replacement\n", format.DiffActionSymbol(terraform.DiffDestroyCreate)) + fmt.Fprintf(headerBuf, "%s destroy and then create replacement\n", format.DiffActionSymbol(plans.DeleteThenCreate)) } if counts[plans.CreateThenDelete] > 0 { - // FIXME: This shows the wrong symbol, because our old diff action - // type can't represent CreateThenDelete. We should switch - // format.DiffActionSymbol over to using plans.Action instead. - fmt.Fprintf(headerBuf, "%s create replacement and then destroy prior\n", format.DiffActionSymbol(terraform.DiffDestroyCreate)) + fmt.Fprintf(headerBuf, "%s create replacement and then destroy\n", format.DiffActionSymbol(plans.CreateThenDelete)) } if counts[plans.Read] > 0 { - fmt.Fprintf(headerBuf, "%s read (data resources)\n", format.DiffActionSymbol(terraform.DiffRefresh)) + fmt.Fprintf(headerBuf, "%s read (data resources)\n", format.DiffActionSymbol(plans.Read)) } b.CLI.Output(b.Colorize().Color(headerBuf.String())) diff --git a/backend/local/backend_plan_test.go b/backend/local/backend_plan_test.go index f0defcffb7..bd0da1c910 100644 --- a/backend/local/backend_plan_test.go +++ b/backend/local/backend_plan_test.go @@ -130,6 +130,132 @@ func TestLocal_planNoConfig(t *testing.T) { } } +func TestLocal_planTainted(t *testing.T) { + b, cleanup := TestLocal(t) + defer cleanup() + p := TestLocalProvider(t, b, "test", planFixtureSchema()) + testStateFile(t, b.StatePath, testPlanState_tainted()) + b.CLI = cli.NewMockUi() + outDir := testTempDir(t) + defer os.RemoveAll(outDir) + planPath := filepath.Join(outDir, "plan.tfplan") + op, configCleanup := testOperationPlan(t, "./test-fixtures/plan") + defer configCleanup() + op.PlanRefresh = true + op.PlanOutPath = planPath + cfg := cty.ObjectVal(map[string]cty.Value{ + "path": cty.StringVal(b.StatePath), + }) + cfgRaw, err := plans.NewDynamicValue(cfg, cfg.Type()) + if err != nil { + t.Fatal(err) + } + op.PlanOutBackend = &plans.Backend{ + // Just a placeholder so that we can generate a valid plan file. + Type: "local", + Config: cfgRaw, + } + run, err := b.Operation(context.Background(), op) + if err != nil { + t.Fatalf("bad: %s", err) + } + <-run.Done() + if run.Result != backend.OperationSuccess { + t.Fatalf("plan operation failed") + } + if !p.ReadResourceCalled { + t.Fatal("ReadResource should be called") + } + if run.PlanEmpty { + t.Fatal("plan should not be empty") + } + + expectedOutput := `An execution plan has been generated and is shown below. +Resource actions are indicated with the following symbols: +-/+ destroy and then create replacement + +Terraform will perform the following actions: + + # test_instance.foo must be replaced +-/+ resource "test_instance" "foo" { + ami = "bar" + + network_interface { + description = "Main network interface" + device_index = 0 + } + } + +Plan: 1 to add, 0 to change, 1 to destroy.` + output := b.CLI.(*cli.MockUi).OutputWriter.String() + if !strings.Contains(output, expectedOutput) { + t.Fatalf("Unexpected output:\n%s", output) + } +} + +func TestLocal_planTainted_createBeforeDestroy(t *testing.T) { + b, cleanup := TestLocal(t) + defer cleanup() + p := TestLocalProvider(t, b, "test", planFixtureSchema()) + testStateFile(t, b.StatePath, testPlanState_tainted()) + b.CLI = cli.NewMockUi() + outDir := testTempDir(t) + defer os.RemoveAll(outDir) + planPath := filepath.Join(outDir, "plan.tfplan") + op, configCleanup := testOperationPlan(t, "./test-fixtures/plan-cbd") + defer configCleanup() + op.PlanRefresh = true + op.PlanOutPath = planPath + cfg := cty.ObjectVal(map[string]cty.Value{ + "path": cty.StringVal(b.StatePath), + }) + cfgRaw, err := plans.NewDynamicValue(cfg, cfg.Type()) + if err != nil { + t.Fatal(err) + } + op.PlanOutBackend = &plans.Backend{ + // Just a placeholder so that we can generate a valid plan file. + Type: "local", + Config: cfgRaw, + } + run, err := b.Operation(context.Background(), op) + if err != nil { + t.Fatalf("bad: %s", err) + } + <-run.Done() + if run.Result != backend.OperationSuccess { + t.Fatalf("plan operation failed") + } + if !p.ReadResourceCalled { + t.Fatal("ReadResource should be called") + } + if run.PlanEmpty { + t.Fatal("plan should not be empty") + } + + expectedOutput := `An execution plan has been generated and is shown below. +Resource actions are indicated with the following symbols: ++/- create replacement and then destroy + +Terraform will perform the following actions: + + # test_instance.foo must be replaced ++/- resource "test_instance" "foo" { + ami = "bar" + + network_interface { + description = "Main network interface" + device_index = 0 + } + } + +Plan: 1 to add, 0 to change, 1 to destroy.` + output := b.CLI.(*cli.MockUi).OutputWriter.String() + if !strings.Contains(output, expectedOutput) { + t.Fatalf("Unexpected output:\n%s", output) + } +} + func TestLocal_planRefreshFalse(t *testing.T) { b, cleanup := TestLocal(t) defer cleanup() @@ -466,6 +592,32 @@ func testPlanState_withDataSource() *states.State { return state } +func testPlanState_tainted() *states.State { + state := states.NewState() + rootModule := state.RootModule() + rootModule.SetResourceInstanceCurrent( + addrs.Resource{ + Mode: addrs.ManagedResourceMode, + Type: "test_instance", + Name: "foo", + }.Instance(addrs.IntKey(0)), + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectTainted, + AttrsJSON: []byte(`{ + "ami": "bar", + "network_interface": [{ + "device_index": 0, + "description": "Main network interface" + }] + }`), + }, + addrs.ProviderConfig{ + Type: "test", + }.Absolute(addrs.RootModuleInstance), + ) + return state +} + func testReadPlan(t *testing.T, path string) *plans.Plan { t.Helper() diff --git a/backend/local/test-fixtures/plan-cbd/main.tf b/backend/local/test-fixtures/plan-cbd/main.tf new file mode 100644 index 0000000000..1a7ae84337 --- /dev/null +++ b/backend/local/test-fixtures/plan-cbd/main.tf @@ -0,0 +1,13 @@ +resource "test_instance" "foo" { + ami = "bar" + + # This is here because at some point it caused a test failure + network_interface { + device_index = 0 + description = "Main network interface" + } + + lifecycle { + create_before_destroy = true + } +} diff --git a/command/format/plan.go b/command/format/plan.go index bca7af5eb0..098653fcf6 100644 --- a/command/format/plan.go +++ b/command/format/plan.go @@ -29,7 +29,7 @@ type Plan struct { // for display, in conjunction with DisplayPlan. type InstanceDiff struct { Addr *terraform.ResourceAddress - Action terraform.DiffChangeType + Action plans.Action // Attributes describes changes to the attributes of the instance. // @@ -47,7 +47,7 @@ type AttributeDiff struct { // intended for display purposes only. Path string - Action terraform.DiffChangeType + Action plans.Action OldValue string NewValue string @@ -73,7 +73,7 @@ func NewPlan(changes *plans.Changes) *Plan { for _, rc := range changes.Resources { addr := rc.Addr - log.Printf("[TRACE] NewPlan found %s", addr) + log.Printf("[TRACE] NewPlan found %s (%s)", addr, rc.Action) dataSource := addr.Resource.Resource.Mode == addrs.DataResourceMode // We create "delete" actions for data resources so we can clean @@ -87,32 +87,8 @@ func NewPlan(changes *plans.Changes) *Plan { // TODO: Update for the new plan types, ideally also switching over to // a structural diff renderer instead of a flat renderer. did := &InstanceDiff{ - Addr: terraform.NewLegacyResourceInstanceAddress(addr), - } - - switch rc.Action { - case plans.NoOp: - continue - case plans.Create: - if dataSource { - // Use "refresh" as the action for display, but core - // currently uses Create for this internally. - // FIXME: Update core to generate plans.Read for this case - // instead. - did.Action = terraform.DiffRefresh - } else { - did.Action = terraform.DiffCreate - } - case plans.Read: - did.Action = terraform.DiffRefresh - case plans.Delete: - did.Action = terraform.DiffDestroy - case plans.DeleteThenCreate, plans.CreateThenDelete: - did.Action = terraform.DiffDestroyCreate - case plans.Update: - did.Action = terraform.DiffUpdate - default: - panic(fmt.Sprintf("unexpected change action %s", rc.Action)) + Addr: terraform.NewLegacyResourceInstanceAddress(addr), + Action: rc.Action, } if rc.DeposedKey != states.NotDeposed { @@ -180,14 +156,14 @@ func (p *Plan) Stats() PlanStats { var ret PlanStats for _, r := range p.Resources { switch r.Action { - case terraform.DiffCreate: + case plans.Create: ret.ToAdd++ - case terraform.DiffUpdate: + case plans.Update: ret.ToChange++ - case terraform.DiffDestroyCreate: + case plans.DeleteThenCreate, plans.CreateThenDelete: ret.ToAdd++ ret.ToDestroy++ - case terraform.DiffDestroy: + case plans.Delete: ret.ToDestroy++ } } @@ -195,8 +171,8 @@ func (p *Plan) Stats() PlanStats { } // ActionCounts returns the number of diffs for each action type -func (p *Plan) ActionCounts() map[terraform.DiffChangeType]int { - ret := map[terraform.DiffChangeType]int{} +func (p *Plan) ActionCounts() map[plans.Action]int { + ret := map[plans.Action]int{} for _, r := range p.Resources { ret[r.Action]++ } @@ -212,18 +188,22 @@ func (p *Plan) Empty() bool { // colorstring.Colorize, will produce a result that can be written // to a terminal to produce a symbol made of three printable // characters, possibly interspersed with VT100 color codes. -func DiffActionSymbol(action terraform.DiffChangeType) string { +func DiffActionSymbol(action plans.Action) string { switch action { - case terraform.DiffDestroyCreate: + case plans.DeleteThenCreate: return "[red]-[reset]/[green]+[reset]" - case terraform.DiffCreate: + case plans.CreateThenDelete: + return "[green]+[reset]/[red]-[reset]" + case plans.Create: return " [green]+[reset]" - case terraform.DiffDestroy: + case plans.Delete: return " [red]-[reset]" - case terraform.DiffRefresh: + case plans.Read: return " [cyan]<=[reset]" - default: + case plans.Update: return " [yellow]~[reset]" + default: + return " ?" } } @@ -239,14 +219,14 @@ func formatPlanInstanceDiff(buf *bytes.Buffer, r *InstanceDiff, keyLen int, colo symbol := DiffActionSymbol(r.Action) oldValues := true switch r.Action { - case terraform.DiffDestroyCreate: + case plans.DeleteThenCreate, plans.CreateThenDelete: color = "yellow" - case terraform.DiffCreate: + case plans.Create: color = "green" oldValues = false - case terraform.DiffDestroy: + case plans.Delete: color = "red" - case terraform.DiffRefresh: + case plans.Read: color = "cyan" oldValues = false } @@ -258,7 +238,7 @@ func formatPlanInstanceDiff(buf *bytes.Buffer, r *InstanceDiff, keyLen int, colo if r.Deposed { extraStr = extraStr + " (deposed)" } - if r.Action == terraform.DiffDestroyCreate { + if r.Action.IsReplace() { extraStr = extraStr + colorizer.Color(" [red][bold](new resource required)") } @@ -284,7 +264,7 @@ func formatPlanInstanceDiff(buf *bytes.Buffer, r *InstanceDiff, keyLen int, colo updateMsg := "" switch { - case attr.ForcesNew && r.Action == terraform.DiffDestroyCreate: + case attr.ForcesNew && r.Action.IsReplace(): updateMsg = colorizer.Color(" [red](forces new resource)") case attr.Sensitive && oldValues: updateMsg = colorizer.Color(" [yellow](attribute changed)")