From cb5ce1d35ea016f7ea38aa0b9fc84fa85ed0bd19 Mon Sep 17 00:00:00 2001 From: Chris Marchesi Date: Tue, 19 Dec 2017 15:46:58 -0800 Subject: [PATCH] helper/schema: Extend diffChange and bubble up customized values This extends the internal diffChange method so that ResourceDiff's implementation of it can report back whether or not the value came from a customized diff. This is an effort to work to preserve the pre-ResourceDiff behaviour that ignores the diff for computed keys when the old value was populated but the new value wasn't - this behaviour is actually being depended on by users that are using it to exploit using zero values in modules. This should allow both scenarios to co-exist by shifting the NewComputed exemption over to exempting values that come from diff customization. --- helper/schema/resource_data.go | 4 +- helper/schema/resource_diff.go | 15 +++-- helper/schema/schema.go | 106 +++++++++++++++++++-------------- 3 files changed, 69 insertions(+), 56 deletions(-) diff --git a/helper/schema/resource_data.go b/helper/schema/resource_data.go index 970dc7b99..9ab8bccaa 100644 --- a/helper/schema/resource_data.go +++ b/helper/schema/resource_data.go @@ -445,7 +445,7 @@ func (d *ResourceData) init() { } func (d *ResourceData) diffChange( - k string) (interface{}, interface{}, bool, bool) { + k string) (interface{}, interface{}, bool, bool, bool) { // Get the change between the state and the config. o, n := d.getChange(k, getSourceState, getSourceConfig|getSourceExact) if !o.Exists { @@ -456,7 +456,7 @@ func (d *ResourceData) diffChange( } // Return the old, new, and whether there is a change - return o.Value, n.Value, !reflect.DeepEqual(o.Value, n.Value), n.Computed + return o.Value, n.Value, !reflect.DeepEqual(o.Value, n.Value), n.Computed, false } func (d *ResourceData) getChange( diff --git a/helper/schema/resource_diff.go b/helper/schema/resource_diff.go index 4fc1dbb68..822d0dc4d 100644 --- a/helper/schema/resource_diff.go +++ b/helper/schema/resource_diff.go @@ -236,8 +236,8 @@ func (d *ResourceDiff) clear(key string) error { // diffChange helps to implement resourceDiffer and derives its change values // from ResourceDiff's own change data, in addition to existing diff, config, and state. -func (d *ResourceDiff) diffChange(key string) (interface{}, interface{}, bool, bool) { - old, new := d.getChange(key) +func (d *ResourceDiff) diffChange(key string) (interface{}, interface{}, bool, bool, bool) { + old, new, customized := d.getChange(key) if !old.Exists { old.Value = nil @@ -246,7 +246,7 @@ func (d *ResourceDiff) diffChange(key string) (interface{}, interface{}, bool, b new.Value = nil } - return old.Value, new.Value, !reflect.DeepEqual(old.Value, new.Value), new.Computed + return old.Value, new.Value, !reflect.DeepEqual(old.Value, new.Value), new.Computed, customized } // SetNew is used to set a new diff value for the mentioned key. The value must @@ -327,7 +327,7 @@ func (d *ResourceDiff) Get(key string) interface{} { // results from the exact levels for the new diff, then from state and diff as // per normal. func (d *ResourceDiff) GetChange(key string) (interface{}, interface{}) { - old, new := d.getChange(key) + old, new, _ := d.getChange(key) return old.Value, new.Value } @@ -387,18 +387,17 @@ func (d *ResourceDiff) Id() string { // This implementation differs from ResourceData's in the way that we first get // results from the exact levels for the new diff, then from state and diff as // per normal. -func (d *ResourceDiff) getChange(key string) (getResult, getResult) { +func (d *ResourceDiff) getChange(key string) (getResult, getResult, bool) { old := d.get(strings.Split(key, "."), "state") var new getResult for p := range d.updatedKeys { if childAddrOf(key, p) { new = d.getExact(strings.Split(key, "."), "newDiff") - goto done + return old, new, true } } new = d.get(strings.Split(key, "."), "newDiff") -done: - return old, new + return old, new, false } // get performs the appropriate multi-level reader logic for ResourceDiff, diff --git a/helper/schema/schema.go b/helper/schema/schema.go index 3eff0a380..edcf58e8f 100644 --- a/helper/schema/schema.go +++ b/helper/schema/schema.go @@ -296,8 +296,7 @@ func (s *Schema) ZeroValue() interface{} { } } -func (s *Schema) finalizeDiff( - d *terraform.ResourceAttrDiff) *terraform.ResourceAttrDiff { +func (s *Schema) finalizeDiff(d *terraform.ResourceAttrDiff, customized bool) *terraform.ResourceAttrDiff { if d == nil { return d } @@ -337,20 +336,16 @@ func (s *Schema) finalizeDiff( return d } - if s.Computed && d.NewComputed && d.Old != "" && d.New == "" { - // old case where we pretended that a NewComputed value of "" on a - // Computed field was unset - return nil - } - - if s.Computed && !d.NewComputed { - if d.Old != "" && d.New == "" { - // This is a computed value with an old value set already, - // just let it go. - return nil + if s.Computed { + if !customized { + if d.Old != "" && d.New == "" { + // This is a computed value with an old value set already, + // just let it go. + return nil + } } - if d.New == "" { + if d.New == "" && !d.NewComputed { // Computed attribute without a new value set d.NewComputed = true } @@ -750,7 +745,7 @@ func isValidFieldName(name string) bool { // This helps facilitate diff logic for both ResourceData and ResoureDiff with // minimal divergence in code. type resourceDiffer interface { - diffChange(string) (interface{}, interface{}, bool, bool) + diffChange(string) (interface{}, interface{}, bool, bool, bool) Get(string) interface{} GetChange(string) (interface{}, interface{}) GetOk(string) (interface{}, bool) @@ -803,7 +798,7 @@ func (m schemaMap) diffList( diff *terraform.InstanceDiff, d resourceDiffer, all bool) error { - o, n, _, computedList := d.diffChange(k) + o, n, _, computedList, customized := d.diffChange(k) if computedList { n = nil } @@ -870,10 +865,13 @@ func (m schemaMap) diffList( oldStr = "" } - diff.Attributes[k+".#"] = countSchema.finalizeDiff(&terraform.ResourceAttrDiff{ - Old: oldStr, - New: newStr, - }) + diff.Attributes[k+".#"] = countSchema.finalizeDiff( + &terraform.ResourceAttrDiff{ + Old: oldStr, + New: newStr, + }, + customized, + ) } // Figure out the maximum @@ -926,7 +924,7 @@ func (m schemaMap) diffMap( // First get all the values from the state var stateMap, configMap map[string]string - o, n, _, nComputed := d.diffChange(k) + o, n, _, nComputed, customized := d.diffChange(k) if err := mapstructure.WeakDecode(o, &stateMap); err != nil { return fmt.Errorf("%s: %s", k, err) } @@ -978,6 +976,7 @@ func (m schemaMap) diffMap( Old: oldStr, New: newStr, }, + customized, ) } @@ -995,16 +994,22 @@ func (m schemaMap) diffMap( continue } - diff.Attributes[prefix+k] = schema.finalizeDiff(&terraform.ResourceAttrDiff{ - Old: old, - New: v, - }) + diff.Attributes[prefix+k] = schema.finalizeDiff( + &terraform.ResourceAttrDiff{ + Old: old, + New: v, + }, + customized, + ) } for k, v := range stateMap { - diff.Attributes[prefix+k] = schema.finalizeDiff(&terraform.ResourceAttrDiff{ - Old: v, - NewRemoved: true, - }) + diff.Attributes[prefix+k] = schema.finalizeDiff( + &terraform.ResourceAttrDiff{ + Old: v, + NewRemoved: true, + }, + customized, + ) } return nil @@ -1017,7 +1022,7 @@ func (m schemaMap) diffSet( d resourceDiffer, all bool) error { - o, n, _, computedSet := d.diffChange(k) + o, n, _, computedSet, customized := d.diffChange(k) if computedSet { n = nil } @@ -1076,20 +1081,26 @@ func (m schemaMap) diffSet( countStr = "" } - diff.Attributes[k+".#"] = countSchema.finalizeDiff(&terraform.ResourceAttrDiff{ - Old: countStr, - NewComputed: true, - }) + diff.Attributes[k+".#"] = countSchema.finalizeDiff( + &terraform.ResourceAttrDiff{ + Old: countStr, + NewComputed: true, + }, + customized, + ) return nil } // If the counts are not the same, then record that diff changed := oldLen != newLen if changed || all { - diff.Attributes[k+".#"] = countSchema.finalizeDiff(&terraform.ResourceAttrDiff{ - Old: oldStr, - New: newStr, - }) + diff.Attributes[k+".#"] = countSchema.finalizeDiff( + &terraform.ResourceAttrDiff{ + Old: oldStr, + New: newStr, + }, + customized, + ) } // Build the list of codes that will make up our set. This is the @@ -1139,7 +1150,7 @@ func (m schemaMap) diffString( all bool) error { var originalN interface{} var os, ns string - o, n, _, computed := d.diffChange(k) + o, n, _, computed, customized := d.diffChange(k) if schema.StateFunc != nil && n != nil { originalN = n n = schema.StateFunc(n) @@ -1176,13 +1187,16 @@ func (m schemaMap) diffString( return nil } - diff.Attributes[k] = schema.finalizeDiff(&terraform.ResourceAttrDiff{ - Old: os, - New: ns, - NewExtra: originalN, - NewRemoved: removed, - NewComputed: computed, - }) + diff.Attributes[k] = schema.finalizeDiff( + &terraform.ResourceAttrDiff{ + Old: os, + New: ns, + NewExtra: originalN, + NewRemoved: removed, + NewComputed: computed, + }, + customized, + ) return nil }