From 8af9610b87f86cc59ef0c4ece1588aae307661f3 Mon Sep 17 00:00:00 2001 From: Chris Marchesi Date: Sat, 27 May 2017 22:58:44 -0700 Subject: [PATCH] helper/schema: Hook CustomizeDiffFunc into diff logic It's alive! CustomizeDiff logic now has been inserted into the diff process. The test_resource_with_custom_diff resource provides some basic testing and a reference implementation. There should now be plenty of test coverage for this feature via the tests added for ResourceDiff, and the basic test added to the schemaMap.Diff test, and the test resource, but more can be added to test any specific case that comes up otherwise. --- builtin/providers/test/provider.go | 5 +- .../test/resource_with_custom_diff.go | 84 +++++++++++++++++++ .../test/resource_with_custom_diff_test.go | 47 +++++++++++ helper/schema/backend.go | 2 +- helper/schema/provider.go | 6 +- helper/schema/provisioner.go | 4 +- helper/schema/resource.go | 5 +- helper/schema/resource_diff.go | 4 +- helper/schema/resource_test.go | 2 +- helper/schema/schema.go | 35 +++++++- helper/schema/schema_test.go | 44 +++++++++- helper/schema/testing.go | 2 +- 12 files changed, 221 insertions(+), 19 deletions(-) create mode 100644 builtin/providers/test/resource_with_custom_diff.go create mode 100644 builtin/providers/test/resource_with_custom_diff_test.go diff --git a/builtin/providers/test/provider.go b/builtin/providers/test/provider.go index 4afec61b0..6c71bfb18 100644 --- a/builtin/providers/test/provider.go +++ b/builtin/providers/test/provider.go @@ -17,8 +17,9 @@ func Provider() terraform.ResourceProvider { }, }, ResourcesMap: map[string]*schema.Resource{ - "test_resource": testResource(), - "test_resource_gh12183": testResourceGH12183(), + "test_resource": testResource(), + "test_resource_gh12183": testResourceGH12183(), + "test_resource_with_custom_diff": testResourceCustomDiff(), }, DataSourcesMap: map[string]*schema.Resource{ "test_data_source": testDataSource(), diff --git a/builtin/providers/test/resource_with_custom_diff.go b/builtin/providers/test/resource_with_custom_diff.go new file mode 100644 index 000000000..20d3f0ef9 --- /dev/null +++ b/builtin/providers/test/resource_with_custom_diff.go @@ -0,0 +1,84 @@ +package test + +import ( + "fmt" + + "github.com/hashicorp/terraform/helper/schema" +) + +func testResourceCustomDiff() *schema.Resource { + return &schema.Resource{ + Create: testResourceCustomDiffCreate, + Read: testResourceCustomDiffRead, + CustomizeDiff: testResourceCustomDiffCustomizeDiff, + Update: testResourceCustomDiffUpdate, + Delete: testResourceCustomDiffDelete, + Schema: map[string]*schema.Schema{ + "required": { + Type: schema.TypeString, + Required: true, + }, + "computed": { + Type: schema.TypeInt, + Computed: true, + }, + "index": { + Type: schema.TypeInt, + Computed: true, + }, + "veto": { + Type: schema.TypeBool, + Optional: true, + }, + }, + } +} + +func testResourceCustomDiffCreate(d *schema.ResourceData, meta interface{}) error { + d.SetId("testId") + + // Required must make it through to Create + if _, ok := d.GetOk("required"); !ok { + return fmt.Errorf("missing attribute 'required', but it's required") + } + + _, new := d.GetChange("computed") + expected := new.(int) - 1 + actual := d.Get("index").(int) + if expected != actual { + return fmt.Errorf("expected computed to be 1 ahead of index, got computed: %d, index: %d", expected, actual) + } + d.Set("index", new) + + return testResourceCustomDiffRead(d, meta) +} + +func testResourceCustomDiffRead(d *schema.ResourceData, meta interface{}) error { + return nil +} + +func testResourceCustomDiffCustomizeDiff(d *schema.ResourceDiff, meta interface{}) error { + if d.Get("veto").(bool) == true { + return fmt.Errorf("veto is true, diff vetoed") + } + // Note that this gets put into state after the update, regardless of whether + // or not anything is acted upon in the diff. + d.SetNew("computed", d.Get("computed").(int)+1) + return nil +} + +func testResourceCustomDiffUpdate(d *schema.ResourceData, meta interface{}) error { + _, new := d.GetChange("computed") + expected := new.(int) - 1 + actual := d.Get("index").(int) + if expected != actual { + return fmt.Errorf("expected computed to be 1 ahead of index, got computed: %d, index: %d", expected, actual) + } + d.Set("index", new) + return nil +} + +func testResourceCustomDiffDelete(d *schema.ResourceData, meta interface{}) error { + d.SetId("") + return nil +} diff --git a/builtin/providers/test/resource_with_custom_diff_test.go b/builtin/providers/test/resource_with_custom_diff_test.go new file mode 100644 index 000000000..d9afbb33f --- /dev/null +++ b/builtin/providers/test/resource_with_custom_diff_test.go @@ -0,0 +1,47 @@ +package test + +import ( + "fmt" + "regexp" + "testing" + + "github.com/hashicorp/terraform/helper/resource" +) + +// TestResourceWithCustomDiff test custom diff behaviour. +func TestResourceWithCustomDiff(t *testing.T) { + resource.UnitTest(t, resource.TestCase{ + Providers: testAccProviders, + Steps: []resource.TestStep{ + { + Config: resourceWithCustomDiffConfig(false), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr("test_resource_with_custom_diff.foo", "computed", "1"), + resource.TestCheckResourceAttr("test_resource_with_custom_diff.foo", "index", "1"), + ), + ExpectNonEmptyPlan: true, + }, + { + Config: resourceWithCustomDiffConfig(false), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr("test_resource_with_custom_diff.foo", "computed", "2"), + resource.TestCheckResourceAttr("test_resource_with_custom_diff.foo", "index", "2"), + ), + ExpectNonEmptyPlan: true, + }, + { + Config: resourceWithCustomDiffConfig(true), + ExpectError: regexp.MustCompile("veto is true, diff vetoed"), + }, + }, + }) +} + +func resourceWithCustomDiffConfig(veto bool) string { + return fmt.Sprintf(` +resource "test_resource_with_custom_diff" "foo" { + required = "yep" + veto = %t +} +`, veto) +} diff --git a/helper/schema/backend.go b/helper/schema/backend.go index a0729c02c..57fbba744 100644 --- a/helper/schema/backend.go +++ b/helper/schema/backend.go @@ -65,7 +65,7 @@ func (b *Backend) 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) + diff, err := sm.Diff(nil, c, nil, nil) if err != nil { return err } diff --git a/helper/schema/provider.go b/helper/schema/provider.go index 30b6b9e3d..0ade98bea 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) + diff, err := sm.Diff(nil, c, nil, p.meta) if err != nil { return err } @@ -293,7 +293,7 @@ func (p *Provider) Diff( return nil, fmt.Errorf("unknown resource type: %s", info.Type) } - return r.Diff(s, c) + return r.Diff(s, c, p.meta) } // Refresh implementation of terraform.ResourceProvider interface. @@ -410,7 +410,7 @@ func (p *Provider) ReadDataDiff( return nil, fmt.Errorf("unknown data source: %s", info.Type) } - return r.Diff(nil, c) + return r.Diff(nil, c, p.meta) } // RefreshData implementation of terraform.ResourceProvider interface. diff --git a/helper/schema/provisioner.go b/helper/schema/provisioner.go index 476192e9d..a8d42dbd0 100644 --- a/helper/schema/provisioner.go +++ b/helper/schema/provisioner.go @@ -146,7 +146,7 @@ func (p *Provisioner) Apply( } sm := schemaMap(p.ConnSchema) - diff, err := sm.Diff(nil, terraform.NewResourceConfig(c)) + diff, err := sm.Diff(nil, terraform.NewResourceConfig(c), nil, nil) if err != nil { return err } @@ -160,7 +160,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) + diff, err := configMap.Diff(nil, c, nil, nil) if err != nil { return err } diff --git a/helper/schema/resource.go b/helper/schema/resource.go index 5c2a4e0d7..5d8bc3941 100644 --- a/helper/schema/resource.go +++ b/helper/schema/resource.go @@ -219,7 +219,8 @@ func (r *Resource) Apply( // ResourceProvider interface. func (r *Resource) Diff( s *terraform.InstanceState, - c *terraform.ResourceConfig) (*terraform.InstanceDiff, error) { + c *terraform.ResourceConfig, + meta interface{}) (*terraform.InstanceDiff, error) { t := &ResourceTimeout{} err := t.ConfigDecode(r, c) @@ -228,7 +229,7 @@ func (r *Resource) Diff( return nil, fmt.Errorf("[ERR] Error decoding timeout: %s", err) } - instanceDiff, err := schemaMap(r.Schema).Diff(s, c) + instanceDiff, err := schemaMap(r.Schema).Diff(s, c, r.CustomizeDiff, meta) if err != nil { return instanceDiff, err } diff --git a/helper/schema/resource_diff.go b/helper/schema/resource_diff.go index cfc36f271..df9c1275d 100644 --- a/helper/schema/resource_diff.go +++ b/helper/schema/resource_diff.go @@ -259,7 +259,7 @@ func (d *ResourceDiff) diffChange(key string) (interface{}, interface{}, bool, b // doing so will taint any existing diff for this key and will remove it from // the catalog. func (d *ResourceDiff) SetNew(key string, value interface{}) error { - return d.SetDiff(key, d.getExact(strings.Split(key, "."), "state").Value, value, false) + return d.SetDiff(key, d.get(strings.Split(key, "."), "state").Value, value, false) } // SetNewComputed functions like SetNew, except that it sets the new value to @@ -267,7 +267,7 @@ func (d *ResourceDiff) SetNew(key string, value interface{}) error { // // This function is only allowed on computed keys. func (d *ResourceDiff) SetNewComputed(key string) error { - return d.SetDiff(key, d.getExact(strings.Split(key, "."), "state").Value, d.schema[key].ZeroValue(), true) + return d.SetDiff(key, d.get(strings.Split(key, "."), "state").Value, d.schema[key].ZeroValue(), true) } // SetDiff allows the setting of both old and new values for the diff diff --git a/helper/schema/resource_test.go b/helper/schema/resource_test.go index b9332ccc6..ab17d6025 100644 --- a/helper/schema/resource_test.go +++ b/helper/schema/resource_test.go @@ -226,7 +226,7 @@ func TestResourceDiff_Timeout_diff(t *testing.T) { var s *terraform.InstanceState = nil conf := terraform.NewResourceConfig(raw) - actual, err := r.Diff(s, conf) + actual, err := r.Diff(s, conf, nil) if err != nil { t.Fatalf("err: %s", err) } diff --git a/helper/schema/schema.go b/helper/schema/schema.go index 95691072a..998a104ca 100644 --- a/helper/schema/schema.go +++ b/helper/schema/schema.go @@ -385,7 +385,9 @@ func (m *schemaMap) DeepCopy() schemaMap { // state, and configuration. func (m schemaMap) Diff( s *terraform.InstanceState, - c *terraform.ResourceConfig) (*terraform.InstanceDiff, error) { + c *terraform.ResourceConfig, + customizeFunc CustomizeDiffFunc, + meta interface{}) (*terraform.InstanceDiff, error) { result := new(terraform.InstanceDiff) result.Attributes = make(map[string]*terraform.ResourceAttrDiff) @@ -407,6 +409,22 @@ func (m schemaMap) Diff( } } + // If this is a non-destroy diff, call any custom diff logic that has been + // defined. + if !result.Destroy && !result.DestroyTainted && customizeFunc != nil { + mc := m.DeepCopy() + rd := newResourceDiff(mc, c, s, result) + if err := customizeFunc(rd, meta); err != nil { + return nil, err + } + for _, k := range rd.UpdatedKeys() { + err := m.diff(k, mc[k], result, rd, false) + if err != nil { + return nil, err + } + } + } + // 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 @@ -432,6 +450,21 @@ func (m schemaMap) Diff( } } + // Re-run customization + if customizeFunc != nil { + mc := m.DeepCopy() + rd := newResourceDiff(mc, c, d.state, result2) + if err := customizeFunc(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 { diff --git a/helper/schema/schema_test.go b/helper/schema/schema_test.go index 5a2d8190b..089a370e5 100644 --- a/helper/schema/schema_test.go +++ b/helper/schema/schema_test.go @@ -138,6 +138,7 @@ func TestSchemaMap_Diff(t *testing.T) { State *terraform.InstanceState Config map[string]interface{} ConfigVariables map[string]ast.Variable + CustomizeDiff CustomizeDiffFunc Diff *terraform.InstanceDiff Err bool }{ @@ -2823,6 +2824,43 @@ func TestSchemaMap_Diff(t *testing.T) { Err: false, }, + + { + Name: "overridden diff with a CustomizeDiff function", + Schema: map[string]*Schema{ + "availability_zone": &Schema{ + Type: TypeString, + Optional: true, + Computed: true, + ForceNew: true, + }, + }, + + State: nil, + + Config: map[string]interface{}{ + "availability_zone": "foo", + }, + + CustomizeDiff: func(d *ResourceDiff, meta interface{}) error { + if err := d.SetNew("availability_zone", "bar"); err != nil { + return err + } + return nil + }, + + Diff: &terraform.InstanceDiff{ + Attributes: map[string]*terraform.ResourceAttrDiff{ + "availability_zone": &terraform.ResourceAttrDiff{ + Old: "", + New: "bar", + RequiresNew: true, + }, + }, + }, + + Err: false, + }, } for i, tc := range cases { @@ -2838,8 +2876,7 @@ func TestSchemaMap_Diff(t *testing.T) { } } - d, err := schemaMap(tc.Schema).Diff( - tc.State, terraform.NewResourceConfig(c)) + d, err := schemaMap(tc.Schema).Diff(tc.State, terraform.NewResourceConfig(c), tc.CustomizeDiff, nil) if err != nil != tc.Err { t.Fatalf("err: %s", err) } @@ -3689,8 +3726,7 @@ func TestSchemaMap_DiffSuppress(t *testing.T) { } } - d, err := schemaMap(tc.Schema).Diff( - tc.State, terraform.NewResourceConfig(c)) + d, err := schemaMap(tc.Schema).Diff(tc.State, terraform.NewResourceConfig(c), nil, nil) if err != nil != tc.Err { t.Fatalf("#%q err: %s", tn, err) } diff --git a/helper/schema/testing.go b/helper/schema/testing.go index 353c9366a..da754ac78 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)) + diff, err := sm.Diff(nil, terraform.NewResourceConfig(c), nil, nil) if err != nil { t.Fatalf("err: %s", err) }