From 46b4c27dbec50102977e002f479c01b55ae67469 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Mon, 15 Oct 2018 19:29:37 -0400 Subject: [PATCH] create a SimpleDiff for the new provider shims Terraform now handles any actual "diffing" of resource, and the existing Diff functions are only used to shim the schema.Provider to the new methods. Since terraform is handling what used to be the Diff, the provider now should not modify the diff based on RequiresNew due to it interfering with the ignore_changes handling. --- helper/schema/backend.go | 4 +- helper/schema/provider.go | 18 ++++- helper/schema/provisioner.go | 4 +- helper/schema/resource.go | 30 +++++++- helper/schema/schema.go | 140 ++++++++++++++++++----------------- helper/schema/schema_test.go | 4 +- helper/schema/shims.go | 6 +- helper/schema/shims_test.go | 36 +++------ helper/schema/testing.go | 2 +- 9 files changed, 138 insertions(+), 106 deletions(-) diff --git a/helper/schema/backend.go b/helper/schema/backend.go index 6785c9f335..eef6bee3d7 100644 --- a/helper/schema/backend.go +++ b/helper/schema/backend.go @@ -6,8 +6,8 @@ import ( "github.com/hashicorp/terraform/tfdiags" "github.com/zclconf/go-cty/cty" - "github.com/hashicorp/terraform/configs/configschema" "github.com/hashicorp/terraform/config/hcl2shim" + "github.com/hashicorp/terraform/configs/configschema" "github.com/hashicorp/terraform/terraform" ) @@ -77,7 +77,7 @@ func (b *Backend) Configure(obj cty.Value) tfdiags.Diagnostics { // Get a ResourceData for this configuration. To do this, we actually // generate an intermediary "diff" although that is never exposed. - diff, err := sm.Diff(nil, shimRC, nil, nil) + diff, err := sm.Diff(nil, shimRC, nil, nil, true) if err != nil { diags = diags.Append(err) return diags diff --git a/helper/schema/provider.go b/helper/schema/provider.go index 4355fa4c2a..7d7503d6dc 100644 --- a/helper/schema/provider.go +++ b/helper/schema/provider.go @@ -251,7 +251,7 @@ func (p *Provider) Configure(c *terraform.ResourceConfig) error { // Get a ResourceData for this configuration. To do this, we actually // generate an intermediary "diff" although that is never exposed. - diff, err := sm.Diff(nil, c, nil, p.meta) + diff, err := sm.Diff(nil, c, nil, p.meta, true) if err != nil { return err } @@ -296,6 +296,20 @@ func (p *Provider) Diff( return r.Diff(s, c, p.meta) } +// SimpleDiff is used by the new protocol wrappers to get a diff that doesn't +// attempt to calculate ignore_changes. +func (p *Provider) SimpleDiff( + info *terraform.InstanceInfo, + s *terraform.InstanceState, + c *terraform.ResourceConfig) (*terraform.InstanceDiff, error) { + r, ok := p.ResourcesMap[info.Type] + if !ok { + return nil, fmt.Errorf("unknown resource type: %s", info.Type) + } + + return r.simpleDiff(s, c, p.meta) +} + // Refresh implementation of terraform.ResourceProvider interface. func (p *Provider) Refresh( info *terraform.InstanceInfo, @@ -311,7 +325,7 @@ func (p *Provider) Refresh( // Resources implementation of terraform.ResourceProvider interface. func (p *Provider) Resources() []terraform.ResourceType { keys := make([]string, 0, len(p.ResourcesMap)) - for k, _ := range p.ResourcesMap { + for k := range p.ResourcesMap { keys = append(keys, k) } sort.Strings(keys) diff --git a/helper/schema/provisioner.go b/helper/schema/provisioner.go index 2def998b49..637e221e1d 100644 --- a/helper/schema/provisioner.go +++ b/helper/schema/provisioner.go @@ -152,7 +152,7 @@ func (p *Provisioner) Apply( } sm := schemaMap(p.ConnSchema) - diff, err := sm.Diff(nil, terraform.NewResourceConfig(c), nil, nil) + diff, err := sm.Diff(nil, terraform.NewResourceConfig(c), nil, nil, true) if err != nil { return err } @@ -166,7 +166,7 @@ func (p *Provisioner) Apply( // Build the configuration data. Doing this requires making a "diff" // even though that's never used. We use that just to get the correct types. configMap := schemaMap(p.Schema) - diff, err := configMap.Diff(nil, c, nil, nil) + diff, err := configMap.Diff(nil, c, nil, nil, true) if err != nil { return err } diff --git a/helper/schema/resource.go b/helper/schema/resource.go index 9b45af4273..1ed58bfee7 100644 --- a/helper/schema/resource.go +++ b/helper/schema/resource.go @@ -287,7 +287,35 @@ func (r *Resource) Diff( return nil, fmt.Errorf("[ERR] Error decoding timeout: %s", err) } - instanceDiff, err := schemaMap(r.Schema).Diff(s, c, r.CustomizeDiff, meta) + instanceDiff, err := schemaMap(r.Schema).Diff(s, c, r.CustomizeDiff, meta, true) + if err != nil { + return instanceDiff, err + } + + if instanceDiff != nil { + if err := t.DiffEncode(instanceDiff); err != nil { + log.Printf("[ERR] Error encoding timeout to instance diff: %s", err) + } + } else { + log.Printf("[DEBUG] Instance Diff is nil in Diff()") + } + + return instanceDiff, err +} + +func (r *Resource) simpleDiff( + s *terraform.InstanceState, + c *terraform.ResourceConfig, + meta interface{}) (*terraform.InstanceDiff, error) { + + t := &ResourceTimeout{} + err := t.ConfigDecode(r, c) + + if err != nil { + return nil, fmt.Errorf("[ERR] Error decoding timeout: %s", err) + } + + instanceDiff, err := schemaMap(r.Schema).Diff(s, c, r.CustomizeDiff, meta, false) if err != nil { return instanceDiff, err } diff --git a/helper/schema/schema.go b/helper/schema/schema.go index ad1b67f4f2..202310a851 100644 --- a/helper/schema/schema.go +++ b/helper/schema/schema.go @@ -410,7 +410,8 @@ func (m schemaMap) Diff( s *terraform.InstanceState, c *terraform.ResourceConfig, customizeDiff CustomizeDiffFunc, - meta interface{}) (*terraform.InstanceDiff, error) { + meta interface{}, + handleRequiresNew bool) (*terraform.InstanceDiff, error) { result := new(terraform.InstanceDiff) result.Attributes = make(map[string]*terraform.ResourceAttrDiff) @@ -456,82 +457,85 @@ func (m schemaMap) Diff( } } - // If the diff requires a new resource, then we recompute the diff - // so we have the complete new resource diff, and preserve the - // RequiresNew fields where necessary so the user knows exactly what - // caused that. - if result.RequiresNew() { - // Create the new diff - result2 := new(terraform.InstanceDiff) - result2.Attributes = make(map[string]*terraform.ResourceAttrDiff) + if handleRequiresNew { + // If the diff requires a new resource, then we recompute the diff + // so we have the complete new resource diff, and preserve the + // RequiresNew fields where necessary so the user knows exactly what + // caused that. + if result.RequiresNew() { + // Create the new diff + result2 := new(terraform.InstanceDiff) + result2.Attributes = make(map[string]*terraform.ResourceAttrDiff) - // Preserve the DestroyTainted flag - result2.DestroyTainted = result.DestroyTainted + // Preserve the DestroyTainted flag + result2.DestroyTainted = result.DestroyTainted - // Reset the data to not contain state. We have to call init() - // again in order to reset the FieldReaders. - d.state = nil - d.init() + // Reset the data to not contain state. We have to call init() + // again in order to reset the FieldReaders. + d.state = nil + d.init() - // Perform the diff again - for k, schema := range m { - err := m.diff(k, schema, result2, d, false) - if err != nil { - return nil, err - } - } - - // Re-run customization - if !result2.DestroyTainted && customizeDiff != nil { - mc := m.DeepCopy() - rd := newResourceDiff(mc, c, d.state, result2) - if err := customizeDiff(rd, meta); err != nil { - return nil, err - } - for _, k := range rd.UpdatedKeys() { - err := m.diff(k, mc[k], result2, rd, false) + // Perform the diff again + for k, schema := range m { + err := m.diff(k, schema, result2, d, false) if err != nil { return nil, err } } + + // Re-run customization + if !result2.DestroyTainted && customizeDiff != nil { + mc := m.DeepCopy() + rd := newResourceDiff(mc, c, d.state, result2) + if err := customizeDiff(rd, meta); err != nil { + return nil, err + } + for _, k := range rd.UpdatedKeys() { + err := m.diff(k, mc[k], result2, rd, false) + if err != nil { + return nil, err + } + } + } + + // Force all the fields to not force a new since we know what we + // want to force new. + for k, attr := range result2.Attributes { + if attr == nil { + continue + } + + if attr.RequiresNew { + attr.RequiresNew = false + } + + if s != nil { + attr.Old = s.Attributes[k] + } + } + + // Now copy in all the requires new diffs... + for k, attr := range result.Attributes { + if attr == nil { + continue + } + + newAttr, ok := result2.Attributes[k] + if !ok { + newAttr = attr + } + + if attr.RequiresNew { + newAttr.RequiresNew = true + } + + result2.Attributes[k] = newAttr + } + + // And set the diff! + result = result2 } - // Force all the fields to not force a new since we know what we - // want to force new. - for k, attr := range result2.Attributes { - if attr == nil { - continue - } - - if attr.RequiresNew { - attr.RequiresNew = false - } - - if s != nil { - attr.Old = s.Attributes[k] - } - } - - // Now copy in all the requires new diffs... - for k, attr := range result.Attributes { - if attr == nil { - continue - } - - newAttr, ok := result2.Attributes[k] - if !ok { - newAttr = attr - } - - if attr.RequiresNew { - newAttr.RequiresNew = true - } - - result2.Attributes[k] = newAttr - } - - // And set the diff! - result = result2 } // Go through and detect all of the ComputedWhens now that we've diff --git a/helper/schema/schema_test.go b/helper/schema/schema_test.go index 6863482a48..9c505255c2 100644 --- a/helper/schema/schema_test.go +++ b/helper/schema/schema_test.go @@ -3246,7 +3246,7 @@ func TestSchemaMap_Diff(t *testing.T) { } } - d, err := schemaMap(tc.Schema).Diff(tc.State, terraform.NewResourceConfig(c), tc.CustomizeDiff, nil) + d, err := schemaMap(tc.Schema).Diff(tc.State, terraform.NewResourceConfig(c), tc.CustomizeDiff, nil, true) if err != nil != tc.Err { t.Fatalf("err: %s", err) } @@ -4096,7 +4096,7 @@ func TestSchemaMap_DiffSuppress(t *testing.T) { } } - d, err := schemaMap(tc.Schema).Diff(tc.State, terraform.NewResourceConfig(c), nil, nil) + d, err := schemaMap(tc.Schema).Diff(tc.State, terraform.NewResourceConfig(c), nil, nil, true) if err != nil != tc.Err { t.Fatalf("#%q err: %s", tn, err) } diff --git a/helper/schema/shims.go b/helper/schema/shims.go index 8122c2646e..52ae7d7446 100644 --- a/helper/schema/shims.go +++ b/helper/schema/shims.go @@ -29,7 +29,7 @@ func diffFromValues(prior, planned cty.Value, res *Resource, cust CustomizeDiffF cfg := terraform.NewResourceConfigShimmed(planned, configSchema) - diff, err := schemaMap(res.Schema).Diff(instanceState, cfg, cust, nil) + diff, err := schemaMap(res.Schema).Diff(instanceState, cfg, cust, nil, false) if err != nil { return nil, err } @@ -41,8 +41,8 @@ func diffFromValues(prior, planned cty.Value, res *Resource, cust CustomizeDiffF // get a new cty.Value state. This is used to convert the diff returned from // the legacy provider Diff method to the state required for the new // PlanResourceChange method. -func ApplyDiff(state cty.Value, d *terraform.InstanceDiff, schemaBlock *configschema.Block) (cty.Value, error) { - return d.ApplyToValue(state, schemaBlock) +func ApplyDiff(base cty.Value, d *terraform.InstanceDiff, schema *configschema.Block) (cty.Value, error) { + return d.ApplyToValue(base, schema) } // StateValueToJSONMap converts a cty.Value to generic JSON map via the cty JSON diff --git a/helper/schema/shims_test.go b/helper/schema/shims_test.go index 2045b63dfb..61919f3cc0 100644 --- a/helper/schema/shims_test.go +++ b/helper/schema/shims_test.go @@ -2024,11 +2024,11 @@ func TestShimSchemaMap_Diff(t *testing.T) { RequiresNew: true, }, - "address": &terraform.ResourceAttrDiff{ - Old: "foo", - New: "", - NewComputed: true, - }, + // "address": &terraform.ResourceAttrDiff{ + // Old: "foo", + // New: "", + // NewComputed: true, + // }, }, }, @@ -2075,11 +2075,11 @@ func TestShimSchemaMap_Diff(t *testing.T) { RequiresNew: true, }, - "ports.#": &terraform.ResourceAttrDiff{ - Old: "1", - New: "", - NewComputed: true, - }, + // "ports.#": &terraform.ResourceAttrDiff{ + // Old: "1", + // New: "", + // NewComputed: true, + // }, }, }, @@ -2849,10 +2849,6 @@ func TestShimSchemaMap_Diff(t *testing.T) { Diff: &terraform.InstanceDiff{ Attributes: map[string]*terraform.ResourceAttrDiff{ - "instances.#": &terraform.ResourceAttrDiff{ - Old: "2", - New: "2", - }, "instances.2": &terraform.ResourceAttrDiff{ Old: "22", New: "", @@ -2973,10 +2969,6 @@ func TestShimSchemaMap_Diff(t *testing.T) { Diff: &terraform.InstanceDiff{ Attributes: map[string]*terraform.ResourceAttrDiff{ - "ports.#": &terraform.ResourceAttrDiff{ - Old: "3", - New: "3", - }, "ports.1": &terraform.ResourceAttrDiff{ Old: "1", New: "1", @@ -3103,8 +3095,6 @@ func TestShimSchemaMap_Diff(t *testing.T) { Diff: &terraform.InstanceDiff{ Attributes: map[string]*terraform.ResourceAttrDiff{ "ports.#": &terraform.ResourceAttrDiff{ - Old: "3", - New: "", NewComputed: true, RequiresNew: true, }, @@ -3357,10 +3347,6 @@ func TestShimSchemaMap_Diff(t *testing.T) { Diff: &terraform.InstanceDiff{ Attributes: map[string]*terraform.ResourceAttrDiff{ - "ports.#": &terraform.ResourceAttrDiff{ - Old: "3", - New: "3", - }, "ports.1": &terraform.ResourceAttrDiff{ Old: "1", New: "1", @@ -3576,7 +3562,7 @@ func TestShimSchemaMap_Diff(t *testing.T) { } { - d, err := InternalMap(tc.Schema).Diff(tc.State, terraform.NewResourceConfig(c), tc.CustomizeDiff, nil) + d, err := InternalMap(tc.Schema).Diff(tc.State, terraform.NewResourceConfig(c), tc.CustomizeDiff, nil, false) if err != nil != tc.Err { t.Fatalf("err: %s", err) } diff --git a/helper/schema/testing.go b/helper/schema/testing.go index da754ac788..a367a1fd59 100644 --- a/helper/schema/testing.go +++ b/helper/schema/testing.go @@ -18,7 +18,7 @@ func TestResourceDataRaw( } sm := schemaMap(schema) - diff, err := sm.Diff(nil, terraform.NewResourceConfig(c), nil, nil) + diff, err := sm.Diff(nil, terraform.NewResourceConfig(c), nil, nil, true) if err != nil { t.Fatalf("err: %s", err) }