From 29999c1d6f0bc58b4d8ce550a33046ce834370fb Mon Sep 17 00:00:00 2001 From: Alisdair McDiarmid Date: Fri, 3 Sep 2021 12:10:16 -0400 Subject: [PATCH] command: Render "moved" annotations in plan UI For resources which are planned to move, render the previous run address as additional information in the plan UI. For the case of a move-only resource (which otherwise is unchanged), we also render that as a planned change, but without any corresponding action symbol. If all changes in the plan are moves without changes, the plan is no longer considered "empty". In this case, we skip rendering the action symbols in the UI. --- internal/command/format/diff.go | 16 +++- internal/command/format/diff_test.go | 96 ++++++++++++++++++++-- internal/command/views/plan.go | 20 +++-- internal/command/views/plan_test.go | 13 +-- internal/plans/changes.go | 2 +- internal/plans/changes_src.go | 4 + internal/plans/changes_test.go | 117 +++++++++++++++++++++++++++ 7 files changed, 248 insertions(+), 20 deletions(-) diff --git a/internal/command/format/diff.go b/internal/command/format/diff.go index 982b95980..e111485e0 100644 --- a/internal/command/format/diff.go +++ b/internal/command/format/diff.go @@ -72,13 +72,27 @@ func ResourceChange( // Some extra context about this unusual situation. buf.WriteString(color.Color("\n # (left over from a partially-failed replacement of this instance)")) } + case plans.NoOp: + if change.Moved() { + buf.WriteString(color.Color(fmt.Sprintf("[bold] # %s[reset] has moved to [bold]%s[reset]", change.PrevRunAddr.String(), dispAddr))) + break + } + fallthrough default: // should never happen, since the above is exhaustive buf.WriteString(fmt.Sprintf("%s has an action the plan renderer doesn't support (this is a bug)", dispAddr)) } buf.WriteString(color.Color("[reset]\n")) - buf.WriteString(color.Color(DiffActionSymbol(change.Action)) + " ") + if change.Moved() && change.Action != plans.NoOp { + buf.WriteString(color.Color(fmt.Sprintf("[bold] # [reset]([bold]%s[reset] has moved to [bold]%s[reset])\n", change.PrevRunAddr.String(), dispAddr))) + } + + if change.Moved() && change.Action == plans.NoOp { + buf.WriteString(" ") + } else { + buf.WriteString(color.Color(DiffActionSymbol(change.Action)) + " ") + } switch addr.Resource.Resource.Mode { case addrs.ManagedResourceMode: diff --git a/internal/command/format/diff_test.go b/internal/command/format/diff_test.go index 31cb62d7d..7ca289c79 100644 --- a/internal/command/format/diff_test.go +++ b/internal/command/format/diff_test.go @@ -4448,6 +4448,79 @@ func TestResourceChange_sensitiveVariable(t *testing.T) { runTestCases(t, testCases) } +func TestResourceChange_moved(t *testing.T) { + prevRunAddr := addrs.Resource{ + Mode: addrs.ManagedResourceMode, + Type: "test_instance", + Name: "previous", + }.Instance(addrs.NoKey).Absolute(addrs.RootModuleInstance) + + testCases := map[string]testCase{ + "moved and updated": { + PrevRunAddr: prevRunAddr, + Action: plans.Update, + Mode: addrs.ManagedResourceMode, + Before: cty.ObjectVal(map[string]cty.Value{ + "id": cty.StringVal("12345"), + "foo": cty.StringVal("hello"), + "bar": cty.StringVal("baz"), + }), + After: cty.ObjectVal(map[string]cty.Value{ + "id": cty.StringVal("12345"), + "foo": cty.StringVal("hello"), + "bar": cty.StringVal("boop"), + }), + Schema: &configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "id": {Type: cty.String, Computed: true}, + "foo": {Type: cty.String, Optional: true}, + "bar": {Type: cty.String, Optional: true}, + }, + }, + RequiredReplace: cty.NewPathSet(), + ExpectedOutput: ` # test_instance.example will be updated in-place + # (test_instance.previous has moved to test_instance.example) + ~ resource "test_instance" "example" { + ~ bar = "baz" -> "boop" + id = "12345" + # (1 unchanged attribute hidden) + } +`, + }, + "moved without changes": { + PrevRunAddr: prevRunAddr, + Action: plans.NoOp, + Mode: addrs.ManagedResourceMode, + Before: cty.ObjectVal(map[string]cty.Value{ + "id": cty.StringVal("12345"), + "foo": cty.StringVal("hello"), + "bar": cty.StringVal("baz"), + }), + After: cty.ObjectVal(map[string]cty.Value{ + "id": cty.StringVal("12345"), + "foo": cty.StringVal("hello"), + "bar": cty.StringVal("baz"), + }), + Schema: &configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "id": {Type: cty.String, Computed: true}, + "foo": {Type: cty.String, Optional: true}, + "bar": {Type: cty.String, Optional: true}, + }, + }, + RequiredReplace: cty.NewPathSet(), + ExpectedOutput: ` # test_instance.previous has moved to test_instance.example + resource "test_instance" "example" { + id = "12345" + # (2 unchanged attributes hidden) + } +`, + }, + } + + runTestCases(t, testCases) +} + type testCase struct { Action plans.Action ActionReason plans.ResourceInstanceChangeActionReason @@ -4460,6 +4533,7 @@ type testCase struct { Schema *configschema.Block RequiredReplace cty.PathSet ExpectedOutput string + PrevRunAddr addrs.AbsResourceInstance } func runTestCases(t *testing.T, testCases map[string]testCase) { @@ -4493,13 +4567,23 @@ func runTestCases(t *testing.T, testCases map[string]testCase) { t.Fatal(err) } + addr := addrs.Resource{ + Mode: tc.Mode, + Type: "test_instance", + Name: "example", + }.Instance(addrs.NoKey).Absolute(addrs.RootModuleInstance) + + prevRunAddr := tc.PrevRunAddr + // If no previous run address is given, reuse the current address + // to make initialization easier + if prevRunAddr.Resource.Resource.Type == "" { + prevRunAddr = addr + } + change := &plans.ResourceInstanceChangeSrc{ - Addr: addrs.Resource{ - Mode: tc.Mode, - Type: "test_instance", - Name: "example", - }.Instance(addrs.NoKey).Absolute(addrs.RootModuleInstance), - DeposedKey: tc.DeposedKey, + Addr: addr, + PrevRunAddr: prevRunAddr, + DeposedKey: tc.DeposedKey, ProviderAddr: addrs.AbsProviderConfig{ Provider: addrs.NewDefaultProvider("test"), Module: addrs.RootModule, diff --git a/internal/command/views/plan.go b/internal/command/views/plan.go index 5d1798d2f..ff794933b 100644 --- a/internal/command/views/plan.go +++ b/internal/command/views/plan.go @@ -116,7 +116,7 @@ func renderPlan(plan *plans.Plan, schemas *terraform.Schemas, view *View) { counts := map[plans.Action]int{} var rChanges []*plans.ResourceInstanceChangeSrc for _, change := range plan.Changes.Resources { - if change.Action == plans.NoOp { + if change.Action == plans.NoOp && !change.Moved() { continue // We don't show anything for no-op changes } if change.Action == plans.Delete && change.Addr.Resource.Resource.Mode == addrs.DataResourceMode { @@ -125,7 +125,11 @@ func renderPlan(plan *plans.Plan, schemas *terraform.Schemas, view *View) { } rChanges = append(rChanges, change) - counts[change.Action]++ + + // Don't count move-only changes + if change.Action != plans.NoOp { + counts[change.Action]++ + } } var changedRootModuleOutputs []*plans.OutputChangeSrc for _, output := range plan.Changes.Outputs { @@ -138,7 +142,7 @@ func renderPlan(plan *plans.Plan, schemas *terraform.Schemas, view *View) { changedRootModuleOutputs = append(changedRootModuleOutputs, output) } - if len(counts) == 0 && len(changedRootModuleOutputs) == 0 { + if len(rChanges) == 0 && len(changedRootModuleOutputs) == 0 { // If we didn't find any changes to report at all then this is a // "No changes" plan. How we'll present this depends on whether // the plan is "applyable" and, if so, whether it had refresh changes @@ -225,7 +229,7 @@ func renderPlan(plan *plans.Plan, schemas *terraform.Schemas, view *View) { view.streams.Println("") } - if len(counts) != 0 { + if len(counts) > 0 { headerBuf := &bytes.Buffer{} fmt.Fprintf(headerBuf, "\n%s\n", strings.TrimSpace(format.WordWrap(planHeaderIntro, view.outputColumns()))) if counts[plans.Create] > 0 { @@ -247,9 +251,11 @@ func renderPlan(plan *plans.Plan, schemas *terraform.Schemas, view *View) { fmt.Fprintf(headerBuf, "%s read (data resources)\n", format.DiffActionSymbol(plans.Read)) } - view.streams.Println(view.colorize.Color(headerBuf.String())) + view.streams.Print(view.colorize.Color(headerBuf.String())) + } - view.streams.Printf("Terraform will perform the following actions:\n\n") + if len(rChanges) > 0 { + view.streams.Printf("\nTerraform will perform the following actions:\n\n") // Note: we're modifying the backing slice of this plan object in-place // here. The ordering of resource changes in a plan is not significant, @@ -265,7 +271,7 @@ func renderPlan(plan *plans.Plan, schemas *terraform.Schemas, view *View) { }) for _, rcs := range rChanges { - if rcs.Action == plans.NoOp { + if rcs.Action == plans.NoOp && !rcs.Moved() { continue } diff --git a/internal/command/views/plan_test.go b/internal/command/views/plan_test.go index 13cd3c4c3..757c7162d 100644 --- a/internal/command/views/plan_test.go +++ b/internal/command/views/plan_test.go @@ -63,12 +63,15 @@ func testPlan(t *testing.T) *plans.Plan { } changes := plans.NewChanges() + addr := addrs.Resource{ + Mode: addrs.ManagedResourceMode, + Type: "test_resource", + Name: "foo", + }.Instance(addrs.NoKey).Absolute(addrs.RootModuleInstance) + changes.SyncWrapper().AppendResourceInstanceChange(&plans.ResourceInstanceChangeSrc{ - Addr: addrs.Resource{ - Mode: addrs.ManagedResourceMode, - Type: "test_resource", - Name: "foo", - }.Instance(addrs.NoKey).Absolute(addrs.RootModuleInstance), + Addr: addr, + PrevRunAddr: addr, ProviderAddr: addrs.AbsProviderConfig{ Provider: addrs.NewDefaultProvider("test"), Module: addrs.RootModule, diff --git a/internal/plans/changes.go b/internal/plans/changes.go index ba06244cb..c9aa38fd3 100644 --- a/internal/plans/changes.go +++ b/internal/plans/changes.go @@ -33,7 +33,7 @@ func NewChanges() *Changes { func (c *Changes) Empty() bool { for _, res := range c.Resources { - if res.Action != NoOp { + if res.Action != NoOp || res.Moved() { return false } } diff --git a/internal/plans/changes_src.go b/internal/plans/changes_src.go index 69330a21d..396493956 100644 --- a/internal/plans/changes_src.go +++ b/internal/plans/changes_src.go @@ -125,6 +125,10 @@ func (rcs *ResourceInstanceChangeSrc) DeepCopy() *ResourceInstanceChangeSrc { return &ret } +func (rcs *ResourceInstanceChangeSrc) Moved() bool { + return !rcs.Addr.Equal(rcs.PrevRunAddr) +} + // OutputChangeSrc describes a change to an output value. type OutputChangeSrc struct { // Addr is the absolute address of the output value that the change diff --git a/internal/plans/changes_test.go b/internal/plans/changes_test.go index 16062429b..5dbe10f08 100644 --- a/internal/plans/changes_test.go +++ b/internal/plans/changes_test.go @@ -4,10 +4,127 @@ import ( "fmt" "testing" + "github.com/hashicorp/terraform/internal/addrs" "github.com/hashicorp/terraform/internal/lang/marks" "github.com/zclconf/go-cty/cty" ) +func TestChangesEmpty(t *testing.T) { + testCases := map[string]struct { + changes *Changes + want bool + }{ + "no changes": { + &Changes{}, + true, + }, + "resource change": { + &Changes{ + Resources: []*ResourceInstanceChangeSrc{ + { + Addr: addrs.Resource{ + Mode: addrs.ManagedResourceMode, + Type: "test_thing", + Name: "woot", + }.Instance(addrs.NoKey).Absolute(addrs.RootModuleInstance), + PrevRunAddr: addrs.Resource{ + Mode: addrs.ManagedResourceMode, + Type: "test_thing", + Name: "woot", + }.Instance(addrs.NoKey).Absolute(addrs.RootModuleInstance), + ChangeSrc: ChangeSrc{ + Action: Update, + }, + }, + }, + }, + false, + }, + "resource change with no-op action": { + &Changes{ + Resources: []*ResourceInstanceChangeSrc{ + { + Addr: addrs.Resource{ + Mode: addrs.ManagedResourceMode, + Type: "test_thing", + Name: "woot", + }.Instance(addrs.NoKey).Absolute(addrs.RootModuleInstance), + PrevRunAddr: addrs.Resource{ + Mode: addrs.ManagedResourceMode, + Type: "test_thing", + Name: "woot", + }.Instance(addrs.NoKey).Absolute(addrs.RootModuleInstance), + ChangeSrc: ChangeSrc{ + Action: NoOp, + }, + }, + }, + }, + true, + }, + "resource moved with no-op change": { + &Changes{ + Resources: []*ResourceInstanceChangeSrc{ + { + Addr: addrs.Resource{ + Mode: addrs.ManagedResourceMode, + Type: "test_thing", + Name: "woot", + }.Instance(addrs.NoKey).Absolute(addrs.RootModuleInstance), + PrevRunAddr: addrs.Resource{ + Mode: addrs.ManagedResourceMode, + Type: "test_thing", + Name: "toot", + }.Instance(addrs.NoKey).Absolute(addrs.RootModuleInstance), + ChangeSrc: ChangeSrc{ + Action: NoOp, + }, + }, + }, + }, + false, + }, + "output change": { + &Changes{ + Outputs: []*OutputChangeSrc{ + { + Addr: addrs.OutputValue{ + Name: "result", + }.Absolute(addrs.RootModuleInstance), + ChangeSrc: ChangeSrc{ + Action: Update, + }, + }, + }, + }, + false, + }, + "output change no-op": { + &Changes{ + Outputs: []*OutputChangeSrc{ + { + Addr: addrs.OutputValue{ + Name: "result", + }.Absolute(addrs.RootModuleInstance), + ChangeSrc: ChangeSrc{ + Action: NoOp, + }, + }, + }, + }, + true, + }, + } + + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + if got, want := tc.changes.Empty(), tc.want; got != want { + t.Fatalf("unexpected result: got %v, want %v", got, want) + } + }) + } +} + func TestChangeEncodeSensitive(t *testing.T) { testVals := []cty.Value{ cty.ObjectVal(map[string]cty.Value{