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.

Fix the null data source test resource, as it had no computed fields at
all, even the id.

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/context_test.go

@@ -693,11 +693,12 @@ func testProviderSchema(name string) *ProviderSchema {
 				Attributes: map[string]*configschema.Attribute{
 					"id": {
 						Type:     cty.String,
-						Optional: true,
+						Computed: true,
 					},
 					"foo": {
 						Type:     cty.String,
 						Optional: true,
+						Computed: true,
 					},
 				},
 			},

terraform/eval_read_data.go

@@ -236,19 +236,22 @@ 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 we've never read this data source and we
+	// have any depends_on, we will have to defer reading until plan to resolve
+	// the dependency changes.
+	// Assuming we can read the data source with depends_on if we have
+	// existing state is a compromise to prevent data sources from continually
+	// showing a diff. We have to make the assumption that if we have a prior
+	// state, since there are no prior dependency changes happening during
+	// refresh, that we can read this resource.
+	if !configKnown || (priorVal.IsNull() && len(n.Config.DependsOn) > 0) {
 		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,21 +261,17 @@ func (n *EvalReadDataRefresh) Eval(ctx EvalContext) (interface{}, error) {
 			Change: plans.Change{
 				Action: plans.Read,
 				Before: priorVal,
-				After:  proposedNewVal,
+				After:  objchange.PlannedDataResourceObject(schema, configVal),
 			},
 		}
 
 		if n.OutputChange != nil {
 			*n.OutputChange = change
 		}
+
 		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 +285,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,42 +102,43 @@ 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() {
 		return nil, diags.ErrWithWarnings()
 	}
 
-	action := plans.NoOp
-	if !newVal.IsNull() && newVal.IsKnown() && newVal.Equals(priorVal).False() {
-		// since a data source is read-only, update here only means that we
-		// need to update the state.
-		action = plans.Update
-	}
-
 	// Produce a change regardless of the outcome.
 	change := &plans.ResourceInstanceChange{
 		Addr:         absAddr,
 		ProviderAddr: n.ProviderAddr,
 		Change: plans.Change{
-			Action: action,
+			Action: plans.Update,
 			Before: priorVal,
 			After:  newVal,
 		},
 	}
 
-	status := states.ObjectReady
-	if action == plans.Update {
-		status = states.ObjectPlanned
-	}
-
 	outputState := &states.ResourceInstanceObject{
 		Value:  newVal,
-		Status: status,
+		Status: states.ObjectPlanned,
 	}
 
 	if err := ctx.Hook(func(h Hook) (HookAction, error) {
-		return h.PostDiff(absAddr, states.CurrentGen, action, priorVal, newVal)
+		return h.PostDiff(absAddr, states.CurrentGen, plans.Update, priorVal, newVal)
 	}); err != nil {
 		return nil, err
 	}

terraform/testdata/apply-data-depends-on/main.tf

@@ -3,6 +3,5 @@ resource "null_instance" "write" {
 }
 
 data "null_data_source" "read" {
-	foo = ""
 	depends_on = ["null_instance.write"]
 }

terraform/transform_reference.go

@@ -51,7 +51,7 @@ type GraphNodeAttachDependencies interface {
 // not yet expended in the graph. While this will cause some extra data
 // resources to show in the plan when their depends_on references may be in
 // unrelated module instances, the fact that it only happens when there are any
-// resource updates pending means we ca still avoid the problem of the
+// resource updates pending means we can still avoid the problem of the
 // "perpetual diff"
 type GraphNodeAttachDependsOn interface {
 	GraphNodeConfigResource
@@ -83,7 +83,7 @@ type GraphNodeReferenceOutside interface {
 	ReferenceOutside() (selfPath, referencePath addrs.Module)
 }
 
-// Referenceeransformer is a GraphTransformer that connects all the
+// ReferenceTransformer is a GraphTransformer that connects all the
 // nodes that reference each other in order to form the proper ordering.
 type ReferenceTransformer struct{}