Skip to content

Commit 91cb440

Browse files
apparentlymartapparentlymart
committed
core: Do everything except the actual action for plans.NoOp
Previously we tried to early-exit before doing anything at all for any no-op changes, but that means we also skip some ancillary steps like evaluating any preconditions/postconditions. Now we'll skip only the main action itself for plans.NoOp, and still run through all of the other side-steps. Since one of those other steps is emitting events through the hooks interface, this means that now no-op actions are visible to hooks, whereas before we always filtered them out before calling. I therefore added some additional logic to the hooks to filter them out at the UI layer instead; the decision for whether or not to report that we visited a particular object and found no action required seems defensible as a UI-level concern anyway.
1 parent b9cc9c0 commit 91cb440

File tree

5 files changed

+230
-26
lines changed

5 files changed

+230
-26
lines changed

‎internal/command/views/hook_json.go‎

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7,13 +7,14 @@ import (
77
"time"
88
"unicode"
99

10+
"github.com/zclconf/go-cty/cty"
11+
1012
"github.com/hashicorp/terraform/internal/addrs"
1113
"github.com/hashicorp/terraform/internal/command/format"
1214
"github.com/hashicorp/terraform/internal/command/views/json"
1315
"github.com/hashicorp/terraform/internal/plans"
1416
"github.com/hashicorp/terraform/internal/states"
1517
"github.com/hashicorp/terraform/internal/terraform"
16-
"github.com/zclconf/go-cty/cty"
1718
)
1819

1920
// How long to wait between sending heartbeat/progress messages
@@ -59,8 +60,10 @@ type applyProgress struct {
5960
}
6061

