From 1f654f6ecffb235d6cd2230cb2c822e2657415ad Mon Sep 17 00:00:00 2001 From: Alex Ott Date: Sun, 3 Nov 2024 11:53:50 +0100 Subject: [PATCH 1/2] [Internal] Make `Read` after `Create`/`Update` configurable This PR adds the ability for a resource to specify that it may not need to call `Read` after `Create` and `Update` operations so we can avoid performing another API call(s). The resource may implement `CanSkipReadAfterCreateAndUpdate` function that can decide if the `Read` operation should be skipped. I decided to move common part from #4173 to make it easier to review --- common/resource.go | 29 ++++++++----- common/resource_test.go | 95 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 113 insertions(+), 11 deletions(-) diff --git a/common/resource.go b/common/resource.go index 4e357305d..fb8a09e5b 100644 --- a/common/resource.go +++ b/common/resource.go @@ -16,17 +16,18 @@ import ( // Resource aims to simplify things like error & deleted entities handling type Resource struct { - Create func(ctx context.Context, d *schema.ResourceData, c *DatabricksClient) error - Read func(ctx context.Context, d *schema.ResourceData, c *DatabricksClient) error - Update func(ctx context.Context, d *schema.ResourceData, c *DatabricksClient) error - Delete func(ctx context.Context, d *schema.ResourceData, c *DatabricksClient) error - CustomizeDiff func(ctx context.Context, d *schema.ResourceDiff) error - StateUpgraders []schema.StateUpgrader - Schema map[string]*schema.Schema - SchemaVersion int - Timeouts *schema.ResourceTimeout - DeprecationMessage string - Importer *schema.ResourceImporter + Create func(ctx context.Context, d *schema.ResourceData, c *DatabricksClient) error + Read func(ctx context.Context, d *schema.ResourceData, c *DatabricksClient) error + Update func(ctx context.Context, d *schema.ResourceData, c *DatabricksClient) error + Delete func(ctx context.Context, d *schema.ResourceData, c *DatabricksClient) error + CustomizeDiff func(ctx context.Context, d *schema.ResourceDiff) error + StateUpgraders []schema.StateUpgrader + Schema map[string]*schema.Schema + SchemaVersion int + Timeouts *schema.ResourceTimeout + DeprecationMessage string + Importer *schema.ResourceImporter + CanSkipReadAfterCreateAndUpdate func(d *schema.ResourceData) bool } func nicerError(ctx context.Context, err error, action string) error { @@ -94,6 +95,9 @@ func (r Resource) ToResource() *schema.Resource { err = nicerError(ctx, err, "update") return diag.FromErr(err) } + if r.CanSkipReadAfterCreateAndUpdate != nil && r.CanSkipReadAfterCreateAndUpdate(d) { + return nil + } if err := recoverable(r.Read)(ctx, d, c); err != nil { err = nicerError(ctx, err, "read") return diag.FromErr(err) @@ -162,6 +166,9 @@ func (r Resource) ToResource() *schema.Resource { err = nicerError(ctx, err, "create") return diag.FromErr(err) } + if r.CanSkipReadAfterCreateAndUpdate != nil && r.CanSkipReadAfterCreateAndUpdate(d) { + return nil + } if err = recoverable(r.Read)(ctx, d, c); err != nil { err = nicerError(ctx, err, "read") return diag.FromErr(err) diff --git a/common/resource_test.go b/common/resource_test.go index f01f373ff..a7349c388 100644 --- a/common/resource_test.go +++ b/common/resource_test.go @@ -3,6 +3,7 @@ package common import ( "context" "fmt" + "log" "testing" "github.com/databricks/databricks-sdk-go/apierr" @@ -38,6 +39,100 @@ func TestImportingCallsRead(t *testing.T) { assert.Equal(t, 1, d.Get("foo")) } +func TestCreateSkipRead(t *testing.T) { + res := Resource{ + Create: func(ctx context.Context, + d *schema.ResourceData, + c *DatabricksClient) error { + log.Println("[DEBUG] Create called") + return d.Set("foo", 1) + }, + Read: func(ctx context.Context, + d *schema.ResourceData, + c *DatabricksClient) error { + log.Println("[DEBUG] Read called") + d.Set("foo", 2) + return nil + }, + Schema: map[string]*schema.Schema{ + "foo": { + Type: schema.TypeInt, + Required: true, + }, + }, + CanSkipReadAfterCreateAndUpdate: func(d *schema.ResourceData) bool { + return true + }, + } + + client := &DatabricksClient{} + ctx := context.Background() + // Test with skipping read + r := res.ToResource() + d := r.TestResourceData() + diags := r.CreateContext(ctx, d, client) + assert.False(t, diags.HasError()) + assert.Equal(t, 1, d.Get("foo")) + // Test without skipping read + res.CanSkipReadAfterCreateAndUpdate = nil + r = res.ToResource() + d = r.TestResourceData() + diags = r.CreateContext(ctx, d, client) + assert.False(t, diags.HasError()) + assert.Equal(t, 2, d.Get("foo")) +} + +func TestUpdateSkipRead(t *testing.T) { + res := Resource{ + Update: func(ctx context.Context, + d *schema.ResourceData, + c *DatabricksClient) error { + log.Println("[DEBUG] Update called") + return d.Set("foo", 1) + }, + Read: func(ctx context.Context, + d *schema.ResourceData, + c *DatabricksClient) error { + log.Println("[DEBUG] Read called") + d.Set("foo", 2) + return nil + }, + Schema: map[string]*schema.Schema{ + "foo": { + Type: schema.TypeInt, + Required: true, + }, + }, + CanSkipReadAfterCreateAndUpdate: func(d *schema.ResourceData) bool { + return true + }, + } + + client := &DatabricksClient{} + ctx := context.Background() + // Test with skipping read + r := res.ToResource() + d := r.TestResourceData() + datas, err := r.Importer.StateContext(ctx, d, client) + require.NoError(t, err) + assert.Len(t, datas, 1) + assert.False(t, r.Schema["foo"].ForceNew) + assert.Equal(t, "", d.Id()) + + diags := r.UpdateContext(ctx, d, client) + assert.False(t, diags.HasError()) + assert.Equal(t, 1, d.Get("foo")) + // Test without skipping read + res.CanSkipReadAfterCreateAndUpdate = nil + r = res.ToResource() + d = r.TestResourceData() + _, err = r.Importer.StateContext(ctx, d, client) + require.NoError(t, err) + diags = r.UpdateContext(ctx, d, client) + assert.False(t, diags.HasError()) + assert.Equal(t, 2, d.Get("foo")) +} + func TestHTTP404TriggersResourceRemovalForReadAndDelete(t *testing.T) { nope := func(ctx context.Context, d *schema.ResourceData, From 3c0a6c74c7851f8e0d391e0205d68f436d4d1493 Mon Sep 17 00:00:00 2001 From: Alex Ott Date: Mon, 4 Nov 2024 12:56:16 +0100 Subject: [PATCH 2/2] Split test cases --- common/resource_test.go | 82 +++++++++++++++++++---------------------- 1 file changed, 38 insertions(+), 44 deletions(-) diff --git a/common/resource_test.go b/common/resource_test.go index a7349c388..2ece50d28 100644 --- a/common/resource_test.go +++ b/common/resource_test.go @@ -39,7 +39,7 @@ func TestImportingCallsRead(t *testing.T) { assert.Equal(t, 1, d.Get("foo")) } -func TestCreateSkipRead(t *testing.T) { +func createTestResourceForSkipRead(skipRead bool) Resource { res := Resource{ Create: func(ctx context.Context, d *schema.ResourceData, @@ -54,64 +54,51 @@ func TestCreateSkipRead(t *testing.T) { d.Set("foo", 2) return nil }, + Update: func(ctx context.Context, + d *schema.ResourceData, + c *DatabricksClient) error { + log.Println("[DEBUG] Update called") + return d.Set("foo", 3) + }, Schema: map[string]*schema.Schema{ "foo": { Type: schema.TypeInt, Required: true, }, }, - CanSkipReadAfterCreateAndUpdate: func(d *schema.ResourceData) bool { + } + if skipRead { + res.CanSkipReadAfterCreateAndUpdate = func(d *schema.ResourceData) bool { return true - }, + } } + return res +} +func TestCreateSkipRead(t *testing.T) { client := &DatabricksClient{} ctx := context.Background() - // Test with skipping read - r := res.ToResource() + r := createTestResourceForSkipRead(true).ToResource() d := r.TestResourceData() diags := r.CreateContext(ctx, d, client) assert.False(t, diags.HasError()) assert.Equal(t, 1, d.Get("foo")) - // Test without skipping read - res.CanSkipReadAfterCreateAndUpdate = nil - r = res.ToResource() - d = r.TestResourceData() - diags = r.CreateContext(ctx, d, client) +} + +func TestCreateDontSkipRead(t *testing.T) { + client := &DatabricksClient{} + ctx := context.Background() + r := createTestResourceForSkipRead(false).ToResource() + d := r.TestResourceData() + diags := r.CreateContext(ctx, d, client) assert.False(t, diags.HasError()) assert.Equal(t, 2, d.Get("foo")) } func TestUpdateSkipRead(t *testing.T) { - res := Resource{ - Update: func(ctx context.Context, - d *schema.ResourceData, - c *DatabricksClient) error { - log.Println("[DEBUG] Update called") - return d.Set("foo", 1) - }, - Read: func(ctx context.Context, - d *schema.ResourceData, - c *DatabricksClient) error { - log.Println("[DEBUG] Read called") - d.Set("foo", 2) - return nil - }, - Schema: map[string]*schema.Schema{ - "foo": { - Type: schema.TypeInt, - Required: true, - }, - }, - CanSkipReadAfterCreateAndUpdate: func(d *schema.ResourceData) bool { - return true - }, - } - client := &DatabricksClient{} ctx := context.Background() - // Test with skipping read - r := res.ToResource() + r := createTestResourceForSkipRead(true).ToResource() d := r.TestResourceData() datas, err := r.Importer.StateContext(ctx, d, client) require.NoError(t, err) @@ -121,14 +108,21 @@ func TestUpdateSkipRead(t *testing.T) { diags := r.UpdateContext(ctx, d, client) assert.False(t, diags.HasError()) - assert.Equal(t, 1, d.Get("foo")) - // Test without skipping read - res.CanSkipReadAfterCreateAndUpdate = nil - r = res.ToResource() - d = r.TestResourceData() - _, err = r.Importer.StateContext(ctx, d, client) + assert.Equal(t, 3, d.Get("foo")) +} + +func TestUpdateDontSkipRead(t *testing.T) { + client := &DatabricksClient{} + ctx := context.Background() + r := createTestResourceForSkipRead(false).ToResource() + d := r.TestResourceData() + datas, err := r.Importer.StateContext(ctx, d, client) require.NoError(t, err) - diags = r.UpdateContext(ctx, d, client) + assert.Len(t, datas, 1) + assert.False(t, r.Schema["foo"].ForceNew) + assert.Equal(t, "", d.Id()) + + diags := r.UpdateContext(ctx, d, client) assert.False(t, diags.HasError()) assert.Equal(t, 2, d.Get("foo")) }