From be757bd41657a758c9a6edffe3f0b6486a79e8d3 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Mon, 27 Jul 2020 16:35:55 -0400 Subject: [PATCH 01/20] Refresh instances during plan This change refreshes the instance state during plan, so a complete Refresh no longer needs to happen before Plan. --- backend/local/backend_apply.go | 12 ------ backend/local/backend_plan.go | 26 ++----------- terraform/node_resource_plan_instance.go | 47 +++++++++++++++++------- 3 files changed, 37 insertions(+), 48 deletions(-) diff --git a/backend/local/backend_apply.go b/backend/local/backend_apply.go index bfcf7f506a..d754573d47 100644 --- a/backend/local/backend_apply.go +++ b/backend/local/backend_apply.go @@ -80,18 +80,6 @@ func (b *Local) opApply( // If we weren't given a plan, then we refresh/plan if op.PlanFile == nil { - // If we're refreshing before apply, perform that - if op.PlanRefresh { - log.Printf("[INFO] backend/local: apply calling Refresh") - _, refreshDiags := tfCtx.Refresh() - diags = diags.Append(refreshDiags) - if diags.HasErrors() { - runningOp.Result = backend.OperationFailure - b.ShowDiagnostics(diags) - return - } - } - // Perform the plan log.Printf("[INFO] backend/local: apply calling Plan") plan, planDiags := tfCtx.Plan() diff --git a/backend/local/backend_plan.go b/backend/local/backend_plan.go index 4ce5f893c5..da82e31163 100644 --- a/backend/local/backend_plan.go +++ b/backend/local/backend_plan.go @@ -97,27 +97,6 @@ func (b *Local) opPlan( runningOp.State = tfCtx.State() - // If we're refreshing before plan, perform that - baseState := runningOp.State - if op.PlanRefresh { - log.Printf("[INFO] backend/local: plan calling Refresh") - - if b.CLI != nil { - b.CLI.Output(b.Colorize().Color(strings.TrimSpace(planRefreshing) + "\n")) - } - - refreshedState, refreshDiags := tfCtx.Refresh() - diags = diags.Append(refreshDiags) - if diags.HasErrors() { - b.ReportResult(runningOp, diags) - return - } - baseState = refreshedState // plan will be relative to our refreshed state - if b.CLI != nil { - b.CLI.Output("\n------------------------------------------------------------------------") - } - } - // Perform the plan in a goroutine so we can be interrupted var plan *plans.Plan var planDiags tfdiags.Diagnostics @@ -142,6 +121,7 @@ func (b *Local) opPlan( b.ReportResult(runningOp, diags) return } + // Record whether this plan includes any side-effects that could be applied. runningOp.PlanEmpty = !planHasSideEffects(priorState, plan.Changes) @@ -161,7 +141,7 @@ func (b *Local) opPlan( // We may have updated the state in the refresh step above, but we // will freeze that updated state in the plan file for now and // only write it if this plan is subsequently applied. - plannedStateFile := statemgr.PlannedStateUpdate(opState, baseState) + plannedStateFile := statemgr.PlannedStateUpdate(opState, plan.State) log.Printf("[INFO] backend/local: writing plan output to: %s", path) err := planfile.Create(path, configSnap, plannedStateFile, plan) @@ -187,7 +167,7 @@ func (b *Local) opPlan( return } - b.renderPlan(plan, baseState, priorState, schemas) + b.renderPlan(plan, plan.State, priorState, schemas) // If we've accumulated any warnings along the way then we'll show them // here just before we show the summary and next steps. If we encountered diff --git a/terraform/node_resource_plan_instance.go b/terraform/node_resource_plan_instance.go index 4b9c2bde44..2071c5278e 100644 --- a/terraform/node_resource_plan_instance.go +++ b/terraform/node_resource_plan_instance.go @@ -63,8 +63,7 @@ func (n *NodePlannableResourceInstance) evalTreeDataResource(addr addrs.AbsResou Addr: addr.Resource, Provider: &provider, ProviderSchema: &providerSchema, - - Output: &state, + Output: &state, }, &EvalValidateSelfRef{ @@ -108,7 +107,8 @@ func (n *NodePlannableResourceInstance) evalTreeManagedResource(addr addrs.AbsRe var provider providers.Interface var providerSchema *ProviderSchema var change *plans.ResourceInstanceChange - var state *states.ResourceInstanceObject + var refreshState *states.ResourceInstanceObject + var planState *states.ResourceInstanceObject return &EvalSequence{ Nodes: []EvalNode{ @@ -118,19 +118,40 @@ func (n *NodePlannableResourceInstance) evalTreeManagedResource(addr addrs.AbsRe Schema: &providerSchema, }, - &EvalReadState{ - Addr: addr.Resource, - Provider: &provider, - ProviderSchema: &providerSchema, - Output: &state, - }, - &EvalValidateSelfRef{ Addr: addr.Resource, Config: config.Config, ProviderSchema: &providerSchema, }, + // Refresh the instance + &EvalReadState{ + Addr: addr.Resource, + Provider: &provider, + ProviderSchema: &providerSchema, + Output: &refreshState, + }, + &EvalRefreshDependencies{ + State: &refreshState, + Dependencies: &n.Dependencies, + }, + &EvalRefresh{ + Addr: addr.Resource, + ProviderAddr: n.ResolvedProvider, + Provider: &provider, + ProviderMetas: n.ProviderMetas, + ProviderSchema: &providerSchema, + State: &refreshState, + Output: &refreshState, + }, + &EvalWriteState{ + Addr: addr.Resource, + ProviderAddr: n.ResolvedProvider, + State: &refreshState, + ProviderSchema: &providerSchema, + }, + + // Plan the instance &EvalDiff{ Addr: addr.Resource, Config: n.Config, @@ -139,9 +160,9 @@ func (n *NodePlannableResourceInstance) evalTreeManagedResource(addr addrs.AbsRe ProviderAddr: n.ResolvedProvider, ProviderMetas: n.ProviderMetas, ProviderSchema: &providerSchema, - State: &state, + State: &refreshState, OutputChange: &change, - OutputState: &state, + OutputState: &planState, }, &EvalCheckPreventDestroy{ Addr: addr.Resource, @@ -151,7 +172,7 @@ func (n *NodePlannableResourceInstance) evalTreeManagedResource(addr addrs.AbsRe &EvalWriteState{ Addr: addr.Resource, ProviderAddr: n.ResolvedProvider, - State: &state, + State: &planState, ProviderSchema: &providerSchema, }, &EvalWriteDiff{ From d6a586709cf8eee53b46b56bcf00fa7fa44d6441 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 4 Sep 2020 15:53:15 -0400 Subject: [PATCH 02/20] Add RefreshState to the eval context Since plan uses the state as a scratch space for evaluation, we need an entirely separate state to store the refreshed resources values during planning. Add a RefreshState method to the EvalContext to retrieve a state used only for refreshing resources. --- terraform/eval_context.go | 5 +++++ terraform/eval_context_builtin.go | 5 +++++ terraform/eval_context_mock.go | 8 ++++++++ 3 files changed, 18 insertions(+) diff --git a/terraform/eval_context.go b/terraform/eval_context.go index 1448ae93de..6c8a5d9d16 100644 --- a/terraform/eval_context.go +++ b/terraform/eval_context.go @@ -152,6 +152,11 @@ type EvalContext interface { // the global state. State() *states.SyncState + // RefreshState returns a wrapper object that provides safe concurrent + // access to the state used to store the most recently refreshed resource + // values. + RefreshState() *states.SyncState + // InstanceExpander returns a helper object for tracking the expansion of // graph nodes during the plan phase in response to "count" and "for_each" // arguments. diff --git a/terraform/eval_context_builtin.go b/terraform/eval_context_builtin.go index efb88b3103..17e84de6e5 100644 --- a/terraform/eval_context_builtin.go +++ b/terraform/eval_context_builtin.go @@ -71,6 +71,7 @@ type BuiltinEvalContext struct { ProvisionerLock *sync.Mutex ChangesValue *plans.ChangesSync StateValue *states.SyncState + RefreshStateValue *states.SyncState InstanceExpanderValue *instances.Expander } @@ -350,6 +351,10 @@ func (ctx *BuiltinEvalContext) State() *states.SyncState { return ctx.StateValue } +func (ctx *BuiltinEvalContext) RefreshState() *states.SyncState { + return ctx.RefreshStateValue +} + func (ctx *BuiltinEvalContext) InstanceExpander() *instances.Expander { return ctx.InstanceExpanderValue } diff --git a/terraform/eval_context_mock.go b/terraform/eval_context_mock.go index f97c3d08cd..11ae6941f5 100644 --- a/terraform/eval_context_mock.go +++ b/terraform/eval_context_mock.go @@ -126,6 +126,9 @@ type MockEvalContext struct { StateCalled bool StateState *states.SyncState + RefreshStateCalled bool + RefreshStateState *states.SyncState + InstanceExpanderCalled bool InstanceExpanderExpander *instances.Expander } @@ -338,6 +341,11 @@ func (c *MockEvalContext) State() *states.SyncState { return c.StateState } +func (c *MockEvalContext) RefreshState() *states.SyncState { + c.RefreshStateCalled = true + return c.RefreshStateState +} + func (c *MockEvalContext) InstanceExpander() *instances.Expander { c.InstanceExpanderCalled = true return c.InstanceExpanderExpander From ad22e137e6aad7b5c3aa442e8aaaaef77384a4b1 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 4 Sep 2020 15:59:22 -0400 Subject: [PATCH 03/20] Get the new RefreshState into the right contexts --- terraform/context.go | 34 ++++++++++++++++++++++----------- terraform/graph_walk_context.go | 2 ++ 2 files changed, 25 insertions(+), 11 deletions(-) diff --git a/terraform/context.go b/terraform/context.go index 4156aa1f0f..8afba75cd8 100644 --- a/terraform/context.go +++ b/terraform/context.go @@ -781,20 +781,32 @@ func (c *Context) walk(graph *Graph, operation walkOperation) (*ContextGraphWalk } func (c *Context) graphWalker(operation walkOperation) *ContextGraphWalker { - if operation == walkValidate { - return &ContextGraphWalker{ - Context: c, - State: states.NewState().SyncWrapper(), - Changes: c.changes.SyncWrapper(), - InstanceExpander: instances.NewExpander(), - Operation: operation, - StopContext: c.runContext, - RootVariableValues: c.variables, - } + var state *states.SyncState + var refreshState *states.SyncState + + switch operation { + case walkValidate: + // validate should not use any state + s := states.NewState() + state = s.SyncWrapper() + + // validate currently uses the plan graph, so we have to populate the + // refreshState. + refreshState = s.SyncWrapper() + + case walkPlan: + state = c.state.SyncWrapper() + // plan requires a second state to store the refreshed resources + refreshState = c.state.DeepCopy().SyncWrapper() + + default: + state = c.state.SyncWrapper() } + return &ContextGraphWalker{ Context: c, - State: c.state.SyncWrapper(), + State: state, + RefreshState: refreshState, Changes: c.changes.SyncWrapper(), InstanceExpander: instances.NewExpander(), Operation: operation, diff --git a/terraform/graph_walk_context.go b/terraform/graph_walk_context.go index c842c0af43..d3ca73b3fa 100644 --- a/terraform/graph_walk_context.go +++ b/terraform/graph_walk_context.go @@ -26,6 +26,7 @@ type ContextGraphWalker struct { // Configurable values Context *Context State *states.SyncState // Used for safe concurrent access to state + RefreshState *states.SyncState // Used for safe concurrent access to state Changes *plans.ChangesSync // Used for safe concurrent writes to changes InstanceExpander *instances.Expander // Tracks our gradual expansion of module and resource instances Operation walkOperation @@ -96,6 +97,7 @@ func (w *ContextGraphWalker) EvalContext() EvalContext { ProvisionerLock: &w.provisionerLock, ChangesValue: w.Changes, StateValue: w.State, + RefreshStateValue: w.RefreshState, Evaluator: evaluator, VariableValues: w.variableValues, VariableValuesLock: &w.variableValuesLock, From 7b178b17888c539bbff6bd0520b99498b1a9b7e6 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 4 Sep 2020 15:59:44 -0400 Subject: [PATCH 04/20] add a way to selectively write to RefreshState All resources use EvalWriteState to store their state, so we need a way to switch the states when the resource is refreshing vs when it is planning. (this will likely change once we factor out the EvalNode pattern) --- terraform/eval_state.go | 21 ++++++++++++++++++++- terraform/node_resource_plan_instance.go | 21 +++++++++++---------- 2 files changed, 31 insertions(+), 11 deletions(-) diff --git a/terraform/eval_state.go b/terraform/eval_state.go index 3adabba4ab..b5c5968732 100644 --- a/terraform/eval_state.go +++ b/terraform/eval_state.go @@ -13,6 +13,13 @@ import ( "github.com/hashicorp/terraform/tfdiags" ) +type phaseState int + +const ( + workingState phaseState = iota + refreshState +) + // EvalReadState is an EvalNode implementation that reads the // current object for a specific instance in the state. type EvalReadState struct { @@ -220,6 +227,10 @@ type EvalWriteState struct { // Dependencies are the inter-resource dependencies to be stored in the // state. Dependencies *[]addrs.ConfigResource + + // targetState determines which context state we're writing to during plan. + // The default is the global working state. + targetState phaseState } func (n *EvalWriteState) Eval(ctx EvalContext) (interface{}, error) { @@ -230,7 +241,15 @@ func (n *EvalWriteState) Eval(ctx EvalContext) (interface{}, error) { } absAddr := n.Addr.Absolute(ctx.Path()) - state := ctx.State() + + var state *states.SyncState + switch n.targetState { + case refreshState: + log.Printf("[TRACE] EvalWriteState: using RefreshState for %s", absAddr) + state = ctx.RefreshState() + default: + state = ctx.State() + } if n.ProviderAddr.Provider.Type == "" { return nil, fmt.Errorf("failed to write state for %s: missing provider type", absAddr) diff --git a/terraform/node_resource_plan_instance.go b/terraform/node_resource_plan_instance.go index 2071c5278e..12bbd5c7de 100644 --- a/terraform/node_resource_plan_instance.go +++ b/terraform/node_resource_plan_instance.go @@ -107,8 +107,8 @@ func (n *NodePlannableResourceInstance) evalTreeManagedResource(addr addrs.AbsRe var provider providers.Interface var providerSchema *ProviderSchema var change *plans.ResourceInstanceChange - var refreshState *states.ResourceInstanceObject - var planState *states.ResourceInstanceObject + var instanceRefreshState *states.ResourceInstanceObject + var instancePlanState *states.ResourceInstanceObject return &EvalSequence{ Nodes: []EvalNode{ @@ -129,10 +129,10 @@ func (n *NodePlannableResourceInstance) evalTreeManagedResource(addr addrs.AbsRe Addr: addr.Resource, Provider: &provider, ProviderSchema: &providerSchema, - Output: &refreshState, + Output: &instanceRefreshState, }, &EvalRefreshDependencies{ - State: &refreshState, + State: &instanceRefreshState, Dependencies: &n.Dependencies, }, &EvalRefresh{ @@ -141,14 +141,15 @@ func (n *NodePlannableResourceInstance) evalTreeManagedResource(addr addrs.AbsRe Provider: &provider, ProviderMetas: n.ProviderMetas, ProviderSchema: &providerSchema, - State: &refreshState, - Output: &refreshState, + State: &instanceRefreshState, + Output: &instanceRefreshState, }, &EvalWriteState{ Addr: addr.Resource, ProviderAddr: n.ResolvedProvider, - State: &refreshState, + State: &instanceRefreshState, ProviderSchema: &providerSchema, + targetState: refreshState, }, // Plan the instance @@ -160,9 +161,9 @@ func (n *NodePlannableResourceInstance) evalTreeManagedResource(addr addrs.AbsRe ProviderAddr: n.ResolvedProvider, ProviderMetas: n.ProviderMetas, ProviderSchema: &providerSchema, - State: &refreshState, + State: &instanceRefreshState, OutputChange: &change, - OutputState: &planState, + OutputState: &instancePlanState, }, &EvalCheckPreventDestroy{ Addr: addr.Resource, @@ -172,7 +173,7 @@ func (n *NodePlannableResourceInstance) evalTreeManagedResource(addr addrs.AbsRe &EvalWriteState{ Addr: addr.Resource, ProviderAddr: n.ResolvedProvider, - State: &planState, + State: &instancePlanState, ProviderSchema: &providerSchema, }, &EvalWriteDiff{ From 8cef62e45593d553e249685b3199100ffc0748f8 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 4 Sep 2020 16:49:19 -0400 Subject: [PATCH 05/20] add state to plans.Plan Since the refreshed state is now an artifact of the plan process, it makes sense to add it to the Plan type, rather than adding an additional return value to the Context.Plan method. --- plans/plan.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/plans/plan.go b/plans/plan.go index 5a3e4548ef..92778e1e78 100644 --- a/plans/plan.go +++ b/plans/plan.go @@ -5,6 +5,7 @@ import ( "github.com/hashicorp/terraform/addrs" "github.com/hashicorp/terraform/configs/configschema" + "github.com/hashicorp/terraform/states" "github.com/zclconf/go-cty/cty" ) @@ -16,15 +17,16 @@ import ( // result that will be completed during apply by resolving any values that // cannot be predicted. // -// A plan must always be accompanied by the state and configuration it was -// built from, since the plan does not itself include all of the information -// required to make the changes indicated. +// A plan must always be accompanied by the configuration it was built from, +// since the plan does not itself include all of the information required to +// make the changes indicated. type Plan struct { VariableValues map[string]DynamicValue Changes *Changes TargetAddrs []addrs.Targetable ProviderSHA256s map[string][]byte Backend Backend + State *states.State } // Backend represents the backend-related configuration and other data as it From 5cf7e237d511186d3a3791d55f73c8192ea14529 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 4 Sep 2020 16:51:11 -0400 Subject: [PATCH 06/20] return the refreshed state in the Plan result --- terraform/context.go | 46 ++++++++++++++++++++++++-------------------- 1 file changed, 25 insertions(+), 21 deletions(-) diff --git a/terraform/context.go b/terraform/context.go index 8afba75cd8..0f36379fc6 100644 --- a/terraform/context.go +++ b/terraform/context.go @@ -93,13 +93,14 @@ type ContextMeta struct { // perform operations on infrastructure. This structure is built using // NewContext. type Context struct { - config *configs.Config - changes *plans.Changes - state *states.State - targets []addrs.Targetable - variables InputValues - meta *ContextMeta - destroy bool + config *configs.Config + changes *plans.Changes + state *states.State + refreshState *states.State + targets []addrs.Targetable + variables InputValues + meta *ContextMeta + destroy bool hooks []Hook components contextComponentFactory @@ -223,17 +224,18 @@ func NewContext(opts *ContextOpts) (*Context, tfdiags.Diagnostics) { } return &Context{ - components: components, - schemas: schemas, - destroy: opts.Destroy, - changes: changes, - hooks: hooks, - meta: opts.Meta, - config: config, - state: state, - targets: opts.Targets, - uiInput: opts.UIInput, - variables: variables, + components: components, + schemas: schemas, + destroy: opts.Destroy, + changes: changes, + hooks: hooks, + meta: opts.Meta, + config: config, + state: state, + refreshState: state.DeepCopy(), + targets: opts.Targets, + uiInput: opts.UIInput, + variables: variables, parallelSem: NewSemaphore(par), providerInputConfig: make(map[string]map[string]cty.Value), @@ -493,7 +495,8 @@ Note that the -target option is not suitable for routine use, and is provided on return c.state, diags } -// Plan generates an execution plan for the given context. +// Plan generates an execution plan for the given context, and returns the +// refreshed state. // // The execution plan encapsulates the context and can be stored // in order to reinstantiate a context later for Apply. @@ -578,6 +581,8 @@ The -target option is not for routine use, and is provided only for exceptional } p.Changes = c.changes + p.State = c.refreshState + return p, diags } @@ -796,8 +801,7 @@ func (c *Context) graphWalker(operation walkOperation) *ContextGraphWalker { case walkPlan: state = c.state.SyncWrapper() - // plan requires a second state to store the refreshed resources - refreshState = c.state.DeepCopy().SyncWrapper() + refreshState = c.refreshState.SyncWrapper() default: state = c.state.SyncWrapper() From 908217acc43786c6ede2fd1636c862b3827407e8 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 4 Sep 2020 18:09:47 -0400 Subject: [PATCH 07/20] try refreshing during plan as the default This breaks a bunch of tests, and we need to figure out why before moving on. --- terraform/context.go | 28 +++++++--------------------- 1 file changed, 7 insertions(+), 21 deletions(-) diff --git a/terraform/context.go b/terraform/context.go index 0f36379fc6..266fe50a63 100644 --- a/terraform/context.go +++ b/terraform/context.go @@ -541,31 +541,13 @@ The -target option is not for routine use, and is provided only for exceptional ProviderSHA256s: c.providerSHA256s, } - var operation walkOperation - if c.destroy { - operation = walkPlanDestroy - } else { - // Set our state to be something temporary. We do this so that - // the plan can update a fake state so that variables work, then - // we replace it back with our old state. - old := c.state - if old == nil { - c.state = states.NewState() - } else { - c.state = old.DeepCopy() - } - defer func() { - c.state = old - }() - - operation = walkPlan - } - - // Build the graph. + operation := walkPlan graphType := GraphTypePlan if c.destroy { + operation = walkPlanDestroy graphType = GraphTypePlanDestroy } + graph, graphDiags := c.Graph(graphType, nil) diags = diags.Append(graphDiags) if graphDiags.HasErrors() { @@ -583,6 +565,10 @@ The -target option is not for routine use, and is provided only for exceptional p.State = c.refreshState + // replace the working state with the updated state, so that immediate calls + // to Apply work as expected. + c.state = c.refreshState + return p, diags } From 7d6472dad0ca5f50a8e299ebdf860af5b4d82f4a Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 10 Sep 2020 16:49:46 -0400 Subject: [PATCH 08/20] use plan state in contextOptsForPlanViaFile --- terraform/context_apply_test.go | 26 ++++++++++++-------------- terraform/context_test.go | 4 ++-- 2 files changed, 14 insertions(+), 16 deletions(-) diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index 70e4285cc6..4deda63152 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -2376,12 +2376,10 @@ func TestContext2Apply_provisionerInterpCount(t *testing.T) { t.Fatalf("plan failed unexpectedly: %s", diags.Err()) } - state := ctx.State() - // We'll marshal and unmarshal the plan here, to ensure that we have // a clean new context as would be created if we separately ran // terraform plan -out=tfplan && terraform apply tfplan - ctxOpts, err := contextOptsForPlanViaFile(snap, state, plan) + ctxOpts, err := contextOptsForPlanViaFile(snap, plan) if err != nil { t.Fatal(err) } @@ -6014,7 +6012,7 @@ func TestContext2Apply_destroyModuleWithAttrsReferencingResource(t *testing.T) { t.Logf("Step 2 plan: %s", legacyDiffComparisonString(plan.Changes)) - ctxOpts, err := contextOptsForPlanViaFile(snap, state, plan) + ctxOpts, err := contextOptsForPlanViaFile(snap, plan) if err != nil { t.Fatalf("failed to round-trip through planfile: %s", err) } @@ -6089,7 +6087,7 @@ func TestContext2Apply_destroyWithModuleVariableAndCount(t *testing.T) { t.Fatalf("destroy plan err: %s", diags.Err()) } - ctxOpts, err := contextOptsForPlanViaFile(snap, state, plan) + ctxOpts, err := contextOptsForPlanViaFile(snap, plan) if err != nil { t.Fatalf("failed to round-trip through planfile: %s", err) } @@ -6245,7 +6243,7 @@ func TestContext2Apply_destroyWithModuleVariableAndCountNested(t *testing.T) { t.Fatalf("destroy plan err: %s", diags.Err()) } - ctxOpts, err := contextOptsForPlanViaFile(snap, state, plan) + ctxOpts, err := contextOptsForPlanViaFile(snap, plan) if err != nil { t.Fatalf("failed to round-trip through planfile: %s", err) } @@ -8319,7 +8317,7 @@ func TestContext2Apply_issue7824(t *testing.T) { } // Write / Read plan to simulate running it through a Plan file - ctxOpts, err := contextOptsForPlanViaFile(snap, ctx.State(), plan) + ctxOpts, err := contextOptsForPlanViaFile(snap, plan) if err != nil { t.Fatalf("failed to round-trip through planfile: %s", err) } @@ -8396,7 +8394,7 @@ func TestContext2Apply_issue5254(t *testing.T) { } // Write / Read plan to simulate running it through a Plan file - ctxOpts, err := contextOptsForPlanViaFile(snap, state, plan) + ctxOpts, err := contextOptsForPlanViaFile(snap, plan) if err != nil { t.Fatalf("failed to round-trip through planfile: %s", err) } @@ -8473,7 +8471,7 @@ func TestContext2Apply_targetedWithTaintedInState(t *testing.T) { } // Write / Read plan to simulate running it through a Plan file - ctxOpts, err := contextOptsForPlanViaFile(snap, ctx.State(), plan) + ctxOpts, err := contextOptsForPlanViaFile(snap, plan) if err != nil { t.Fatalf("failed to round-trip through planfile: %s", err) } @@ -8730,7 +8728,7 @@ func TestContext2Apply_destroyNestedModuleWithAttrsReferencingResource(t *testin t.Fatalf("destroy plan err: %s", diags.Err()) } - ctxOpts, err := contextOptsForPlanViaFile(snap, state, plan) + ctxOpts, err := contextOptsForPlanViaFile(snap, plan) if err != nil { t.Fatalf("failed to round-trip through planfile: %s", err) } @@ -9313,7 +9311,7 @@ func TestContext2Apply_plannedInterpolatedCount(t *testing.T) { // We'll marshal and unmarshal the plan here, to ensure that we have // a clean new context as would be created if we separately ran // terraform plan -out=tfplan && terraform apply tfplan - ctxOpts, err := contextOptsForPlanViaFile(snap, ctx.State(), plan) + ctxOpts, err := contextOptsForPlanViaFile(snap, plan) if err != nil { t.Fatalf("failed to round-trip through planfile: %s", err) } @@ -9376,7 +9374,7 @@ func TestContext2Apply_plannedDestroyInterpolatedCount(t *testing.T) { // We'll marshal and unmarshal the plan here, to ensure that we have // a clean new context as would be created if we separately ran // terraform plan -out=tfplan && terraform apply tfplan - ctxOpts, err := contextOptsForPlanViaFile(snap, ctx.State(), plan) + ctxOpts, err := contextOptsForPlanViaFile(snap, plan) if err != nil { t.Fatalf("failed to round-trip through planfile: %s", err) } @@ -9826,7 +9824,7 @@ func TestContext2Apply_destroyDataCycle(t *testing.T) { // We'll marshal and unmarshal the plan here, to ensure that we have // a clean new context as would be created if we separately ran // terraform plan -out=tfplan && terraform apply tfplan - ctxOpts, err := contextOptsForPlanViaFile(snap, state, plan) + ctxOpts, err := contextOptsForPlanViaFile(snap, plan) if err != nil { t.Fatal(err) } @@ -10141,7 +10139,7 @@ func TestContext2Apply_cbdCycle(t *testing.T) { // We'll marshal and unmarshal the plan here, to ensure that we have // a clean new context as would be created if we separately ran // terraform plan -out=tfplan && terraform apply tfplan - ctxOpts, err := contextOptsForPlanViaFile(snap, state, plan) + ctxOpts, err := contextOptsForPlanViaFile(snap, plan) if err != nil { t.Fatal(err) } diff --git a/terraform/context_test.go b/terraform/context_test.go index f071a2afa9..a07e275e2a 100644 --- a/terraform/context_test.go +++ b/terraform/context_test.go @@ -749,7 +749,7 @@ func testProviderSchema(name string) *ProviderSchema { // our context tests try to exercise lots of stuff at once and so having them // round-trip things through on-disk files is often an important part of // fully representing an old bug in a regression test. -func contextOptsForPlanViaFile(configSnap *configload.Snapshot, state *states.State, plan *plans.Plan) (*ContextOpts, error) { +func contextOptsForPlanViaFile(configSnap *configload.Snapshot, plan *plans.Plan) (*ContextOpts, error) { dir, err := ioutil.TempDir("", "terraform-contextForPlanViaFile") if err != nil { return nil, err @@ -760,7 +760,7 @@ func contextOptsForPlanViaFile(configSnap *configload.Snapshot, state *states.St // to run through any of the codepaths that care about Lineage/Serial/etc // here anyway. stateFile := &statefile.File{ - State: state, + State: plan.State, } // To make life a little easier for test authors, we'll populate a simple From ced7aedeca8623fce53933abf02cb39a3a25d7ad Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 10 Sep 2020 17:59:30 -0400 Subject: [PATCH 09/20] fixup count transition for refresh state We need to do this for both states during plan --- terraform/eval_count.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/terraform/eval_count.go b/terraform/eval_count.go index b09c72e661..5247077971 100644 --- a/terraform/eval_count.go +++ b/terraform/eval_count.go @@ -117,8 +117,12 @@ func evaluateCountExpressionValue(expr hcl.Expression, ctx EvalContext) (cty.Val // or this function will block forever awaiting the lock. func fixResourceCountSetTransition(ctx EvalContext, addr addrs.ConfigResource, countEnabled bool) { state := ctx.State() - changed := state.MaybeFixUpResourceInstanceAddressForCount(addr, countEnabled) - if changed { + if state.MaybeFixUpResourceInstanceAddressForCount(addr, countEnabled) { + log.Printf("[TRACE] renamed first %s instance in transient state due to count argument change", addr) + } + + refreshState := ctx.RefreshState() + if refreshState != nil && refreshState.MaybeFixUpResourceInstanceAddressForCount(addr, countEnabled) { log.Printf("[TRACE] renamed first %s instance in transient state due to count argument change", addr) } } From a3c9d7abc1331f42e3b6aa374c1b3eea776c9739 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 11 Sep 2020 10:55:39 -0400 Subject: [PATCH 10/20] update comments around evaluating 0 instances --- terraform/evaluate.go | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/terraform/evaluate.go b/terraform/evaluate.go index 9d5cfda24a..5f5c944e65 100644 --- a/terraform/evaluate.go +++ b/terraform/evaluate.go @@ -658,24 +658,21 @@ func (d *evaluationStateData) GetResource(addr addrs.Resource, rng tfdiags.Sourc case config.ForEach != nil: return cty.EmptyObjectVal, diags default: - // FIXME: try to prove this path should not be reached during plan. - // - // while we can reference an expanded resource with 0 + // While we can reference an expanded resource with 0 // instances, we cannot reference instances that do not exist. - // Since we haven't ensured that all instances exist in all - // cases (this path only ever returned unknown), only log this as - // an error for now, and continue to return a DynamicVal + // Due to the fact that we may have direct references to + // instances that may end up in a root output during destroy + // (since a planned destroy cannot yet remove root outputs), we + // need to return a dynamic value here to allow evaluation to + // continue. log.Printf("[ERROR] unknown instance %q referenced during plan", addr.Absolute(d.ModulePath)) return cty.DynamicVal, diags } default: - // we must return DynamicVal so that both interpretations - // can proceed without generating errors, and we'll deal with this - // in a later step where more information is gathered. - // (In practice we should only end up here during the validate walk, + // We should only end up here during the validate walk, // since later walks should have at least partial states populated - // for all resources in the configuration.) + // for all resources in the configuration. return cty.DynamicVal, diags } } From d19f440d81d735a20d4d7c9c1e093a29a8229f87 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 11 Sep 2020 11:12:16 -0400 Subject: [PATCH 11/20] contexts have a copy of the state We need to build a new context go get at the modified state --- terraform/context_apply_test.go | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index 4deda63152..be11ee57bb 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -9155,11 +9155,16 @@ func TestContext2Apply_destroyWithProviders(t *testing.T) { } // correct the state - state.Modules["module.mod.module.removed"].Resources["aws_instance.child"].ProviderConfig = addrs.AbsProviderConfig{ - Provider: addrs.NewDefaultProvider("aws"), - Alias: "bar", - Module: addrs.RootModule, - } + state.Modules["module.mod.module.removed"].Resources["aws_instance.child"].ProviderConfig = mustProviderConfig(`provider["registry.terraform.io/hashicorp/aws"].bar`) + + ctx = testContext2(t, &ContextOpts{ + Config: m, + Providers: map[addrs.Provider]providers.Factory{ + addrs.NewDefaultProvider("aws"): testProviderFuncFixed(p), + }, + State: state, + Destroy: true, + }) if _, diags := ctx.Plan(); diags.HasErrors() { t.Fatal(diags.Err()) From 1fa3503acd1f7da9cc6bd70e9d2073d4eb695d33 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 11 Sep 2020 11:42:05 -0400 Subject: [PATCH 12/20] fixup last tests that need correct state --- terraform/context_apply_test.go | 18 +++++++++++++++++- terraform/context_plan_test.go | 6 +++--- .../child/main.tf | 4 ++-- .../apply-destroy-module-with-attrs/main.tf | 4 ++-- 4 files changed, 24 insertions(+), 8 deletions(-) diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index be11ee57bb..22f0d56573 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -9861,6 +9861,22 @@ func TestContext2Apply_taintedDestroyFailure(t *testing.T) { return testApplyFn(info, s, d) } + p.GetSchemaReturn = &ProviderSchema{ + ResourceTypes: map[string]*configschema.Block{ + "test_instance": { + Attributes: map[string]*configschema.Attribute{ + "id": { + Type: cty.String, + Computed: true, + }, + "foo": { + Type: cty.String, + Optional: true, + }, + }, + }, + }, + } state := states.NewState() root := state.EnsureModule(addrs.RootModuleInstance) @@ -9981,7 +9997,7 @@ func TestContext2Apply_taintedDestroyFailure(t *testing.T) { t.Fatal("test_instance.c should have no deposed instances") } - if string(c.Current.AttrsJSON) != `{"id":"c","foo":"old"}` { + if string(c.Current.AttrsJSON) != `{"foo":"old","id":"c"}` { t.Fatalf("unexpected attrs for c: %q\n", c.Current.AttrsJSON) } } diff --git a/terraform/context_plan_test.go b/terraform/context_plan_test.go index c981834bc5..c2fc4fefed 100644 --- a/terraform/context_plan_test.go +++ b/terraform/context_plan_test.go @@ -2829,7 +2829,7 @@ func TestContext2Plan_countDecreaseToOne(t *testing.T) { } } - expectedState := `aws_instance.foo.0: + expectedState := `aws_instance.foo: ID = bar provider = provider["registry.terraform.io/hashicorp/aws"] foo = foo @@ -4068,7 +4068,7 @@ func TestContext2Plan_taintDestroyInterpolatedCountRace(t *testing.T) { Providers: map[addrs.Provider]providers.Factory{ addrs.NewDefaultProvider("aws"): testProviderFuncFixed(p), }, - State: state, + State: state.DeepCopy(), }) plan, diags := ctx.Plan() @@ -4092,7 +4092,7 @@ func TestContext2Plan_taintDestroyInterpolatedCountRace(t *testing.T) { switch i := ric.Addr.String(); i { case "aws_instance.foo[0]": if res.Action != plans.DeleteThenCreate { - t.Fatalf("resource %s should be replaced", i) + t.Fatalf("resource %s should be replaced, not %s", i, res.Action) } checkVals(t, objectVal(t, schema, map[string]cty.Value{ "id": cty.StringVal("bar"), diff --git a/terraform/testdata/apply-destroy-module-with-attrs/child/main.tf b/terraform/testdata/apply-destroy-module-with-attrs/child/main.tf index 95c7012687..55fa601707 100644 --- a/terraform/testdata/apply-destroy-module-with-attrs/child/main.tf +++ b/terraform/testdata/apply-destroy-module-with-attrs/child/main.tf @@ -1,9 +1,9 @@ variable "vpc_id" {} resource "aws_instance" "child" { - vpc_id = "${var.vpc_id}" + vpc_id = var.vpc_id } output "modout" { - value = "${aws_instance.child.id}" + value = aws_instance.child.id } diff --git a/terraform/testdata/apply-destroy-module-with-attrs/main.tf b/terraform/testdata/apply-destroy-module-with-attrs/main.tf index d369873bf9..9b2d46db74 100644 --- a/terraform/testdata/apply-destroy-module-with-attrs/main.tf +++ b/terraform/testdata/apply-destroy-module-with-attrs/main.tf @@ -2,9 +2,9 @@ resource "aws_instance" "vpc" { } module "child" { source = "./child" - vpc_id = "${aws_instance.vpc.id}" + vpc_id = aws_instance.vpc.id } output "out" { - value = "${module.child.modout}" + value = module.child.modout } From ad5899d8bb79f8b6f1de24855353c4f646cca160 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 11 Sep 2020 13:47:30 -0400 Subject: [PATCH 13/20] ReadResource is called during plan but not destroy --- command/plan_test.go | 8 -------- 1 file changed, 8 deletions(-) diff --git a/command/plan_test.go b/command/plan_test.go index e89103b792..46b631a6f0 100644 --- a/command/plan_test.go +++ b/command/plan_test.go @@ -96,10 +96,6 @@ func TestPlan_plan(t *testing.T) { if code := c.Run(args); code != 1 { t.Fatalf("wrong exit status %d; want 1\nstderr: %s", code, ui.ErrorWriter.String()) } - - if p.ReadResourceCalled { - t.Fatal("ReadResource should not have been called") - } } func TestPlan_destroy(t *testing.T) { @@ -142,10 +138,6 @@ func TestPlan_destroy(t *testing.T) { t.Fatalf("bad: %d\n\n%s", code, ui.ErrorWriter.String()) } - if !p.ReadResourceCalled { - t.Fatal("ReadResource should have been called") - } - plan := testReadPlan(t, outPath) for _, rc := range plan.Changes.Resources { if got, want := rc.Action, plans.Delete; got != want { From e54949f2e1cbd62d0401f7d2b7b7d2f34735cbfc Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 11 Sep 2020 15:53:21 -0400 Subject: [PATCH 14/20] fix show -json tests The prior state recorded in the plans did not match the actual prior state. Make the plans and state match depending on whether there was existing state or not. --- command/testdata/show-json/basic-create/output.json | 13 +------------ .../show-json/basic-delete/terraform.tfstate | 7 ++++++- .../show-json/basic-update/terraform.tfstate | 7 ++++++- command/testdata/show-json/modules/output.json | 13 +------------ .../show-json/multi-resource-update/output.json | 8 +------- 5 files changed, 15 insertions(+), 33 deletions(-) diff --git a/command/testdata/show-json/basic-create/output.json b/command/testdata/show-json/basic-create/output.json index ff83de3fef..19e1846cc9 100644 --- a/command/testdata/show-json/basic-create/output.json +++ b/command/testdata/show-json/basic-create/output.json @@ -53,18 +53,7 @@ ] } }, - "prior_state": { - "format_version": "0.1", - "values": { - "outputs": { - "test": { - "sensitive": false, - "value": "bar" - } - }, - "root_module": {} - } - }, + "prior_state": {}, "resource_changes": [ { "address": "test_instance.test[0]", diff --git a/command/testdata/show-json/basic-delete/terraform.tfstate b/command/testdata/show-json/basic-delete/terraform.tfstate index ac865b8644..6d2b622377 100644 --- a/command/testdata/show-json/basic-delete/terraform.tfstate +++ b/command/testdata/show-json/basic-delete/terraform.tfstate @@ -3,7 +3,12 @@ "terraform_version": "0.12.0", "serial": 7, "lineage": "configuredUnchanged", - "outputs": {}, + "outputs": { + "test": { + "value": "bar", + "type": "string" + } + }, "resources": [ { "mode": "managed", diff --git a/command/testdata/show-json/basic-update/terraform.tfstate b/command/testdata/show-json/basic-update/terraform.tfstate index b57f60f843..bc691aee08 100644 --- a/command/testdata/show-json/basic-update/terraform.tfstate +++ b/command/testdata/show-json/basic-update/terraform.tfstate @@ -3,7 +3,12 @@ "terraform_version": "0.12.0", "serial": 7, "lineage": "configuredUnchanged", - "outputs": {}, + "outputs": { + "test": { + "value": "bar", + "type": "string" + } + }, "resources": [ { "mode": "managed", diff --git a/command/testdata/show-json/modules/output.json b/command/testdata/show-json/modules/output.json index 898763aad9..76b4f471ca 100644 --- a/command/testdata/show-json/modules/output.json +++ b/command/testdata/show-json/modules/output.json @@ -69,18 +69,7 @@ ] } }, - "prior_state": { - "format_version": "0.1", - "values": { - "outputs": { - "test": { - "sensitive": false, - "value": "baz" - } - }, - "root_module": {} - } - }, + "prior_state": {}, "resource_changes": [ { "address": "module.module_test_bar.test_instance.test", diff --git a/command/testdata/show-json/multi-resource-update/output.json b/command/testdata/show-json/multi-resource-update/output.json index cc8f6d1ede..ba764b2651 100644 --- a/command/testdata/show-json/multi-resource-update/output.json +++ b/command/testdata/show-json/multi-resource-update/output.json @@ -101,20 +101,14 @@ "format_version": "0.1", "terraform_version": "0.13.0", "values": { - "outputs": { - "test": { - "sensitive": false, - "value": "bar" - } - }, "root_module": { "resources": [ { "address": "test_instance.test[0]", + "index": 0, "mode": "managed", "type": "test_instance", "name": "test", - "index": 0, "provider_name": "registry.terraform.io/hashicorp/test", "schema_version": 0, "values": { From 19d67b760678e572b5af8b2cf39721fef1ab99fe Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 11 Sep 2020 17:27:12 -0400 Subject: [PATCH 15/20] don't leave old refreshed state in the context After apply, any refreshed state from a plan would be invalid. Normal usage doesn't ever see this, bu internal tests may re-use the context. --- terraform/context.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/terraform/context.go b/terraform/context.go index 266fe50a63..dd98897214 100644 --- a/terraform/context.go +++ b/terraform/context.go @@ -492,6 +492,10 @@ Note that the -target option is not suitable for routine use, and is provided on )) } + // This isn't technically needed, but don't leave an old refreshed state + // around in case we re-use the context in internal tests. + c.refreshState = c.state.DeepCopy() + return c.state, diags } From 88509de7db00fe70f99d0ebeacd07940604e25e4 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 11 Sep 2020 17:28:43 -0400 Subject: [PATCH 16/20] remove Refresh steps from legacy provider tests Update the old acceptance test framework just enough to make the tests pass. --- helper/resource/testing_config.go | 70 ++++++++----------------------- 1 file changed, 18 insertions(+), 52 deletions(-) diff --git a/helper/resource/testing_config.go b/helper/resource/testing_config.go index 8f31e38bed..04194dddf9 100644 --- a/helper/resource/testing_config.go +++ b/helper/resource/testing_config.go @@ -9,7 +9,6 @@ import ( "sort" "strings" - "github.com/hashicorp/terraform/addrs" "github.com/hashicorp/terraform/configs/hcl2shim" "github.com/hashicorp/terraform/states" @@ -61,28 +60,18 @@ func testStep(opts terraform.ContextOpts, state *terraform.State, step TestStep) log.Printf("[WARN] Config warnings:\n%s", stepDiags) } - // Refresh! - newState, stepDiags := ctx.Refresh() - // shim the state first so the test can check the state on errors - - state, err = shimNewState(newState, step.providers) - if err != nil { - return nil, err - } - if stepDiags.HasErrors() { - return state, newOperationError("refresh", stepDiags) - } - // If this step is a PlanOnly step, skip over this first Plan and subsequent // Apply, and use the follow up Plan that checks for perpetual diffs if !step.PlanOnly { // Plan! - if p, stepDiags := ctx.Plan(); stepDiags.HasErrors() { + p, stepDiags := ctx.Plan() + if stepDiags.HasErrors() { return state, newOperationError("plan", stepDiags) - } else { - log.Printf("[WARN] Test: Step plan: %s", legacyPlanComparisonString(newState, p.Changes)) } + newState := p.State + log.Printf("[WARN] Test: Step plan: %s", legacyPlanComparisonString(newState, p.Changes)) + // We need to keep a copy of the state prior to destroying // such that destroy steps can verify their behavior in the check // function @@ -115,11 +104,21 @@ func testStep(opts terraform.ContextOpts, state *terraform.State, step TestStep) // Now, verify that Plan is now empty and we don't have a perpetual diff issue // We do this with TWO plans. One without a refresh. - var p *plans.Plan - if p, stepDiags = ctx.Plan(); stepDiags.HasErrors() { + p, stepDiags := ctx.Plan() + if stepDiags.HasErrors() { return state, newOperationError("follow-up plan", stepDiags) } - if !p.Changes.Empty() { + + // we don't technically need this any longer with plan handling refreshing, + // but run it anyway to ensure the context is working as expected. + p, stepDiags = ctx.Plan() + if stepDiags.HasErrors() { + return state, newOperationError("second follow-up plan", stepDiags) + } + empty := p.Changes.Empty() + newState := p.State + + if !empty { if step.ExpectNonEmptyPlan { log.Printf("[INFO] Got non-empty plan, as expected:\n\n%s", legacyPlanComparisonString(newState, p.Changes)) } else { @@ -128,39 +127,6 @@ func testStep(opts terraform.ContextOpts, state *terraform.State, step TestStep) } } - // And another after a Refresh. - if !step.Destroy || (step.Destroy && !step.PreventPostDestroyRefresh) { - newState, stepDiags = ctx.Refresh() - if stepDiags.HasErrors() { - return state, newOperationError("follow-up refresh", stepDiags) - } - - state, err = shimNewState(newState, step.providers) - if err != nil { - return nil, err - } - } - if p, stepDiags = ctx.Plan(); stepDiags.HasErrors() { - return state, newOperationError("second follow-up refresh", stepDiags) - } - empty := p.Changes.Empty() - - // Data resources are tricky because they legitimately get instantiated - // during refresh so that they will be already populated during the - // plan walk. Because of this, if we have any data resources in the - // config we'll end up wanting to destroy them again here. This is - // acceptable and expected, and we'll treat it as "empty" for the - // sake of this testing. - if step.Destroy && !empty { - empty = true - for _, change := range p.Changes.Resources { - if change.Addr.Resource.Resource.Mode != addrs.DataResourceMode { - empty = false - break - } - } - } - if !empty { if step.ExpectNonEmptyPlan { log.Printf("[INFO] Got non-empty plan, as expected:\n\n%s", legacyPlanComparisonString(newState, p.Changes)) From f52d836e0a5ceb7ce056c5738b349be5e6453498 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 11 Sep 2020 17:56:12 -0400 Subject: [PATCH 17/20] fix local backend tests to match new behavior Leaving plan with -refresh=false tests failing for now. --- backend/local/backend_plan_test.go | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/backend/local/backend_plan_test.go b/backend/local/backend_plan_test.go index 64025edec7..8b7240ec7b 100644 --- a/backend/local/backend_plan_test.go +++ b/backend/local/backend_plan_test.go @@ -357,8 +357,8 @@ func TestLocal_planDeposedOnly(t *testing.T) { if run.Result != backend.OperationSuccess { t.Fatalf("plan operation failed") } - if !p.ReadResourceCalled { - t.Fatal("ReadResource should be called") + if p.ReadResourceCalled { + t.Fatal("ReadResource should not be called") } if run.PlanEmpty { t.Fatal("plan should not be empty") @@ -543,8 +543,8 @@ func TestLocal_planDestroy(t *testing.T) { t.Fatalf("plan operation failed") } - if !p.ReadResourceCalled { - t.Fatal("ReadResource should be called") + if p.ReadResourceCalled { + t.Fatal("ReadResource should not be called") } if run.PlanEmpty { @@ -599,12 +599,12 @@ func TestLocal_planDestroy_withDataSources(t *testing.T) { t.Fatalf("plan operation failed") } - if !p.ReadResourceCalled { - t.Fatal("ReadResource should be called") + if p.ReadResourceCalled { + t.Fatal("ReadResource should not be called") } - if !p.ReadDataSourceCalled { - t.Fatal("ReadDataSourceCalled should be called") + if p.ReadDataSourceCalled { + t.Fatal("ReadDataSourceCalled should not be called") } if run.PlanEmpty { @@ -621,7 +621,7 @@ func TestLocal_planDestroy_withDataSources(t *testing.T) { // Data source should not be rendered in the output expectedOutput := `Terraform will perform the following actions: - # test_instance.foo will be destroyed + # test_instance.foo[0] will be destroyed - resource "test_instance" "foo" { - ami = "bar" -> null @@ -635,7 +635,7 @@ Plan: 0 to add, 0 to change, 1 to destroy.` output := b.CLI.(*cli.MockUi).OutputWriter.String() if !strings.Contains(output, expectedOutput) { - t.Fatalf("Unexpected output (expected no data source):\n%s", output) + t.Fatalf("Unexpected output:\n%s", output) } } From 865842405927caf75a8242d5dd49eed2a3800f8d Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 15 Sep 2020 10:04:52 -0400 Subject: [PATCH 18/20] skip plan with no refresh test We still need to determine if `-refresh=false` is even useful with the new planning strategy. --- backend/local/backend_plan_test.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/backend/local/backend_plan_test.go b/backend/local/backend_plan_test.go index 8b7240ec7b..446fb9d515 100644 --- a/backend/local/backend_plan_test.go +++ b/backend/local/backend_plan_test.go @@ -478,6 +478,12 @@ Plan: 1 to add, 0 to change, 1 to destroy.` } func TestLocal_planRefreshFalse(t *testing.T) { + // since there is no longer a separate Refresh walk, `-refresh=false + // doesn't do anything. + // FIXME: determine if we need a refresh option for the new plan, or remove + // this test + t.Skip() + b, cleanup := TestLocal(t) defer cleanup() From 312317abd0ab74f1db3ea57c8e52bce53229b973 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 16 Sep 2020 13:56:08 -0400 Subject: [PATCH 19/20] wrong instance key in test state This was never picked up by the tests until now --- backend/local/backend_plan_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/backend/local/backend_plan_test.go b/backend/local/backend_plan_test.go index 446fb9d515..09fd11a95b 100644 --- a/backend/local/backend_plan_test.go +++ b/backend/local/backend_plan_test.go @@ -830,7 +830,7 @@ func testPlanState_tainted() *states.State { Mode: addrs.ManagedResourceMode, Type: "test_instance", Name: "foo", - }.Instance(addrs.IntKey(0)), + }.Instance(addrs.NoKey), &states.ResourceInstanceObjectSrc{ Status: states.ObjectTainted, AttrsJSON: []byte(`{ From 86dd8938c9e86cd3ec1761190cf0a34d8a2cb615 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 16 Sep 2020 16:25:51 -0400 Subject: [PATCH 20/20] data sources now show up in the initial plan --- command/e2etest/automation_test.go | 19 +++++++++++++++++-- command/e2etest/primary_test.go | 20 ++++++++++++++++++-- 2 files changed, 35 insertions(+), 4 deletions(-) diff --git a/command/e2etest/automation_test.go b/command/e2etest/automation_test.go index af641a786a..9c42ae5276 100644 --- a/command/e2etest/automation_test.go +++ b/command/e2etest/automation_test.go @@ -8,6 +8,7 @@ import ( "testing" "github.com/hashicorp/terraform/e2e" + "github.com/hashicorp/terraform/plans" ) // The tests in this file run through different scenarios recommended in our @@ -72,11 +73,25 @@ func TestPlanApplyInAutomation(t *testing.T) { // stateResources := plan.Changes.Resources diffResources := plan.Changes.Resources - - if len(diffResources) != 1 || diffResources[0].Addr.String() != "null_resource.test" { + if len(diffResources) != 2 { t.Errorf("incorrect number of resources in plan") } + expected := map[string]plans.Action{ + "data.template_file.test": plans.Read, + "null_resource.test": plans.Create, + } + + for _, r := range diffResources { + expectedAction, ok := expected[r.Addr.String()] + if !ok { + t.Fatalf("unexpected change for %q", r.Addr) + } + if r.Action != expectedAction { + t.Fatalf("unexpected action %q for %q", r.Action, r.Addr) + } + } + //// APPLY stdout, stderr, err = tf.Run("apply", "-input=false", "tfplan") if err != nil { diff --git a/command/e2etest/primary_test.go b/command/e2etest/primary_test.go index 46a7c33bf5..31aabd2fd4 100644 --- a/command/e2etest/primary_test.go +++ b/command/e2etest/primary_test.go @@ -9,6 +9,7 @@ import ( "github.com/davecgh/go-spew/spew" "github.com/hashicorp/terraform/e2e" + "github.com/hashicorp/terraform/plans" "github.com/zclconf/go-cty/cty" ) @@ -71,8 +72,23 @@ func TestPrimarySeparatePlan(t *testing.T) { } diffResources := plan.Changes.Resources - if len(diffResources) != 1 || diffResources[0].Addr.String() != "null_resource.test" { - t.Errorf("incorrect diff in plan; want just null_resource.test to have been rendered, but have:\n%s", spew.Sdump(diffResources)) + if len(diffResources) != 2 { + t.Errorf("incorrect number of resources in plan") + } + + expected := map[string]plans.Action{ + "data.template_file.test": plans.Read, + "null_resource.test": plans.Create, + } + + for _, r := range diffResources { + expectedAction, ok := expected[r.Addr.String()] + if !ok { + t.Fatalf("unexpected change for %q", r.Addr) + } + if r.Action != expectedAction { + t.Fatalf("unexpected action %q for %q", r.Action, r.Addr) + } } //// APPLY