diff --git a/internal/terraform/context_plan.go b/internal/terraform/context_plan.go index 001a2507ad..5610a445ba 100644 --- a/internal/terraform/context_plan.go +++ b/internal/terraform/context_plan.go @@ -262,8 +262,19 @@ func (c *Context) destroyPlan(config *configs.Config, prevRunState *states.State normalOpts := *opts normalOpts.Mode = plans.NormalMode refreshPlan, refreshDiags := c.plan(config, prevRunState, rootVariables, &normalOpts) - diags = diags.Append(refreshDiags) - if diags.HasErrors() { + if refreshDiags.HasErrors() { + // NOTE: Normally we'd append diagnostics regardless of whether + // there are errors, just in case there are warnings we'd want to + // preserve, but we're intentionally _not_ doing that here because + // if the first plan succeeded then we'll be running another plan + // in DestroyMode below, and we don't want to double-up any + // warnings that both plan walks would generate. + // (This does mean we won't show any warnings that would've been + // unique to only this walk, but we're assuming here that if the + // warnings aren't also applicable to a destroy plan then we'd + // rather not show them here, because this non-destroy plan for + // refreshing is largely an implementation detail.) + diags = diags.Append(refreshDiags) return nil, diags } diff --git a/internal/terraform/context_plan2_test.go b/internal/terraform/context_plan2_test.go index e0c57cd0a8..6d54e9eef1 100644 --- a/internal/terraform/context_plan2_test.go +++ b/internal/terraform/context_plan2_test.go @@ -887,6 +887,124 @@ Terraform has planned to destroy these objects. If Terraform's proposed changes }) } +func TestContext2Plan_movedResourceCollisionDestroy(t *testing.T) { + // This is like TestContext2Plan_movedResourceCollision but intended to + // ensure we still produce the expected warning (and produce it only once) + // when we're creating a destroy plan, rather than a normal plan. + // (This case is interesting at the time of writing because we happen to + // use a normal plan as a trick to refresh before creating a destroy plan. + // This test will probably become uninteresting if a future change to + // the destroy-time planning behavior handles refreshing in a different + // way, which avoids this pre-processing step of running a normal plan + // first.) + + addrNoKey := mustResourceInstanceAddr("test_object.a") + addrZeroKey := mustResourceInstanceAddr("test_object.a[0]") + m := testModuleInline(t, map[string]string{ + "main.tf": ` + resource "test_object" "a" { + # No "count" set, so test_object.a[0] will want + # to implicitly move to test_object.a, but will get + # blocked by the existing object at that address. + } + `, + }) + + state := states.BuildState(func(s *states.SyncState) { + s.SetResourceInstanceCurrent(addrNoKey, &states.ResourceInstanceObjectSrc{ + AttrsJSON: []byte(`{}`), + Status: states.ObjectReady, + }, mustProviderConfig(`provider["registry.terraform.io/hashicorp/test"]`)) + s.SetResourceInstanceCurrent(addrZeroKey, &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.DestroyMode, + }) + if diags.HasErrors() { + t.Fatalf("unexpected errors\n%s", diags.Err().Error()) + } + + // We should have a warning, though! We'll lightly abuse the "for RPC" + // feature of diagnostics to get some more-readily-comparable diagnostic + // values. + gotDiags := diags.ForRPC() + wantDiags := tfdiags.Diagnostics{ + tfdiags.Sourceless( + tfdiags.Warning, + "Unresolved resource instance address changes", + // NOTE: This message is _lightly_ confusing in the destroy case, + // because it says "Terraform has planned to destroy these objects" + // but this is a plan to destroy all objects, anyway. We expect the + // conflict situation to be pretty rare though, and even rarer in + // a "terraform destroy", so we'll just live with that for now + // unless we see evidence that lots of folks are being confused by + // it in practice. + `Terraform tried to adjust resource instance addresses in the prior state based on change information recorded in the configuration, but some adjustments did not succeed due to existing objects already at the intended addresses: + - test_object.a[0] could not move to test_object.a + +Terraform has planned to destroy these objects. If Terraform's proposed changes aren't appropriate, you must first resolve the conflicts using the "terraform state" subcommands and then create a new plan.`, + ), + }.ForRPC() + if diff := cmp.Diff(wantDiags, gotDiags); diff != "" { + // If we get here with a diff that makes it seem like the above warning + // is being reported twice, the likely cause is not correctly handling + // the warnings from the hidden normal plan we run as part of preparing + // for a destroy plan, unless that strategy has changed in the meantime + // since we originally wrote this test. + t.Errorf("wrong diagnostics\n%s", diff) + } + + t.Run(addrNoKey.String(), func(t *testing.T) { + instPlan := plan.Changes.ResourceInstance(addrNoKey) + if instPlan == nil { + t.Fatalf("no plan for %s at all", addrNoKey) + } + + if got, want := instPlan.Addr, addrNoKey; !got.Equal(want) { + t.Errorf("wrong current address\ngot: %s\nwant: %s", got, want) + } + if got, want := instPlan.PrevRunAddr, addrNoKey; !got.Equal(want) { + t.Errorf("wrong previous run address\ngot: %s\nwant: %s", got, want) + } + if got, want := instPlan.Action, plans.Delete; got != want { + t.Errorf("wrong planned action\ngot: %s\nwant: %s", got, want) + } + if got, want := instPlan.ActionReason, plans.ResourceInstanceChangeNoReason; got != want { + t.Errorf("wrong action reason\ngot: %s\nwant: %s", got, want) + } + }) + t.Run(addrZeroKey.String(), func(t *testing.T) { + instPlan := plan.Changes.ResourceInstance(addrZeroKey) + if instPlan == nil { + t.Fatalf("no plan for %s at all", addrZeroKey) + } + + if got, want := instPlan.Addr, addrZeroKey; !got.Equal(want) { + t.Errorf("wrong current address\ngot: %s\nwant: %s", got, want) + } + if got, want := instPlan.PrevRunAddr, addrZeroKey; !got.Equal(want) { + t.Errorf("wrong previous run address\ngot: %s\nwant: %s", got, want) + } + if got, want := instPlan.Action, plans.Delete; got != want { + t.Errorf("wrong planned action\ngot: %s\nwant: %s", got, want) + } + if got, want := instPlan.ActionReason, plans.ResourceInstanceChangeNoReason; got != want { + t.Errorf("wrong action reason\ngot: %s\nwant: %s", got, want) + } + }) +} + func TestContext2Plan_movedResourceUntargeted(t *testing.T) { addrA := mustResourceInstanceAddr("test_object.a") addrB := mustResourceInstanceAddr("test_object.b")