From b61c02da05e6af8ebc1a3881438c91ff414ca9f8 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 28 Oct 2022 13:57:51 -0400 Subject: [PATCH 1/7] don't lose checks from refresh-only plan If there are no changes, then there is no reason to create an apply graph since all objects are known. We however do need the walk to match the expected state structure. This is probably only cleanup of empty nested modules and outputs, but some investigation is needed before making the full change. For now we can store the checks from the plan directly into the new state, since the apply walk overwrote the results we had already. --- internal/terraform/context_apply.go | 12 ++++ internal/terraform/context_apply2_test.go | 72 +++++++++++++++++++++++ 2 files changed, 84 insertions(+) diff --git a/internal/terraform/context_apply.go b/internal/terraform/context_apply.go index 4630f66a9193..adb363a3e6a0 100644 --- a/internal/terraform/context_apply.go +++ b/internal/terraform/context_apply.go @@ -69,6 +69,18 @@ Note that the -target option is not suitable for routine use, and is provided on )) } + // FIXME: we cannot check for an empty plan for refresh-only, because root + // outputs are always stored as changes. The final condition of the state + // also depends on some cleanup which happens during the apply walk. It + // would probably make more sense if applying a refresh-only plan were + // simply just returning the planned state and checks, but some extra + // cleanup is going to be needed to make the plan state match what apply + // would do. For now we can copy the checks over which were overwritten + // during the apply walk. + if len(plan.Changes.Resources) == 0 { + newState.CheckResults = plan.Checks.DeepCopy() + } + return newState, diags } diff --git a/internal/terraform/context_apply2_test.go b/internal/terraform/context_apply2_test.go index 100e75def83d..e7261640b38e 100644 --- a/internal/terraform/context_apply2_test.go +++ b/internal/terraform/context_apply2_test.go @@ -1641,3 +1641,75 @@ output "from_resource" { _, diags = ctx.Apply(plan, m) assertNoErrors(t, diags) } + +// -refresh-only should update checks +func TestContext2Apply_refreshApplyUpdatesChecks(t *testing.T) { + m := testModuleInline(t, map[string]string{ + "main.tf": ` +resource "test_object" "x" { + test_string = "ok" + lifecycle { + postcondition { + condition = self.test_string == "ok" + error_message = "wrong val" + } + } +} + +output "from_resource" { + value = test_object.x.test_string + precondition { + condition = test_object.x.test_string == "ok" + error_message = "wrong val" + } +} +`}) + + p := simpleMockProvider() + p.ReadResourceResponse = &providers.ReadResourceResponse{ + NewState: cty.ObjectVal(map[string]cty.Value{ + "test_string": cty.StringVal("ok"), + }), + } + + state := states.NewState() + mod := state.EnsureModule(addrs.RootModuleInstance) + mod.SetResourceInstanceCurrent( + mustResourceInstanceAddr("test_object.x").Resource, + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"test_string":"wrong val"}`), + }, + mustProviderConfig(`provider["registry.terraform.io/hashicorp/test"]`), + ) + mod.SetOutputValue("from_resource", cty.StringVal("wrong val"), false) + + ctx := testContext2(t, &ContextOpts{ + Providers: map[addrs.Provider]providers.Factory{ + addrs.NewDefaultProvider("test"): testProviderFuncFixed(p), + }, + }) + + opts := SimplePlanOpts(plans.RefreshOnlyMode, nil) + plan, diags := ctx.Plan(m, state, opts) + assertNoErrors(t, diags) + + state, diags = ctx.Apply(plan, m) + assertNoErrors(t, diags) + + resCheck := state.CheckResults.GetObjectResult(mustResourceInstanceAddr("test_object.x")) + if resCheck.Status != checks.StatusPass { + t.Fatalf("unexpected check %s: %s\n", resCheck.Status, resCheck.FailureMessages) + } + + outAddr := addrs.AbsOutputValue{ + Module: addrs.RootModuleInstance, + OutputValue: addrs.OutputValue{ + Name: "from_resource", + }, + } + outCheck := state.CheckResults.GetObjectResult(outAddr) + if outCheck.Status != checks.StatusPass { + t.Fatalf("unexpected check %s: %s\n", outCheck.Status, outCheck.FailureMessages) + } +} From fa4c652013a26bf29f924964fc669ac89e6ed961 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 28 Oct 2022 16:12:06 -0400 Subject: [PATCH 2/7] changes are mutated during apply --- internal/terraform/context_apply.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/internal/terraform/context_apply.go b/internal/terraform/context_apply.go index adb363a3e6a0..8631f04c30a0 100644 --- a/internal/terraform/context_apply.go +++ b/internal/terraform/context_apply.go @@ -25,6 +25,10 @@ func (c *Context) Apply(plan *plans.Plan, config *configs.Config) (*states.State log.Printf("[DEBUG] Building and walking apply graph for %s plan", plan.UIMode) + // FIXME: refresh plans still store outputs as changes, so we can't use + // Empty() + possibleRefresh := len(plan.Changes.Resources) == 0 + graph, operation, diags := c.applyGraph(plan, config, true) if diags.HasErrors() { return nil, diags @@ -77,7 +81,7 @@ Note that the -target option is not suitable for routine use, and is provided on // cleanup is going to be needed to make the plan state match what apply // would do. For now we can copy the checks over which were overwritten // during the apply walk. - if len(plan.Changes.Resources) == 0 { + if possibleRefresh { newState.CheckResults = plan.Checks.DeepCopy() } From eae246cfb572a19a53a7c22c518535ac0bb57115 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 26 Oct 2022 10:33:21 -0400 Subject: [PATCH 3/7] normalize empty CheckResults fields in stateV4 Ensure that empty check results are normalized in state serialization to prevent unexpected state changes from being written. Because there is no consistent empty, null and omit_empty usage for state structs, there's no good way to create a test which will fail for future additions. --- internal/states/statefile/version4.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/internal/states/statefile/version4.go b/internal/states/statefile/version4.go index cb3dd694dd3d..2cdde4be7107 100644 --- a/internal/states/statefile/version4.go +++ b/internal/states/statefile/version4.go @@ -414,9 +414,7 @@ func writeStateV4(file *File, w io.Writer) tfdiags.Diagnostics { } } - if file.State.CheckResults != nil { - sV4.CheckResults = encodeCheckResultsV4(file.State.CheckResults) - } + sV4.CheckResults = encodeCheckResultsV4(file.State.CheckResults) sV4.normalize() @@ -576,6 +574,11 @@ func decodeCheckResultsV4(in []checkResultsV4) (*states.CheckResults, tfdiags.Di } func encodeCheckResultsV4(in *states.CheckResults) []checkResultsV4 { + // normalize empty and nil sets in the serialized state + if in == nil || in.ConfigResults.Len() == 0 { + return nil + } + ret := make([]checkResultsV4, 0, in.ConfigResults.Len()) for _, configElem := range in.ConfigResults.Elems { From 19152e7ba522f6b91f1778ca695015782d20cd30 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 26 Oct 2022 10:38:01 -0400 Subject: [PATCH 4/7] fix log mesage --- internal/terraform/transform_reference.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/terraform/transform_reference.go b/internal/terraform/transform_reference.go index 7c45955b86f8..fb8feb2f2a7f 100644 --- a/internal/terraform/transform_reference.go +++ b/internal/terraform/transform_reference.go @@ -136,7 +136,7 @@ func (t *ReferenceTransformer) Transform(g *Graph) error { if !graphNodesAreResourceInstancesInDifferentInstancesOfSameModule(v, parent) { g.Connect(dag.BasicEdge(v, parent)) } else { - log.Printf("[TRACE] ReferenceTransformer: skipping %s => %s inter-module-instance dependency", v, parent) + log.Printf("[TRACE] ReferenceTransformer: skipping %s => %s inter-module-instance dependency", dag.VertexName(v), dag.VertexName(parent)) } } From eb88ccbc7b5dbd223f2e8df6c1a571b42fca633d Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 26 Oct 2022 12:04:08 -0400 Subject: [PATCH 5/7] only add NoOp nodes with conditions ONly add NoOp changes to the apply graph if they have conditions which need to be evaluated. --- internal/terraform/graph_builder_apply.go | 1 + internal/terraform/transform_diff.go | 26 ++++++++++++++- internal/terraform/transform_diff_test.go | 39 +++++++++++++++++++++++ 3 files changed, 65 insertions(+), 1 deletion(-) diff --git a/internal/terraform/graph_builder_apply.go b/internal/terraform/graph_builder_apply.go index 4d7763d2a5f3..22f1b8c3bc10 100644 --- a/internal/terraform/graph_builder_apply.go +++ b/internal/terraform/graph_builder_apply.go @@ -107,6 +107,7 @@ func (b *ApplyGraphBuilder) Steps() []GraphTransformer { Concrete: concreteResourceInstance, State: b.State, Changes: b.Changes, + Config: b.Config, }, // Attach the state diff --git a/internal/terraform/transform_diff.go b/internal/terraform/transform_diff.go index bfabf60e9fac..3bce1131f5a5 100644 --- a/internal/terraform/transform_diff.go +++ b/internal/terraform/transform_diff.go @@ -4,6 +4,8 @@ import ( "fmt" "log" + "github.com/hashicorp/terraform/internal/addrs" + "github.com/hashicorp/terraform/internal/configs" "github.com/hashicorp/terraform/internal/dag" "github.com/hashicorp/terraform/internal/plans" "github.com/hashicorp/terraform/internal/states" @@ -16,6 +18,28 @@ type DiffTransformer struct { Concrete ConcreteResourceInstanceNodeFunc State *states.State Changes *plans.Changes + Config *configs.Config +} + +// return true if the given resource instance has either Preconditions or +// Postconditions defined in the configuration. +func (t *DiffTransformer) hasConfigConditions(addr addrs.AbsResourceInstance) bool { + // unit tests may have no config + if t.Config == nil { + return false + } + + cfg := t.Config.DescendentForInstance(addr.Module) + if cfg == nil { + return false + } + + res := cfg.Module.ResourceByAddr(addr.ConfigResource().Resource) + if res == nil { + return false + } + + return len(res.Preconditions) > 0 || len(res.Postconditions) > 0 } func (t *DiffTransformer) Transform(g *Graph) error { @@ -69,7 +93,7 @@ func (t *DiffTransformer) Transform(g *Graph) error { // run any condition checks associated with the object, to // make sure that they still hold when considering the // results of other changes. - update = true + update = t.hasConfigConditions(addr) case plans.Delete: delete = true case plans.DeleteThenCreate, plans.CreateThenDelete: diff --git a/internal/terraform/transform_diff_test.go b/internal/terraform/transform_diff_test.go index c7cddc452fb2..fa1feae34d61 100644 --- a/internal/terraform/transform_diff_test.go +++ b/internal/terraform/transform_diff_test.go @@ -81,6 +81,26 @@ func TestDiffTransformer_noOpChange(t *testing.T) { // results changed even if the resource instance they are attached to // didn't actually change directly itself. + // aws_instance.foo has a precondition, so should be included in the final + // graph. aws_instance.bar has no conditions, so there is nothing to + // execute during apply and it should not be included in the graph. + m := testModuleInline(t, map[string]string{ + "main.tf": ` +resource "aws_instance" "bar" { +} + +resource "aws_instance" "foo" { + test_string = "ok" + + lifecycle { + precondition { + condition = self.test_string != "" + error_message = "resource error" + } + } +} +`}) + g := Graph{Path: addrs.RootModuleInstance} beforeVal, err := plans.NewDynamicValue(cty.StringVal(""), cty.String) @@ -89,6 +109,7 @@ func TestDiffTransformer_noOpChange(t *testing.T) { } tf := &DiffTransformer{ + Config: m, Changes: &plans.Changes{ Resources: []*plans.ResourceInstanceChangeSrc{ { @@ -109,6 +130,24 @@ func TestDiffTransformer_noOpChange(t *testing.T) { After: beforeVal, }, }, + { + Addr: addrs.Resource{ + Mode: addrs.ManagedResourceMode, + Type: "aws_instance", + Name: "bar", + }.Instance(addrs.NoKey).Absolute(addrs.RootModuleInstance), + ProviderAddr: addrs.AbsProviderConfig{ + Provider: addrs.NewDefaultProvider("aws"), + Module: addrs.RootModule, + }, + ChangeSrc: plans.ChangeSrc{ + // A "no-op" change has the no-op action and has the + // same object as both Before and After. + Action: plans.NoOp, + Before: beforeVal, + After: beforeVal, + }, + }, }, }, } From ffe2e3935e35f9c8e1db6e462d3d0f465309a28c Mon Sep 17 00:00:00 2001 From: James Bardin Date: Mon, 31 Oct 2022 10:10:12 -0400 Subject: [PATCH 6/7] avoid re-writing state for noop applies We need to avoid re-writing the state for every NoOp apply. We may still be evaluating the instance to account for any side-effects in the condition checks, however the state of the instance has not changes. Re-writing the state is a non-current operation, which may require encoding a fairly large instance state and re-serializing the entire state blob, so it is best avoided if possible. --- internal/terraform/context_apply2_test.go | 71 +++++++++++++++++++ .../node_resource_abstract_instance.go | 45 ++++++------ .../terraform/node_resource_apply_instance.go | 24 +++++-- internal/terraform/node_resource_destroy.go | 3 +- .../node_resource_destroy_deposed.go | 3 +- 5 files changed, 118 insertions(+), 28 deletions(-) diff --git a/internal/terraform/context_apply2_test.go b/internal/terraform/context_apply2_test.go index e7261640b38e..9000364ed39b 100644 --- a/internal/terraform/context_apply2_test.go +++ b/internal/terraform/context_apply2_test.go @@ -1713,3 +1713,74 @@ output "from_resource" { t.Fatalf("unexpected check %s: %s\n", outCheck.Status, outCheck.FailureMessages) } } + +// NoOp changes may have conditions to evaluate, but should not re-plan and +// apply the entire resource. +func TestContext2Apply_noRePlanNoOp(t *testing.T) { + m := testModuleInline(t, map[string]string{ + "main.tf": ` +resource "test_object" "x" { +} + +resource "test_object" "y" { + # test_object.w is being re-created, so this precondition must be evaluated + # during apply, however this resource should otherwise be a NoOp. + lifecycle { + precondition { + condition = test_object.x.test_string == null + error_message = "test_object.x.test_string should be null" + } + } +} +`}) + + p := simpleMockProvider() + // make sure we can compute the attr + testString := p.GetProviderSchemaResponse.ResourceTypes["test_object"].Block.Attributes["test_string"] + testString.Computed = true + testString.Optional = false + + yAddr := mustResourceInstanceAddr("test_object.y") + + state := states.NewState() + mod := state.RootModule() + mod.SetResourceInstanceCurrent( + yAddr.Resource, + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"test_string":"y"}`), + }, + mustProviderConfig(`provider["registry.terraform.io/hashicorp/test"]`), + ) + + ctx := testContext2(t, &ContextOpts{ + Providers: map[addrs.Provider]providers.Factory{ + addrs.NewDefaultProvider("test"): testProviderFuncFixed(p), + }, + }) + + opts := SimplePlanOpts(plans.NormalMode, nil) + plan, diags := ctx.Plan(m, state, opts) + assertNoErrors(t, diags) + + for _, c := range plan.Changes.Resources { + if c.Addr.Equal(yAddr) && c.Action != plans.NoOp { + t.Fatalf("unexpected %s change for test_object.y", c.Action) + } + } + + // test_object.y is a NoOp change from the plan, but is included in the + // graph due to the conditions which must be evaluated. This however should + // not cause the resource to be re-planned. + p.PlanResourceChangeFn = func(req providers.PlanResourceChangeRequest) (resp providers.PlanResourceChangeResponse) { + testString := req.ProposedNewState.GetAttr("test_string") + if !testString.IsNull() && testString.AsString() == "y" { + resp.Diagnostics = resp.Diagnostics.Append(errors.New("Unexpected apply-time plan for test_object.y. Original plan was a NoOp")) + } + resp.PlannedState = req.ProposedNewState + return resp + } + + _, diags = ctx.Apply(plan, m) + assertNoErrors(t, diags) +} diff --git a/internal/terraform/node_resource_abstract_instance.go b/internal/terraform/node_resource_abstract_instance.go index ba1aa3a17c60..659ff5ff5397 100644 --- a/internal/terraform/node_resource_abstract_instance.go +++ b/internal/terraform/node_resource_abstract_instance.go @@ -715,6 +715,13 @@ func (n *NodeAbstractResourceInstance) plan( return plan, state, keyData, diags // failed preconditions prevent further evaluation } + // If we have a previous plan and the action was a noop, then the only + // reason we're in this method was to evaluate the preconditions. There's + // no need to re-plan this resource. + if plannedChange != nil && plannedChange.Action == plans.NoOp { + return plannedChange, currentState.DeepCopy(), keyData, diags + } + origConfigVal, _, configDiags := ctx.EvaluateBlock(config.Config, schema, nil, keyData) diags = diags.Append(configDiags) if configDiags.HasErrors() { @@ -2059,44 +2066,38 @@ func (n *NodeAbstractResourceInstance) evalDestroyProvisionerConfig(ctx EvalCont } // apply accepts an applyConfig, instead of using n.Config, so destroy plans can -// send a nil config. Most of the errors generated in apply are returned as -// diagnostics, but if provider.ApplyResourceChange itself fails, that error is -// returned as an error and nil diags are returned. +// send a nil config. The keyData information can be empty if the config is +// nil, since it is only used to evaluate the configuration. func (n *NodeAbstractResourceInstance) apply( ctx EvalContext, state *states.ResourceInstanceObject, change *plans.ResourceInstanceChange, applyConfig *configs.Resource, - createBeforeDestroy bool) (*states.ResourceInstanceObject, instances.RepetitionData, tfdiags.Diagnostics) { + keyData instances.RepetitionData, + createBeforeDestroy bool) (*states.ResourceInstanceObject, tfdiags.Diagnostics) { var diags tfdiags.Diagnostics - var keyData instances.RepetitionData if state == nil { state = &states.ResourceInstanceObject{} } - if applyConfig != nil { - forEach, _ := evaluateForEachExpression(applyConfig.ForEach, ctx) - keyData = EvalDataForInstanceKey(n.ResourceInstanceAddr().Resource.Key, forEach) - } - if change.Action == plans.NoOp { // If this is a no-op change then we don't want to actually change // anything, so we'll just echo back the state we were given and // let our internal checks and updates proceed. log.Printf("[TRACE] NodeAbstractResourceInstance.apply: skipping %s because it has no planned action", n.Addr) - return state, keyData, diags + return state, diags } provider, providerSchema, err := getProvider(ctx, n.ResolvedProvider) if err != nil { - return nil, keyData, diags.Append(err) + return nil, diags.Append(err) } schema, _ := providerSchema.SchemaForResourceType(n.Addr.Resource.Resource.Mode, n.Addr.Resource.Resource.Type) if schema == nil { // Should be caught during validation, so we don't bother with a pretty error here diags = diags.Append(fmt.Errorf("provider does not support resource type %q", n.Addr.Resource.Resource.Type)) - return nil, keyData, diags + return nil, diags } log.Printf("[INFO] Starting apply for %s", n.Addr) @@ -2107,7 +2108,7 @@ func (n *NodeAbstractResourceInstance) apply( configVal, _, configDiags = ctx.EvaluateBlock(applyConfig.Config, schema, nil, keyData) diags = diags.Append(configDiags) if configDiags.HasErrors() { - return nil, keyData, diags + return nil, diags } } @@ -2132,13 +2133,13 @@ func (n *NodeAbstractResourceInstance) apply( strings.Join(unknownPaths, "\n"), ), )) - return nil, keyData, diags + return nil, diags } metaConfigVal, metaDiags := n.providerMetas(ctx) diags = diags.Append(metaDiags) if diags.HasErrors() { - return nil, keyData, diags + return nil, diags } log.Printf("[DEBUG] %s: applying the planned %s change", n.Addr, change.Action) @@ -2166,7 +2167,7 @@ func (n *NodeAbstractResourceInstance) apply( Status: state.Status, Value: change.After, } - return newState, keyData, diags + return newState, diags } resp := provider.ApplyResourceChange(providers.ApplyResourceChangeRequest{ @@ -2237,7 +2238,7 @@ func (n *NodeAbstractResourceInstance) apply( // Bail early in this particular case, because an object that doesn't // conform to the schema can't be saved in the state anyway -- the // serializer will reject it. - return nil, keyData, diags + return nil, diags } // After this point we have a type-conforming result object and so we @@ -2363,7 +2364,7 @@ func (n *NodeAbstractResourceInstance) apply( // prior state as the new value, making this effectively a no-op. If // the item really _has_ been deleted then our next refresh will detect // that and fix it up. - return state.DeepCopy(), keyData, diags + return state.DeepCopy(), diags case diags.HasErrors() && !newVal.IsNull(): // if we have an error, make sure we restore the object status in the new state @@ -2380,7 +2381,7 @@ func (n *NodeAbstractResourceInstance) apply( newState.Dependencies = state.Dependencies } - return newState, keyData, diags + return newState, diags case !newVal.IsNull(): // Non error case with a new state @@ -2390,11 +2391,11 @@ func (n *NodeAbstractResourceInstance) apply( Private: resp.Private, CreateBeforeDestroy: createBeforeDestroy, } - return newState, keyData, diags + return newState, diags default: // Non error case, were the object was deleted - return nil, keyData, diags + return nil, diags } } diff --git a/internal/terraform/node_resource_apply_instance.go b/internal/terraform/node_resource_apply_instance.go index f257c3f7d0c4..be6531924005 100644 --- a/internal/terraform/node_resource_apply_instance.go +++ b/internal/terraform/node_resource_apply_instance.go @@ -6,6 +6,7 @@ import ( "github.com/hashicorp/terraform/internal/addrs" "github.com/hashicorp/terraform/internal/configs" + "github.com/hashicorp/terraform/internal/instances" "github.com/hashicorp/terraform/internal/plans" "github.com/hashicorp/terraform/internal/plans/objchange" "github.com/hashicorp/terraform/internal/states" @@ -295,7 +296,19 @@ func (n *NodeApplyableResourceInstance) managedResourceExecute(ctx EvalContext) return diags } - state, repeatData, applyDiags := n.apply(ctx, state, diffApply, n.Config, n.CreateBeforeDestroy()) + var repeatData instances.RepetitionData + if n.Config != nil { + forEach, _ := evaluateForEachExpression(n.Config.ForEach, ctx) + repeatData = EvalDataForInstanceKey(n.ResourceInstanceAddr().Resource.Key, forEach) + } + + // If there is no change, there was nothing to apply, and we don't need to + // re-write the state, but we do need to re-evaluate postconditions. + if diffApply.Action == plans.NoOp { + return diags.Append(n.managedResourcePostconditions(ctx, repeatData)) + } + + state, applyDiags := n.apply(ctx, state, diffApply, n.Config, repeatData, n.CreateBeforeDestroy()) diags = diags.Append(applyDiags) // We clear the change out here so that future nodes don't see a change @@ -370,15 +383,18 @@ func (n *NodeApplyableResourceInstance) managedResourceExecute(ctx EvalContext) // _after_ writing the state because we want to check against // the result of the operation, and to fail on future operations // until the user makes the condition succeed. + return diags.Append(n.managedResourcePostconditions(ctx, repeatData)) +} + +func (n *NodeApplyableResourceInstance) managedResourcePostconditions(ctx EvalContext, repeatData instances.RepetitionData) (diags tfdiags.Diagnostics) { + checkDiags := evalCheckRules( addrs.ResourcePostcondition, n.Config.Postconditions, ctx, n.ResourceInstanceAddr(), repeatData, tfdiags.Error, ) - diags = diags.Append(checkDiags) - - return diags + return diags.Append(checkDiags) } // checkPlannedChange produces errors if the _actual_ expected value is not diff --git a/internal/terraform/node_resource_destroy.go b/internal/terraform/node_resource_destroy.go index d0cc7276bb2a..a468ad020ba1 100644 --- a/internal/terraform/node_resource_destroy.go +++ b/internal/terraform/node_resource_destroy.go @@ -4,6 +4,7 @@ import ( "fmt" "log" + "github.com/hashicorp/terraform/internal/instances" "github.com/hashicorp/terraform/internal/plans" "github.com/hashicorp/terraform/internal/tfdiags" @@ -210,7 +211,7 @@ func (n *NodeDestroyResourceInstance) managedResourceExecute(ctx EvalContext) (d // Managed resources need to be destroyed, while data sources // are only removed from state. // we pass a nil configuration to apply because we are destroying - s, _, d := n.apply(ctx, state, changeApply, nil, false) + s, d := n.apply(ctx, state, changeApply, nil, instances.RepetitionData{}, false) state, diags = s, diags.Append(d) // we don't return immediately here on error, so that the state can be // finalized diff --git a/internal/terraform/node_resource_destroy_deposed.go b/internal/terraform/node_resource_destroy_deposed.go index 2c042386a380..702e0596c124 100644 --- a/internal/terraform/node_resource_destroy_deposed.go +++ b/internal/terraform/node_resource_destroy_deposed.go @@ -6,6 +6,7 @@ import ( "github.com/hashicorp/terraform/internal/addrs" "github.com/hashicorp/terraform/internal/dag" + "github.com/hashicorp/terraform/internal/instances" "github.com/hashicorp/terraform/internal/plans" "github.com/hashicorp/terraform/internal/states" "github.com/hashicorp/terraform/internal/tfdiags" @@ -249,7 +250,7 @@ func (n *NodeDestroyDeposedResourceInstanceObject) Execute(ctx EvalContext, op w } // we pass a nil configuration to apply because we are destroying - state, _, applyDiags := n.apply(ctx, state, change, nil, false) + state, applyDiags := n.apply(ctx, state, change, nil, instances.RepetitionData{}, false) diags = diags.Append(applyDiags) // don't return immediately on errors, we need to handle the state From efd77159ddcd1938a317d04085f9c8c9d116e865 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Mon, 31 Oct 2022 11:00:07 -0400 Subject: [PATCH 7/7] use key data from plan method for apply --- internal/terraform/node_resource_apply_instance.go | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/internal/terraform/node_resource_apply_instance.go b/internal/terraform/node_resource_apply_instance.go index be6531924005..82aebec296c4 100644 --- a/internal/terraform/node_resource_apply_instance.go +++ b/internal/terraform/node_resource_apply_instance.go @@ -271,7 +271,7 @@ func (n *NodeApplyableResourceInstance) managedResourceExecute(ctx EvalContext) // Make a new diff, in case we've learned new values in the state // during apply which we can now incorporate. - diffApply, _, _, planDiags := n.plan(ctx, diff, state, false, n.forceReplace) + diffApply, _, repeatData, planDiags := n.plan(ctx, diff, state, false, n.forceReplace) diags = diags.Append(planDiags) if diags.HasErrors() { return diags @@ -296,12 +296,6 @@ func (n *NodeApplyableResourceInstance) managedResourceExecute(ctx EvalContext) return diags } - var repeatData instances.RepetitionData - if n.Config != nil { - forEach, _ := evaluateForEachExpression(n.Config.ForEach, ctx) - repeatData = EvalDataForInstanceKey(n.ResourceInstanceAddr().Resource.Key, forEach) - } - // If there is no change, there was nothing to apply, and we don't need to // re-write the state, but we do need to re-evaluate postconditions. if diffApply.Action == plans.NoOp {