6162
func (h *jsonHook) PreApply(addr addrs.AbsResourceInstance, gen states.Generation, action plans.Action, priorState, plannedNewState cty.Value) (terraform.HookAction, error) {
62-
idKey, idValue := format.ObjectValueIDOrName(priorState)
63-
h.view.Hook(json.NewApplyStart(addr, action, idKey, idValue))
63+
if action != plans.NoOp {
64+
idKey, idValue := format.ObjectValueIDOrName(priorState)
65+
h.view.Hook(json.NewApplyStart(addr, action, idKey, idValue))
66+
}
6467

6568
progress := applyProgress{
6669
addr: addr,
@@ -73,7 +76,9 @@ func (h *jsonHook) PreApply(addr addrs.AbsResourceInstance, gen states.Generatio
7376
h.applying[addr.String()] = progress
7477
h.applyingLock.Unlock()
7578

76-
go h.applyingHeartbeat(progress)
79+
if action != plans.NoOp {
80+
go h.applyingHeartbeat(progress)
81+
}
7782
return terraform.HookActionContinue, nil
7883
}
7984

@@ -101,6 +106,10 @@ func (h *jsonHook) PostApply(addr addrs.AbsResourceInstance, gen states.Generati
101106
delete(h.applying, key)
102107
h.applyingLock.Unlock()
103108

109+
if progress.action == plans.NoOp {
110+
return terraform.HookActionContinue, nil
111+
}
112+
104113
elapsed := h.timeNow().Round(time.Second).Sub(progress.start)
105114

106115
if err != nil {

‎internal/command/views/hook_ui.go‎

Lines changed: 17 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,7 @@ const (
6565
uiResourceModify
6666
uiResourceDestroy
6767
uiResourceRead
68+
uiResourceNoOp
6869
)
6970

7071
func (h *UiHook) PreApply(addr addrs.AbsResourceInstance, gen states.Generation, action plans.Action, priorState, plannedNewState cty.Value) (terraform.HookAction, error) {
@@ -89,6 +90,8 @@ func (h *UiHook) PreApply(addr addrs.AbsResourceInstance, gen states.Generation,
8990
case plans.Read:
9091
operation = "Reading..."
9192
op = uiResourceRead
93+
case plans.NoOp:
94+
op = uiResourceNoOp
9295
default:
9396
// We don't expect any other actions in here, so anything else is a
9497
// bug in the caller but we'll ignore it in order to be robust.
@@ -106,12 +109,14 @@ func (h *UiHook) PreApply(addr addrs.AbsResourceInstance, gen states.Generation,
106109
idValue = ""
107110
}
108111

109-
h.println(fmt.Sprintf(
110-
h.view.colorize.Color("[reset][bold]%s: %s%s[reset]"),
111-
dispAddr,
112-
operation,
113-
stateIdSuffix,
114-
))
112+
if operation != "" {
113+
h.println(fmt.Sprintf(
114+
h.view.colorize.Color("[reset][bold]%s: %s%s[reset]"),
115+
dispAddr,
116+
operation,
117+
stateIdSuffix,
118+
))
119+
}
115120

116121
key := addr.String()
117122
uiState := uiResourceState{
@@ -129,7 +134,9 @@ func (h *UiHook) PreApply(addr addrs.AbsResourceInstance, gen states.Generation,
129134
h.resourcesLock.Unlock()
130135

131136
// Start goroutine that shows progress
132-
go h.stillApplying(uiState)
137+
if op != uiResourceNoOp {
138+
go h.stillApplying(uiState)
139+
}
133140

134141
return terraform.HookActionContinue, nil
135142
}
@@ -201,6 +208,9 @@ func (h *UiHook) PostApply(addr addrs.AbsResourceInstance, gen states.Generation
201208
msg = "Creation complete"
202209
case uiResourceRead:
203210
msg = "Read complete"
211+
case uiResourceNoOp:
212+
// We don't make any announcements about no-op changes
213+
return terraform.HookActionContinue, nil
204214
case uiResourceUnknown:
205215
return terraform.HookActionContinue, nil
206216
}

‎internal/terraform/context_apply2_test.go‎

Lines changed: 160 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,21 +1,24 @@
11
package terraform
22

33
import (
4+
"bytes"
45
"errors"
56
"fmt"
67
"strings"
78
"sync"
89
"testing"
910
"time"
1011

12+
"github.com/davecgh/go-spew/spew"
1113
"github.com/google/go-cmp/cmp"
14+
"github.com/zclconf/go-cty/cty"
15+
1216
"github.com/hashicorp/terraform/internal/addrs"
1317
"github.com/hashicorp/terraform/internal/configs/configschema"
1418
"github.com/hashicorp/terraform/internal/lang/marks"
1519
"github.com/hashicorp/terraform/internal/plans"
1620
"github.com/hashicorp/terraform/internal/providers"
1721
"github.com/hashicorp/terraform/internal/states"
18-
"github.com/zclconf/go-cty/cty"
1922
)
2023

2124
// Test that the PreApply hook is called with the correct deposed key
@@ -914,6 +917,162 @@ resource "test_resource" "c" {
914917
})
915918
}
916919

920+
func TestContext2Apply_resourceConditionApplyTimeFail(t *testing.T) {
921+
// This tests the less common situation where a condition fails due to
922+
// a change in a resource other than the one the condition is attached to,
923+
// and the condition result is unknown during planning.
924+
//
925+
// This edge case is a tricky one because it relies on Terraform still
926+
// visiting test_resource.b (in the configuration below) to evaluate
927+
// its conditions even though there aren't any changes directly planned
928+
// for it, so that we can consider whether changes to test_resource.a
929+
// have changed the outcome.
930+
931+
m := testModuleInline(t, map[string]string{
932+
"main.tf": `
933+
variable "input" {
934+
type = string
935+
}
936+
937+
resource "test_resource" "a" {
938+
value = var.input
939+
}
940+
941+
resource "test_resource" "b" {
942+
value = "beep"
943+
944+
lifecycle {
945+
postcondition {
946+
condition = test_resource.a.output == self.output
947+
error_message = "Outputs must match."
948+
}
949+
}
950+
}
951+
`,
952+
})
953+
954+
p := testProvider("test")
955+
p.GetProviderSchemaResponse = getProviderSchemaResponseFromProviderSchema(&ProviderSchema{
956+
ResourceTypes: map[string]*configschema.Block{
957+
"test_resource": {
958+
Attributes: map[string]*configschema.Attribute{
959+
"value": {
960+
Type: cty.String,
961+
Required: true,
962+
},
963+
"output": {
964+
Type: cty.String,
965+
Computed: true,
966+
},
967+
},
968+
},
969+
},
970+
})
971+
p.PlanResourceChangeFn = func(req providers.PlanResourceChangeRequest) (resp providers.PlanResourceChangeResponse) {
972+
// Whenever "value" changes, "output" follows it during the apply step,
973+
// but is initially unknown during the plan step.
974+
975+
m := req.ProposedNewState.AsValueMap()
976+
priorVal := cty.NullVal(cty.String)
977+
if !req.PriorState.IsNull() {
978+
priorVal = req.PriorState.GetAttr("value")
979+
}
980+
if m["output"].IsNull() || !priorVal.RawEquals(m["value"]) {
981+
m["output"] = cty.UnknownVal(cty.String)
982+
}
983+
984+
resp.PlannedState = cty.ObjectVal(m)
985+
resp.LegacyTypeSystem = true
986+
return resp
987+
}
988+
p.ApplyResourceChangeFn = func(req providers.ApplyResourceChangeRequest) (resp providers.ApplyResourceChangeResponse) {
989+
m := req.PlannedState.AsValueMap()
990+
m["output"] = m["value"]
991+
resp.NewState = cty.ObjectVal(m)
992+
return resp
993+
}
994+
ctx := testContext2(t, &ContextOpts{
995+
Providers: map[addrs.Provider]providers.Factory{
996+
addrs.NewDefaultProvider("test"): testProviderFuncFixed(p),
997+
},
998+
})
999+
instA := mustResourceInstanceAddr("test_resource.a")
1000+
instB := mustResourceInstanceAddr("test_resource.b")
1001+
1002+
// Preparation: an initial plan and apply with a correct input variable
1003+
// should succeed and give us a valid and complete state to use for the
1004+
// subsequent plan and apply that we'll expect to fail.
1005+
var prevRunState *states.State
1006+
{
1007+
plan, diags := ctx.Plan(m, states.NewState(), &PlanOpts{
1008+
Mode: plans.NormalMode,
1009+
SetVariables: InputValues{
1010+
"input": &InputValue{
1011+
Value: cty.StringVal("beep"),
1012+
SourceType: ValueFromCLIArg,
1013+
},
1014+
},
1015+
})
1016+
assertNoErrors(t, diags)
1017+
planA := plan.Changes.ResourceInstance(instA)
1018+
if planA == nil || planA.Action != plans.Create {
1019+
t.Fatalf("incorrect initial plan for instance A\nwant a 'create' change\ngot: %s", spew.Sdump(planA))
1020+
}
1021+
planB := plan.Changes.ResourceInstance(instB)
1022+
if planB == nil || planB.Action != plans.Create {
1023+
t.Fatalf("incorrect initial plan for instance B\nwant a 'create' change\ngot: %s", spew.Sdump(planB))
1024+
}
1025+
1026+
state, diags := ctx.Apply(plan, m)
1027+
assertNoErrors(t, diags)
1028+
1029+
stateA := state.ResourceInstance(instA)
1030+
if stateA == nil || stateA.Current == nil || !bytes.Contains(stateA.Current.AttrsJSON, []byte(`"beep"`)) {
1031+
t.Fatalf("incorrect initial state for instance A\ngot: %s", spew.Sdump(stateA))
1032+
}
1033+
stateB := state.ResourceInstance(instB)
1034+
if stateB == nil || stateB.Current == nil || !bytes.Contains(stateB.Current.AttrsJSON, []byte(`"beep"`)) {
1035+
t.Fatalf("incorrect initial state for instance B\ngot: %s", spew.Sdump(stateB))
1036+
}
1037+
prevRunState = state
1038+
}
1039+
1040+
// Now we'll run another plan and apply with a different value for
1041+
// var.input that should cause the test_resource.b condition to be unknown
1042+
// during planning and then fail during apply.
1043+
{
1044+
plan, diags := ctx.Plan(m, prevRunState, &PlanOpts{
1045+
Mode: plans.NormalMode,
1046+
SetVariables: InputValues{
1047+
"input": &InputValue{
1048+
Value: cty.StringVal("boop"), // NOTE: This has changed
1049+
SourceType: ValueFromCLIArg,
1050+
},
1051+
},
1052+
})
1053+
assertNoErrors(t, diags)
1054+
planA := plan.Changes.ResourceInstance(instA)
1055+
if planA == nil || planA.Action != plans.Update {
1056+
t.Fatalf("incorrect initial plan for instance A\nwant an 'update' change\ngot: %s", spew.Sdump(planA))
1057+
}
1058+
planB := plan.Changes.ResourceInstance(instB)
1059+
if planB == nil || planB.Action != plans.NoOp {
1060+
t.Fatalf("incorrect initial plan for instance B\nwant a 'no-op' change\ngot: %s", spew.Sdump(planB))
1061+
}
1062+
1063+
_, diags = ctx.Apply(plan, m)
1064+
if !diags.HasErrors() {
1065+
t.Fatal("final apply succeeded, but should've failed with a postcondition error")
1066+
}
1067+
if len(diags) != 1 {
1068+
t.Fatalf("expected exactly one diagnostic, but got: %s", diags.Err().Error())
1069+
}
1070+
if got, want := diags[0].Description().Summary, "Resource postcondition failed"; got != want {
1071+
t.Fatalf("wrong diagnostic summary\ngot: %s\nwant: %s", got, want)
1072+
}
1073+
}
1074+
}
1075+
9171076
// pass an input through some expanded values, and back to a provider to make
9181077
// sure we can fully evaluate a provider configuration during a destroy plan.
9191078
func TestContext2Apply_destroyWithConfiguredProvider(t *testing.T) {

‎internal/terraform/node_resource_abstract_instance.go‎

Lines changed: 23 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,8 @@ import (
66
"strings"
77

88
"github.com/hashicorp/hcl/v2"
9+
"github.com/zclconf/go-cty/cty"
10+
911
"github.com/hashicorp/terraform/internal/addrs"
1012
"github.com/hashicorp/terraform/internal/configs"
1113
"github.com/hashicorp/terraform/internal/configs/configschema"
@@ -16,7 +18,6 @@ import (
1618
"github.com/hashicorp/terraform/internal/provisioners"
1719
"github.com/hashicorp/terraform/internal/states"
1820
"github.com/hashicorp/terraform/internal/tfdiags"
19-
"github.com/zclconf/go-cty/cty"
2021
)
2122

2223
// NodeAbstractResourceInstance represents a resource instance with no
@@ -1710,7 +1711,7 @@ func (n *NodeAbstractResourceInstance) applyDataSource(ctx EvalContext, planned
17101711
return nil, keyData, diags.Append(fmt.Errorf("provider schema not available for %s", n.Addr))
17111712
}
17121713

1713-
if planned != nil && planned.Action != plans.Read {
1714+
if planned != nil && planned.Action != plans.Read && planned.Action != plans.NoOp {
17141715
// If any other action gets in here then that's always a bug; this
17151716
// EvalNode only deals with reading.
17161717
diags = diags.Append(fmt.Errorf(
@@ -1745,6 +1746,13 @@ func (n *NodeAbstractResourceInstance) applyDataSource(ctx EvalContext, planned
17451746
return nil, keyData, diags // failed preconditions prevent further evaluation
17461747
}
17471748

1749+
if planned.Action == plans.NoOp {
1750+
// If we didn't actually plan to read this then we have nothing more
1751+
// to do; we're evaluating this only for incidentals like the
1752+
// precondition/postcondition checks.
1753+
return nil, keyData, diags
1754+
}
1755+
17481756
configVal, _, configDiags := ctx.EvaluateBlock(config.Config, schema, nil, keyData)
17491757
diags = diags.Append(configDiags)
17501758
if configDiags.HasErrors() {
@@ -2042,6 +2050,19 @@ func (n *NodeAbstractResourceInstance) apply(
20422050
state = &states.ResourceInstanceObject{}
20432051
}
20442052

2053+
if applyConfig != nil {
2054+
forEach, _ := evaluateForEachExpression(applyConfig.ForEach, ctx)
2055+
keyData = EvalDataForInstanceKey(n.ResourceInstanceAddr().Resource.Key, forEach)
2056+
}
2057+
2058+
if change.Action == plans.NoOp {
2059+
// If this is a no-op change then we don't want to actually change
2060+
// anything, so we'll just echo back the state we were given and
2061+
// let our internal checks and updates proceed.
2062+
log.Printf("[TRACE] NodeAbstractResourceInstance.apply: skipping %s because it has no planned action", n.Addr)
2063+
return state, keyData, diags
2064+
}
2065+
20452066
provider, providerSchema, err := getProvider(ctx, n.ResolvedProvider)
20462067
if err != nil {
20472068
return nil, keyData, diags.Append(err)
@@ -2058,8 +2079,6 @@ func (n *NodeAbstractResourceInstance) apply(
20582079
configVal := cty.NullVal(cty.DynamicPseudoType)
20592080
if applyConfig != nil {
20602081
var configDiags tfdiags.Diagnostics
2061-
forEach, _ := evaluateForEachExpression(applyConfig.ForEach, ctx)
2062-
keyData = EvalDataForInstanceKey(n.ResourceInstanceAddr().Resource.Key, forEach)
20632082
configVal, _, configDiags = ctx.EvaluateBlock(applyConfig.Config, schema, nil, keyData)
20642083
diags = diags.Append(configDiags)
20652084
if configDiags.HasErrors() {

0 commit comments

Comments
 (0)