Merge pull request #29597 from hashicorp/alisdair/move-refresh-ui

cli: Improved plan UI for move-only changes

internal/command/views/operation_test.go

@@ -189,6 +189,55 @@ func TestOperation_planNoChanges(t *testing.T) {
 			},
 			"If you were expecting these changes then you can apply this plan",
 		},
+		"move-only changes in refresh-only mode": {
+			func(schemas *terraform.Schemas) *plans.Plan {
+				addr := addrs.Resource{
+					Mode: addrs.ManagedResourceMode,
+					Type: "test_resource",
+					Name: "somewhere",
+				}.Instance(addrs.NoKey).Absolute(addrs.RootModuleInstance)
+				addrPrev := addrs.Resource{
+					Mode: addrs.ManagedResourceMode,
+					Type: "test_resource",
+					Name: "anywhere",
+				}.Instance(addrs.NoKey).Absolute(addrs.RootModuleInstance)
+				schema, _ := schemas.ResourceTypeConfig(
+					addrs.NewDefaultProvider("test"),
+					addr.Resource.Resource.Mode,
+					addr.Resource.Resource.Type,
+				)
+				ty := schema.ImpliedType()
+				rc := &plans.ResourceInstanceChange{
+					Addr:        addr,
+					PrevRunAddr: addrPrev,
+					ProviderAddr: addrs.RootModuleInstance.ProviderConfigDefault(
+						addrs.NewDefaultProvider("test"),
+					),
+					Change: plans.Change{
+						Action: plans.NoOp,
+						Before: cty.ObjectVal(map[string]cty.Value{
+							"id":  cty.StringVal("1234"),
+							"foo": cty.StringVal("bar"),
+						}),
+						After: cty.ObjectVal(map[string]cty.Value{
+							"id":  cty.StringVal("1234"),
+							"foo": cty.StringVal("bar"),
+						}),
+					},
+				}
+				rcs, err := rc.Encode(ty)
+				if err != nil {
+					panic(err)
+				}
+				drs := []*plans.ResourceInstanceChangeSrc{rcs}
+				return &plans.Plan{
+					UIMode:           plans.RefreshOnlyMode,
+					Changes:          plans.NewChanges(),
+					DriftedResources: drs,
+				}
+			},
+			"test_resource.anywhere has moved to test_resource.somewhere",
+		},
 		"drift detected in destroy mode": {
 			func(schemas *terraform.Schemas) *plans.Plan {
 				return &plans.Plan{

internal/command/views/plan.go

@@ -96,9 +96,24 @@ func (v *PlanJSON) HelpPrompt() {
 // The plan renderer is used by the Operation view (for plan and apply
 // commands) and the Show view (for the show command).
 func renderPlan(plan *plans.Plan, schemas *terraform.Schemas, view *View) {
-	haveRefreshChanges := len(plan.DriftedResources) > 0
+	// In refresh-only mode, we show all resources marked as drifted,
+	// including those which have moved without other changes. In other plan
+	// modes, move-only changes will be rendered in the planned changes, so
+	// we skip them here.
+	var driftedResources []*plans.ResourceInstanceChangeSrc
+	if plan.UIMode == plans.RefreshOnlyMode {
+		driftedResources = plan.DriftedResources
+	} else {
+		for _, dr := range plan.DriftedResources {
+			if dr.Action != plans.NoOp {
+				driftedResources = append(driftedResources, dr)
+			}
+		}
+	}
+
+	haveRefreshChanges := len(driftedResources) > 0
 	if haveRefreshChanges {
-		renderChangesDetectedByRefresh(plan.DriftedResources, schemas, view)
+		renderChangesDetectedByRefresh(driftedResources, schemas, view)
 		switch plan.UIMode {
 		case plans.RefreshOnlyMode:
 			view.streams.Println(format.WordWrap(
@@ -368,10 +383,6 @@ func renderChangesDetectedByRefresh(drs []*plans.ResourceInstanceChangeSrc, sche
 	})
 
 	for _, rcs := range drs {
-		if rcs.Action == plans.NoOp && !rcs.Moved() {
-			continue
-		}
-
 		providerSchema := schemas.ProviderSchema(rcs.ProviderAddr.Provider)
 		if providerSchema == nil {
 			// Should never happen

internal/terraform/context_plan.go

@@ -471,7 +471,7 @@ func (c *Context) planGraph(config *configs.Config, prevRunState *states.State,
 func (c *Context) driftedResources(config *configs.Config, oldState, newState *states.State, moves map[addrs.UniqueKey]refactoring.MoveResult) ([]*plans.ResourceInstanceChangeSrc, tfdiags.Diagnostics) {
 	var diags tfdiags.Diagnostics
 
-	if newState.ManagedResourcesEqual(oldState) {
+	if newState.ManagedResourcesEqual(oldState) && len(moves) == 0 {
 		// Nothing to do, because we only detect and report drift for managed
 		// resource instances.
 		return nil, diags
@@ -499,6 +499,14 @@ func (c *Context) driftedResources(config *configs.Config, oldState, newState *s
 					continue
 				}
 				addr := rs.Addr.Instance(key)
+
+				// Previous run address defaults to the current address, but
+				// can differ if the resource moved before refreshing
+				prevRunAddr := addr
+				if move, ok := moves[addr.UniqueKey()]; ok {
+					prevRunAddr = move.From
+				}
+
 				newIS := newState.ResourceInstance(addr)
 
 				schema, _ := schemas.ResourceTypeConfig(
@@ -547,20 +555,30 @@ func (c *Context) driftedResources(config *configs.Config, oldState, newState *s
 					newVal = cty.NullVal(ty)
 				}
 
-				if oldVal.RawEquals(newVal) {
+				if oldVal.RawEquals(newVal) && addr.Equal(prevRunAddr) {
 					// No drift if the two values are semantically equivalent
+					// and no move has happened
 					continue
 				}
 
-				// We can only detect updates and deletes as drift.
-				action := plans.Update
-				if newVal.IsNull() {
+				// We can detect three types of changes after refreshing state,
+				// only two of which are easily understood as "drift":
+				//
+				// - Resources which were deleted outside of Terraform;
+				// - Resources where the object value has changed outside of
+				//   Terraform;
+				// - Resources which have been moved without other changes.
+				//
+				// All of these are returned as drift, to allow refresh-only plans
+				// to present a full set of changes which will be applied.
+				var action plans.Action
+				switch {
+				case newVal.IsNull():
 					action = plans.Delete
-				}
-
-				prevRunAddr := addr
-				if move, ok := moves[addr.UniqueKey()]; ok {
-					prevRunAddr = move.From
+				case !oldVal.RawEquals(newVal):
+					action = plans.Update
+				default:
+					action = plans.NoOp
 				}
 
 				change := &plans.ResourceInstanceChange{

internal/terraform/context_plan2_test.go

@@ -984,7 +984,82 @@ The -target option is not for routine use, and is provided only for exceptional
 		if diff := cmp.Diff(wantDiags, gotDiags); diff != "" {
 			t.Errorf("wrong diagnostics\n%s", diff)
 		}
+	})
+}
+
+func TestContext2Plan_movedResourceRefreshOnly(t *testing.T) {
+	addrA := mustResourceInstanceAddr("test_object.a")
+	addrB := mustResourceInstanceAddr("test_object.b")
+	m := testModuleInline(t, map[string]string{
+		"main.tf": `
+			resource "test_object" "b" {
+			}
+
+			moved {
+				from = test_object.a
+				to   = test_object.b
+			}
+
+			terraform {
+				experiments = [config_driven_move]
+			}
+		`,
+	})
+
+	state := states.BuildState(func(s *states.SyncState) {
+		// The prior state tracks test_object.a, which we should treat as
+		// test_object.b because of the "moved" block in the config.
+		s.SetResourceInstanceCurrent(addrA, &states.ResourceInstanceObjectSrc{
+			AttrsJSON: []byte(`{}`),
+			Status:    states.ObjectReady,
+		}, mustProviderConfig(`provider["registry.terraform.io/hashicorp/test"]`))
+	})
 
+	p := simpleMockProvider()
+	ctx := testContext2(t, &ContextOpts{
+		Providers: map[addrs.Provider]providers.Factory{
+			addrs.NewDefaultProvider("test"): testProviderFuncFixed(p),
+		},
+	})
+
+	plan, diags := ctx.Plan(m, state, &PlanOpts{
+		Mode: plans.RefreshOnlyMode,
+	})
+	if diags.HasErrors() {
+		t.Fatalf("unexpected errors\n%s", diags.Err().Error())
+	}
+
+	t.Run(addrA.String(), func(t *testing.T) {
+		instPlan := plan.Changes.ResourceInstance(addrA)
+		if instPlan != nil {
+			t.Fatalf("unexpected plan for %s; should've moved to %s", addrA, addrB)
+		}
+	})
+	t.Run(addrB.String(), func(t *testing.T) {
+		instPlan := plan.Changes.ResourceInstance(addrB)
+		if instPlan != nil {
+			t.Fatalf("unexpected plan for %s", addrB)
+		}
+	})
+	t.Run("drift", func(t *testing.T) {
+		var drifted *plans.ResourceInstanceChangeSrc
+		for _, dr := range plan.DriftedResources {
+			if dr.Addr.Equal(addrB) {
+				drifted = dr
+				break
+			}
+		}
+
+		if drifted == nil {
+			t.Fatalf("instance %s is missing from the drifted resource changes", addrB)
+		}
+
+		if got, want := drifted.PrevRunAddr, addrA; !got.Equal(want) {
+			t.Errorf("wrong previous run address\ngot:  %s\nwant: %s", got, want)
+		}
+		if got, want := drifted.Action, plans.NoOp; got != want {
+			t.Errorf("wrong planned action\ngot:  %s\nwant: %s", got, want)
+		}
 	})
 }