check for data source changed during plan

Rather than re-read the data source during every plan cycle, apply the
config to the prior state, and skip reading if there is no change.

Remove the TODOs, as we're going to accept that data-only changes will
still not be plan-able for the time being.

Author: jbardin

terraform/context_apply_test.go

@@ -8782,6 +8782,16 @@ func TestContext2Apply_dataDependsOn(t *testing.T) {
 	if actual != expected {
 		t.Fatalf("bad:\n%s", strings.TrimSpace(state.String()))
 	}
+
+	// run another plan to make sure the data source doesn't show as a change
+	plan, diags := ctx.Plan()
+	assertNoErrors(t, diags)
+
+	for _, c := range plan.Changes.Resources {
+		if c.Action != plans.NoOp {
+			t.Fatalf("unexpected change for %s", c.Addr)
+		}
+	}
 }
 
 func TestContext2Apply_terraformWorkspace(t *testing.T) {

terraform/context_plan_test.go

@@ -1892,8 +1892,9 @@ func TestContext2Plan_computedInFunction(t *testing.T) {
 	_, diags = ctx.Plan() // should do nothing with data resource in this step, since it was already read
 	assertNoErrors(t, diags)
 
-	if !p.ReadDataSourceCalled {
-		t.Fatalf("ReadDataSource should have been called")
+	if p.ReadDataSourceCalled {
+		// there was no config change to read during plan
+		t.Fatalf("ReadDataSource should not have been called")
 	}
 }
 

terraform/eval_read_data.go

@@ -236,19 +236,15 @@ func (n *EvalReadDataRefresh) Eval(ctx EvalContext) (interface{}, error) {
 	}
 
 	configKnown := configVal.IsWhollyKnown()
-	// If our configuration contains any unknown values, or we depend on any
-	// unknown values then we must defer the read to the apply phase by
-	// producing a "Read" change for this resource, and a placeholder value for
-	// it in the state.
-	if len(n.Config.DependsOn) > 0 || !configKnown {
+	// If our configuration contains any unknown values, then we must defer the
+	// read until plan or apply.
+	if !configKnown {
 		if configKnown {
 			log.Printf("[TRACE] EvalReadDataRefresh: %s configuration is fully known, but we're forcing a read plan to be created", absAddr)
 		} else {
 			log.Printf("[TRACE] EvalReadDataRefresh: %s configuration not fully known yet, so deferring to apply phase", absAddr)
 		}
 
-		proposedNewVal := objchange.PlannedDataResourceObject(schema, configVal)
-
 		// We need to store a change so tat other references to this data
 		// source can resolve correctly, since the state is not going to be up
 		// to date.
@@ -258,7 +254,7 @@ func (n *EvalReadDataRefresh) Eval(ctx EvalContext) (interface{}, error) {
 			Change: plans.Change{
 				Action: plans.Read,
 				Before: priorVal,
-				After:  proposedNewVal,
+				After:  objchange.PlannedDataResourceObject(schema, configVal),
 			},
 		}
 
@@ -267,12 +263,7 @@ func (n *EvalReadDataRefresh) Eval(ctx EvalContext) (interface{}, error) {
 		}
 		if n.State != nil {
 			*n.State = &states.ResourceInstanceObject{
-				// We need to keep the prior value in the state so that plan
-				// has something to diff against.
-				Value: priorVal,
-				// TODO: this needs to be ObjectPlanned to trigger a plan, but
-				// the prior value is lost preventing plan from resulting in a
-				// NoOp
+				Value:  cty.NullVal(objTy),
 				Status: states.ObjectPlanned,
 			}
 		}
@@ -286,9 +277,8 @@ func (n *EvalReadDataRefresh) Eval(ctx EvalContext) (interface{}, error) {
 		return nil, diags.ErrWithWarnings()
 	}
 
-	// TODO: Need to signal to plan that this may have changed. We may be able
-	// to use ObjectPlanned for that, but that currently causes the state to be
-	// dropped altogether
+	// This may still have been refreshed with references to resources that
+	// will be updated, but that will be caught as a change during plan.
 	outputState := &states.ResourceInstanceObject{
 		Value:  newVal,
 		Status: states.ObjectReady,

terraform/eval_read_data_plan.go

@@ -61,7 +61,6 @@ func (n *EvalReadDataPlan) Eval(ctx EvalContext) (interface{}, error) {
 	}
 
 	configKnown := configVal.IsWhollyKnown()
-	proposedNewVal := objchange.PlannedDataResourceObject(schema, configVal)
 	// If our configuration contains any unknown values, or we depend on any
 	// unknown values then we must defer the read to the apply phase by
 	// producing a "Read" change for this resource, and a placeholder value for
@@ -73,6 +72,8 @@ func (n *EvalReadDataPlan) Eval(ctx EvalContext) (interface{}, error) {
 			log.Printf("[TRACE] EvalReadDataPlan: %s configuration not fully known yet, so deferring to apply phase", absAddr)
 		}
 
+		proposedNewVal := objchange.PlannedDataResourceObject(schema, configVal)
+
 		err := ctx.Hook(func(h Hook) (HookAction, error) {
 			return h.PreDiff(absAddr, states.CurrentGen, priorVal, proposedNewVal)
 		})
@@ -101,6 +102,19 @@ func (n *EvalReadDataPlan) Eval(ctx EvalContext) (interface{}, error) {
 		return nil, diags.ErrWithWarnings()
 	}
 
+	// If we have a stored state we may not need to re-read the data source.
+	// Check the config against the state to see if there are any difference.
+	if !priorVal.IsNull() {
+		// Applying the configuration to the prior state lets us see if there
+		// are any differences.
+		proposed := objchange.ProposedNewObject(schema, priorVal, configVal)
+		if proposed.Equals(priorVal).True() {
+			log.Printf("[TRACE] EvalReadDataPlan: %s no change detected, using existing state", absAddr)
+			// state looks up to date, and must have been read during refresh
+			return nil, diags.ErrWithWarnings()
+		}
+	}
+
 	newVal, readDiags := n.readDataSource(ctx, configVal)
 	diags = diags.Append(readDiags)
 	if diags.HasErrors() {