From dc668dff386b70a8c151ded6cd1af5ff32867956 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 8 Mar 2022 13:06:30 -0500 Subject: [PATCH 1/2] ensure UI hooks are called for data sources The UI hooks for data source reads were missed during planning. Move the hook calls to immediatley before and after the ReadDataSource calls to ensure they are called during both plan and apply. --- internal/terraform/context_apply_test.go | 9 ++++++++ .../node_resource_abstract_instance.go | 22 +++++++++---------- 2 files changed, 20 insertions(+), 11 deletions(-) diff --git a/internal/terraform/context_apply_test.go b/internal/terraform/context_apply_test.go index 46dcbd58b..5963b2242 100644 --- a/internal/terraform/context_apply_test.go +++ b/internal/terraform/context_apply_test.go @@ -1428,7 +1428,9 @@ func TestContext2Apply_dataBasic(t *testing.T) { }), } + hook := new(MockHook) ctx := testContext2(t, &ContextOpts{ + Hooks: []Hook{hook}, Providers: map[addrs.Provider]providers.Factory{ addrs.NewDefaultProvider("null"): testProviderFuncFixed(p), }, @@ -1449,6 +1451,13 @@ func TestContext2Apply_dataBasic(t *testing.T) { if actual != expected { t.Fatalf("wrong result\n\ngot:\n%s\n\nwant:\n%s", actual, expected) } + + if !hook.PreApplyCalled { + t.Fatal("PreApply not called for data source read") + } + if !hook.PostApplyCalled { + t.Fatal("PostApply not called for data source read") + } } func TestContext2Apply_destroyData(t *testing.T) { diff --git a/internal/terraform/node_resource_abstract_instance.go b/internal/terraform/node_resource_abstract_instance.go index fb431df53..67298b65a 100644 --- a/internal/terraform/node_resource_abstract_instance.go +++ b/internal/terraform/node_resource_abstract_instance.go @@ -1381,6 +1381,13 @@ func (n *NodeAbstractResourceInstance) readDataSource(ctx EvalContext, configVal // to actually call the provider to read the data. log.Printf("[TRACE] readDataSource: %s configuration is complete, so reading from provider", n.Addr) + diags = diags.Append(ctx.Hook(func(h Hook) (HookAction, error) { + return h.PreApply(n.Addr, states.CurrentGen, plans.Read, cty.NullVal(configVal.Type()), configVal) + })) + if diags.HasErrors() { + return newVal, diags + } + resp := provider.ReadDataSource(providers.ReadDataSourceRequest{ TypeName: n.Addr.ContainingResource().Resource.Type, Config: configVal, @@ -1445,6 +1452,10 @@ func (n *NodeAbstractResourceInstance) readDataSource(ctx EvalContext, configVal newVal = newVal.MarkWithPaths(pvm) } + diags = diags.Append(ctx.Hook(func(h Hook) (HookAction, error) { + return h.PostApply(n.Addr, states.CurrentGen, newVal, diags.Err()) + })) + return newVal, diags } @@ -1703,13 +1714,6 @@ func (n *NodeAbstractResourceInstance) applyDataSource(ctx EvalContext, planned return nil, keyData, diags } - diags = diags.Append(ctx.Hook(func(h Hook) (HookAction, error) { - return h.PreApply(n.Addr, states.CurrentGen, planned.Action, planned.Before, planned.After) - })) - if diags.HasErrors() { - return nil, keyData, diags - } - config := *n.Config schema, _ := providerSchema.SchemaForResourceAddr(n.Addr.ContainingResource().Resource) if schema == nil { @@ -1751,10 +1755,6 @@ func (n *NodeAbstractResourceInstance) applyDataSource(ctx EvalContext, planned Status: states.ObjectReady, } - diags = diags.Append(ctx.Hook(func(h Hook) (HookAction, error) { - return h.PostApply(n.Addr, states.CurrentGen, newVal, diags.Err()) - })) - return state, keyData, diags } From 05a10f06d11ad3c2d6bceb1287e829f786f8becf Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 8 Mar 2022 13:48:41 -0500 Subject: [PATCH 2/2] remove PreDiff and PostDiff hook calls PreDiff and PostDiff hooks were designed to be called immediately before and after the PlanResourceChange calls to the provider. Probably due to the confusing legacy naming of the hooks, these were scattered about the nodes involved with planning, causing the hooks to be called in a number of places where they were designed, including data sources and destroy plans. Since these hooks are not used at all any longer anyway, we can removed the extra calls with no effect. If we choose in the future to call PlanResourceChange for resource destroy plans, the hooks can be re-inserted (even though they currently are unused) into the new code path which must diverge from the current combined path of managed and data sources. --- internal/terraform/context_apply_test.go | 6 +-- .../node_resource_abstract_instance.go | 40 ------------------- 2 files changed, 2 insertions(+), 44 deletions(-) diff --git a/internal/terraform/context_apply_test.go b/internal/terraform/context_apply_test.go index 5963b2242..64f357700 100644 --- a/internal/terraform/context_apply_test.go +++ b/internal/terraform/context_apply_test.go @@ -1512,10 +1512,8 @@ func TestContext2Apply_destroyData(t *testing.T) { } wantHookCalls := []*testHookCall{ - {"PreDiff", "data.null_data_source.testing"}, - {"PostDiff", "data.null_data_source.testing"}, - {"PreDiff", "data.null_data_source.testing"}, - {"PostDiff", "data.null_data_source.testing"}, + {"PreApply", "data.null_data_source.testing"}, + {"PostApply", "data.null_data_source.testing"}, {"PostStateUpdate", ""}, } if !reflect.DeepEqual(hook.Calls, wantHookCalls) { diff --git a/internal/terraform/node_resource_abstract_instance.go b/internal/terraform/node_resource_abstract_instance.go index 67298b65a..f0bedd11b 100644 --- a/internal/terraform/node_resource_abstract_instance.go +++ b/internal/terraform/node_resource_abstract_instance.go @@ -410,18 +410,6 @@ func (n *NodeAbstractResourceInstance) planDestroy(ctx EvalContext, currentState return noop, nil } - // Call pre-diff hook - diags = diags.Append(ctx.Hook(func(h Hook) (HookAction, error) { - return h.PreDiff( - absAddr, deposedKey.Generation(), - currentState.Value, - cty.NullVal(cty.DynamicPseudoType), - ) - })) - if diags.HasErrors() { - return nil, diags - } - // Plan is always the same for a destroy. We don't need the provider's // help for this one. plan := &plans.ResourceInstanceChange{ @@ -437,17 +425,6 @@ func (n *NodeAbstractResourceInstance) planDestroy(ctx EvalContext, currentState ProviderAddr: n.ResolvedProvider, } - // Call post-diff hook - diags = diags.Append(ctx.Hook(func(h Hook) (HookAction, error) { - return h.PostDiff( - absAddr, - deposedKey.Generation(), - plan.Action, - plan.Before, - plan.After, - ) - })) - return plan, diags } @@ -1567,13 +1544,6 @@ func (n *NodeAbstractResourceInstance) planDataSource(ctx EvalContext, currentSt } proposedNewVal := objchange.PlannedDataResourceObject(schema, unmarkedConfigVal) - - diags = diags.Append(ctx.Hook(func(h Hook) (HookAction, error) { - return h.PreDiff(n.Addr, states.CurrentGen, priorVal, proposedNewVal) - })) - if diags.HasErrors() { - return nil, nil, keyData, diags - } proposedNewVal = proposedNewVal.MarkWithPaths(configMarkPaths) // Apply detects that the data source will need to be read by the After @@ -1601,13 +1571,6 @@ func (n *NodeAbstractResourceInstance) planDataSource(ctx EvalContext, currentSt return plannedChange, plannedNewState, keyData, diags } - // While this isn't a "diff", continue to call this for data sources. - diags = diags.Append(ctx.Hook(func(h Hook) (HookAction, error) { - return h.PreDiff(n.Addr, states.CurrentGen, priorVal, configVal) - })) - if diags.HasErrors() { - return nil, nil, keyData, diags - } // We have a complete configuration with no dependencies to wait on, so we // can read the data source into the state. newVal, readDiags := n.readDataSource(ctx, configVal) @@ -1643,9 +1606,6 @@ func (n *NodeAbstractResourceInstance) planDataSource(ctx EvalContext, currentSt Status: states.ObjectReady, } - diags = diags.Append(ctx.Hook(func(h Hook) (HookAction, error) { - return h.PostDiff(n.Addr, states.CurrentGen, plans.Update, priorVal, newVal) - })) return nil, plannedNewState, keyData, diags }