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.

internal/command/views/hook_json.go

@@ -7,13 +7,14 @@ import (
 	"time"
 	"unicode"
 
+	"github.com/zclconf/go-cty/cty"
+
 	"github.com/hashicorp/terraform/internal/addrs"
 	"github.com/hashicorp/terraform/internal/command/format"
 	"github.com/hashicorp/terraform/internal/command/views/json"
 	"github.com/hashicorp/terraform/internal/plans"
 	"github.com/hashicorp/terraform/internal/states"
 	"github.com/hashicorp/terraform/internal/terraform"
-	"github.com/zclconf/go-cty/cty"
 )
 
 // How long to wait between sending heartbeat/progress messages
@@ -59,8 +60,10 @@ type applyProgress struct {
 }
 
 func (h *jsonHook) PreApply(addr addrs.AbsResourceInstance, gen states.Generation, action plans.Action, priorState, plannedNewState cty.Value) (terraform.HookAction, error) {
-	idKey, idValue := format.ObjectValueIDOrName(priorState)
-	h.view.Hook(json.NewApplyStart(addr, action, idKey, idValue))
+	if action != plans.NoOp {
+		idKey, idValue := format.ObjectValueIDOrName(priorState)
+		h.view.Hook(json.NewApplyStart(addr, action, idKey, idValue))
+	}
 
 	progress := applyProgress{
 		addr:          addr,
@@ -73,7 +76,9 @@ func (h *jsonHook) PreApply(addr addrs.AbsResourceInstance, gen states.Generatio
 	h.applying[addr.String()] = progress
 	h.applyingLock.Unlock()
 
-	go h.applyingHeartbeat(progress)
+	if action != plans.NoOp {
+		go h.applyingHeartbeat(progress)
+	}
 	return terraform.HookActionContinue, nil
 }
 
@@ -101,6 +106,10 @@ func (h *jsonHook) PostApply(addr addrs.AbsResourceInstance, gen states.Generati
 	delete(h.applying, key)
 	h.applyingLock.Unlock()
 
+	if progress.action == plans.NoOp {
+		return terraform.HookActionContinue, nil
+	}
+
 	elapsed := h.timeNow().Round(time.Second).Sub(progress.start)
 
 	if err != nil {

internal/command/views/hook_ui.go

@@ -65,6 +65,7 @@ const (
 	uiResourceModify
 	uiResourceDestroy
 	uiResourceRead
+	uiResourceNoOp
 )
 
 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,
 	case plans.Read:
 		operation = "Reading..."
 		op = uiResourceRead
+	case plans.NoOp:
+		op = uiResourceNoOp
 	default:
 		// We don't expect any other actions in here, so anything else is a
 		// 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,
 		idValue = ""
 	}
 
-	h.println(fmt.Sprintf(
-		h.view.colorize.Color("[reset][bold]%s: %s%s[reset]"),
-		dispAddr,
-		operation,
-		stateIdSuffix,
-	))
+	if operation != "" {
+		h.println(fmt.Sprintf(
+			h.view.colorize.Color("[reset][bold]%s: %s%s[reset]"),
+			dispAddr,
+			operation,
+			stateIdSuffix,
+		))
+	}
 
 	key := addr.String()
 	uiState := uiResourceState{
@@ -129,7 +134,9 @@ func (h *UiHook) PreApply(addr addrs.AbsResourceInstance, gen states.Generation,
 	h.resourcesLock.Unlock()
 
 	// Start goroutine that shows progress
-	go h.stillApplying(uiState)
+	if op != uiResourceNoOp {
+		go h.stillApplying(uiState)
+	}
 
 	return terraform.HookActionContinue, nil
 }
@@ -201,6 +208,9 @@ func (h *UiHook) PostApply(addr addrs.AbsResourceInstance, gen states.Generation
 		msg = "Creation complete"
 	case uiResourceRead:
 		msg = "Read complete"
+	case uiResourceNoOp:
+		// We don't make any announcements about no-op changes
+		return terraform.HookActionContinue, nil
 	case uiResourceUnknown:
 		return terraform.HookActionContinue, nil
 	}

internal/terraform/context_apply2_test.go

@@ -1,6 +1,7 @@
 package terraform
 
 import (
+	"bytes"
 	"errors"
 	"fmt"
 	"strings"
@@ -8,14 +9,16 @@ import (
 	"testing"
 	"time"
 
+	"github.com/davecgh/go-spew/spew"
 	"github.com/google/go-cmp/cmp"
+	"github.com/zclconf/go-cty/cty"
+
 	"github.com/hashicorp/terraform/internal/addrs"
 	"github.com/hashicorp/terraform/internal/configs/configschema"
 	"github.com/hashicorp/terraform/internal/lang/marks"
 	"github.com/hashicorp/terraform/internal/plans"
 	"github.com/hashicorp/terraform/internal/providers"
 	"github.com/hashicorp/terraform/internal/states"
-	"github.com/zclconf/go-cty/cty"
 )
 
 // Test that the PreApply hook is called with the correct deposed key
@@ -914,6 +917,162 @@ resource "test_resource" "c" {
 	})
 }
 
+func TestContext2Apply_resourceConditionApplyTimeFail(t *testing.T) {
+	// This tests the less common situation where a condition fails due to
+	// a change in a resource other than the one the condition is attached to,
+	// and the condition result is unknown during planning.
+	//
+	// This edge case is a tricky one because it relies on Terraform still
+	// visiting test_resource.b (in the configuration below) to evaluate
+	// its conditions even though there aren't any changes directly planned
+	// for it, so that we can consider whether changes to test_resource.a
+	// have changed the outcome.
+
+	m := testModuleInline(t, map[string]string{
+		"main.tf": `
+			variable "input" {
+				type = string
+			}
+
+			resource "test_resource" "a" {
+				value = var.input
+			}
+
+			resource "test_resource" "b" {
+				value = "beep"
+
+				lifecycle {
+					postcondition {
+						condition     = test_resource.a.output == self.output
+						error_message = "Outputs must match."
+					}
+				}
+			}
+		`,
+	})
+
+	p := testProvider("test")
+	p.GetProviderSchemaResponse = getProviderSchemaResponseFromProviderSchema(&ProviderSchema{
+		ResourceTypes: map[string]*configschema.Block{
+			"test_resource": {
+				Attributes: map[string]*configschema.Attribute{
+					"value": {
+						Type:     cty.String,
+						Required: true,
+					},
+					"output": {
+						Type:     cty.String,
+						Computed: true,
+					},
+				},
+			},
+		},
+	})
+	p.PlanResourceChangeFn = func(req providers.PlanResourceChangeRequest) (resp providers.PlanResourceChangeResponse) {
+		// Whenever "value" changes, "output" follows it during the apply step,
+		// but is initially unknown during the plan step.
+
+		m := req.ProposedNewState.AsValueMap()
+		priorVal := cty.NullVal(cty.String)
+		if !req.PriorState.IsNull() {
+			priorVal = req.PriorState.GetAttr("value")
+		}
+		if m["output"].IsNull() || !priorVal.RawEquals(m["value"]) {
+			m["output"] = cty.UnknownVal(cty.String)
+		}
+
+		resp.PlannedState = cty.ObjectVal(m)
+		resp.LegacyTypeSystem = true
+		return resp
+	}
+	p.ApplyResourceChangeFn = func(req providers.ApplyResourceChangeRequest) (resp providers.ApplyResourceChangeResponse) {
+		m := req.PlannedState.AsValueMap()
+		m["output"] = m["value"]
+		resp.NewState = cty.ObjectVal(m)
+		return resp
+	}
+	ctx := testContext2(t, &ContextOpts{
+		Providers: map[addrs.Provider]providers.Factory{
+			addrs.NewDefaultProvider("test"): testProviderFuncFixed(p),
+		},
+	})
+	instA := mustResourceInstanceAddr("test_resource.a")
+	instB := mustResourceInstanceAddr("test_resource.b")
+
+	// Preparation: an initial plan and apply with a correct input variable
+	// should succeed and give us a valid and complete state to use for the
+	// subsequent plan and apply that we'll expect to fail.
+	var prevRunState *states.State
+	{
+		plan, diags := ctx.Plan(m, states.NewState(), &PlanOpts{
+			Mode: plans.NormalMode,
+			SetVariables: InputValues{
+				"input": &InputValue{
+					Value:      cty.StringVal("beep"),
+					SourceType: ValueFromCLIArg,
+				},
+			},
+		})
+		assertNoErrors(t, diags)
+		planA := plan.Changes.ResourceInstance(instA)
+		if planA == nil || planA.Action != plans.Create {
+			t.Fatalf("incorrect initial plan for instance A\nwant a 'create' change\ngot: %s", spew.Sdump(planA))
+		}
+		planB := plan.Changes.ResourceInstance(instB)
+		if planB == nil || planB.Action != plans.Create {
+			t.Fatalf("incorrect initial plan for instance B\nwant a 'create' change\ngot: %s", spew.Sdump(planB))
+		}
+
+		state, diags := ctx.Apply(plan, m)
+		assertNoErrors(t, diags)
+
+		stateA := state.ResourceInstance(instA)
+		if stateA == nil || stateA.Current == nil || !bytes.Contains(stateA.Current.AttrsJSON, []byte(`"beep"`)) {
+			t.Fatalf("incorrect initial state for instance A\ngot: %s", spew.Sdump(stateA))
+		}
+		stateB := state.ResourceInstance(instB)
+		if stateB == nil || stateB.Current == nil || !bytes.Contains(stateB.Current.AttrsJSON, []byte(`"beep"`)) {
+			t.Fatalf("incorrect initial state for instance B\ngot: %s", spew.Sdump(stateB))
+		}
+		prevRunState = state
+	}
+
+	// Now we'll run another plan and apply with a different value for
+	// var.input that should cause the test_resource.b condition to be unknown
+	// during planning and then fail during apply.
+	{
+		plan, diags := ctx.Plan(m, prevRunState, &PlanOpts{
+			Mode: plans.NormalMode,
+			SetVariables: InputValues{
+				"input": &InputValue{
+					Value:      cty.StringVal("boop"), // NOTE: This has changed
+					SourceType: ValueFromCLIArg,
+				},
+			},
+		})
+		assertNoErrors(t, diags)
+		planA := plan.Changes.ResourceInstance(instA)
+		if planA == nil || planA.Action != plans.Update {
+			t.Fatalf("incorrect initial plan for instance A\nwant an 'update' change\ngot: %s", spew.Sdump(planA))
+		}
+		planB := plan.Changes.ResourceInstance(instB)
+		if planB == nil || planB.Action != plans.NoOp {
+			t.Fatalf("incorrect initial plan for instance B\nwant a 'no-op' change\ngot: %s", spew.Sdump(planB))
+		}
+
+		_, diags = ctx.Apply(plan, m)
+		if !diags.HasErrors() {
+			t.Fatal("final apply succeeded, but should've failed with a postcondition error")
+		}
+		if len(diags) != 1 {
+			t.Fatalf("expected exactly one diagnostic, but got: %s", diags.Err().Error())
+		}
+		if got, want := diags[0].Description().Summary, "Resource postcondition failed"; got != want {
+			t.Fatalf("wrong diagnostic summary\ngot:  %s\nwant: %s", got, want)
+		}
+	}
+}
+
 // pass an input through some expanded values, and back to a provider to make
 // sure we can fully evaluate a provider configuration during a destroy plan.
 func TestContext2Apply_destroyWithConfiguredProvider(t *testing.T) {

internal/terraform/node_resource_abstract_instance.go

@@ -6,6 +6,8 @@ import (
 	"strings"
 
 	"github.com/hashicorp/hcl/v2"
+	"github.com/zclconf/go-cty/cty"
+
 	"github.com/hashicorp/terraform/internal/addrs"
 	"github.com/hashicorp/terraform/internal/configs"
 	"github.com/hashicorp/terraform/internal/configs/configschema"
@@ -16,7 +18,6 @@ import (
 	"github.com/hashicorp/terraform/internal/provisioners"
 	"github.com/hashicorp/terraform/internal/states"
 	"github.com/hashicorp/terraform/internal/tfdiags"
-	"github.com/zclconf/go-cty/cty"
 )
 
 // NodeAbstractResourceInstance represents a resource instance with no
@@ -1710,7 +1711,7 @@ func (n *NodeAbstractResourceInstance) applyDataSource(ctx EvalContext, planned
 		return nil, keyData, diags.Append(fmt.Errorf("provider schema not available for %s", n.Addr))
 	}
 
-	if planned != nil && planned.Action != plans.Read {
+	if planned != nil && planned.Action != plans.Read && planned.Action != plans.NoOp {
 		// If any other action gets in here then that's always a bug; this
 		// EvalNode only deals with reading.
 		diags = diags.Append(fmt.Errorf(
@@ -1745,6 +1746,13 @@ func (n *NodeAbstractResourceInstance) applyDataSource(ctx EvalContext, planned
 		return nil, keyData, diags // failed preconditions prevent further evaluation
 	}
 
+	if planned.Action == plans.NoOp {
+		// If we didn't actually plan to read this then we have nothing more
+		// to do; we're evaluating this only for incidentals like the
+		// precondition/postcondition checks.
+		return nil, keyData, diags
+	}
+
 	configVal, _, configDiags := ctx.EvaluateBlock(config.Config, schema, nil, keyData)
 	diags = diags.Append(configDiags)
 	if configDiags.HasErrors() {
@@ -2042,6 +2050,19 @@ func (n *NodeAbstractResourceInstance) apply(
 		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
+	}
+
 	provider, providerSchema, err := getProvider(ctx, n.ResolvedProvider)
 	if err != nil {
 		return nil, keyData, diags.Append(err)
@@ -2058,8 +2079,6 @@ func (n *NodeAbstractResourceInstance) apply(
 	configVal := cty.NullVal(cty.DynamicPseudoType)
 	if applyConfig != nil {
 		var configDiags tfdiags.Diagnostics
-		forEach, _ := evaluateForEachExpression(applyConfig.ForEach, ctx)
-		keyData = EvalDataForInstanceKey(n.ResourceInstanceAddr().Resource.Key, forEach)
 		configVal, _, configDiags = ctx.EvaluateBlock(applyConfig.Config, schema, nil, keyData)
 		diags = diags.Append(configDiags)
 		if configDiags.HasErrors() {

internal/terraform/node_resource_apply_instance.go

@@ -153,10 +153,10 @@ func (n *NodeApplyableResourceInstance) dataResourceExecute(ctx EvalContext) (di
 		return diags
 	}
 	// Stop early if we don't actually have a diff
-	if change == nil || change.Action == plans.NoOp {
+	if change == nil {
 		return diags
 	}
-	if change.Action != plans.Read {
+	if change.Action != plans.Read && change.Action != plans.NoOp {
 		diags = diags.Append(fmt.Errorf("nonsensical planned action %#v for %s; this is a bug in Terraform", change.Action, n.Addr))
 	}
 
@@ -169,9 +169,16 @@ func (n *NodeApplyableResourceInstance) dataResourceExecute(ctx EvalContext) (di
 		return diags
 	}
 
-	diags = diags.Append(n.writeResourceInstanceState(ctx, state, workingState))
-	if diags.HasErrors() {
-		return diags
+	if state != nil {
+		// If n.applyDataSource returned a nil state object with no accompanying
+		// errors then it determined that the given change doesn't require
+		// actually reading the data (e.g. because it was already read during
+		// the plan phase) and so we're only running through here to get the
+		// extra details like precondition/postcondition checks.
+		diags = diags.Append(n.writeResourceInstanceState(ctx, state, workingState))
+		if diags.HasErrors() {
+			return diags
+		}
 	}
 
 	diags = diags.Append(n.writeChange(ctx, nil, ""))
@@ -217,7 +224,7 @@ func (n *NodeApplyableResourceInstance) managedResourceExecute(ctx EvalContext)
 
 	// We don't want to do any destroys
 	// (these are handled by NodeDestroyResourceInstance instead)
-	if diffApply == nil || diffApply.Action == plans.NoOp || diffApply.Action == plans.Delete {
+	if diffApply == nil || diffApply.Action == plans.Delete {
 		return diags
 	}
 	if diffApply.Action == plans.Read {
@@ -273,10 +280,10 @@ func (n *NodeApplyableResourceInstance) managedResourceExecute(ctx EvalContext)
 	diffApply = reducePlan(addr, diffApply, false)
 	// reducePlan may have simplified our planned change
 	// into a NoOp if it only requires destroying, since destroying
-	// is handled by NodeDestroyResourceInstance.
-	if diffApply == nil || diffApply.Action == plans.NoOp {
-		return diags
-	}
+	// is handled by NodeDestroyResourceInstance. If so, we'll
+	// still run through most of the logic here because we do still
+	// need to deal with other book-keeping such as marking the
+	// change as "complete", and running the author's postconditions.
 
 	diags = diags.Append(n.preApplyHook(ctx, diffApply))
 	if diags.HasErrors() {