From 2448d1d38b2e3c2fe5bb3fc5656038e0cfe50157 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 19 Jun 2019 22:41:23 -0400 Subject: [PATCH 1/3] move timeout handling to grpc_provider simpleDiff is only called from the grpc_provider, and we always need to make sure the timeouts are encoded in the private data. --- helper/schema/resource.go | 15 ++------------- 1 file changed, 2 insertions(+), 13 deletions(-) diff --git a/helper/schema/resource.go b/helper/schema/resource.go index b5e306574555..bcfe5666fcca 100644 --- a/helper/schema/resource.go +++ b/helper/schema/resource.go @@ -329,21 +329,13 @@ func (r *Resource) simpleDiff( 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 } if instanceDiff == nil { - log.Printf("[DEBUG] Instance Diff is nil in SimpleDiff()") - return nil, err + instanceDiff = terraform.NewInstanceDiff() } // Make sure the old value is set in each of the instance diffs. @@ -357,10 +349,7 @@ func (r *Resource) simpleDiff( } } - if err := t.DiffEncode(instanceDiff); err != nil { - log.Printf("[ERR] Error encoding timeout to instance diff: %s", err) - } - return instanceDiff, err + return instanceDiff, nil } // Validate validates the resource configuration against the schema. From c4874aa5b36ccc8445d12bb76b2633f8a92bb4e2 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 19 Jun 2019 22:43:40 -0400 Subject: [PATCH 2/3] add more timeout provider tests --- .../providers/test/resource_timeout_test.go | 46 +++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/builtin/providers/test/resource_timeout_test.go b/builtin/providers/test/resource_timeout_test.go index 8227bfb63945..312a37a781bd 100644 --- a/builtin/providers/test/resource_timeout_test.go +++ b/builtin/providers/test/resource_timeout_test.go @@ -28,6 +28,52 @@ resource "test_resource_timeout" "foo" { }) } +// start with the default, then modify it +func TestResourceTimeout_defaults(t *testing.T) { + resource.UnitTest(t, resource.TestCase{ + Providers: testAccProviders, + CheckDestroy: testAccCheckResourceDestroy, + Steps: []resource.TestStep{ + resource.TestStep{ + Config: strings.TrimSpace(` +resource "test_resource_timeout" "foo" { + update_delay = "1ms" +} + `), + }, + resource.TestStep{ + Config: strings.TrimSpace(` +resource "test_resource_timeout" "foo" { + update_delay = "2ms" + timeouts { + update = "3s" + } +} + `), + }, + resource.TestStep{ + Config: strings.TrimSpace(` +resource "test_resource_timeout" "foo" { + update_delay = "2s" + delete_delay = "2s" + timeouts { + delete = "3s" + update = "3s" + } +} + `), + }, + // delete "foo" + resource.TestStep{ + Config: strings.TrimSpace(` +resource "test_resource_timeout" "bar" { +} + `), + }, + }, + }) +} + func TestResourceTimeout_delete(t *testing.T) { // If the delete timeout isn't saved until destroy, the cleanup here will // fail because the default is only 20m. From 9365a2d97d2fa16045d05386d0b34f3a26980308 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 19 Jun 2019 22:44:38 -0400 Subject: [PATCH 3/3] private and timeout handling in grpc_provider Load private data for read, so the resource can get it's configured timeouts if they exist. Ensure PlanResourceChange returns the saved private data when there is an empty diff. Handle the timeout decoding into Meta in the PlanResourceChange, so that it's always there for later operations. --- helper/plugin/grpc_provider.go | 34 ++++++++++++++++++++++++++++------ 1 file changed, 28 insertions(+), 6 deletions(-) diff --git a/helper/plugin/grpc_provider.go b/helper/plugin/grpc_provider.go index db9d12cfbc5b..ace7e1389bdc 100644 --- a/helper/plugin/grpc_provider.go +++ b/helper/plugin/grpc_provider.go @@ -492,7 +492,12 @@ func (s *GRPCProviderServer) Configure(_ context.Context, req *proto.Configure_R } func (s *GRPCProviderServer) ReadResource(_ context.Context, req *proto.ReadResource_Request) (*proto.ReadResource_Response, error) { - resp := &proto.ReadResource_Response{} + resp := &proto.ReadResource_Response{ + // helper/schema did previously handle private data during refresh, but + // core is now going to expect this to be maintained in order to + // persist it in the state. + Private: req.Private, + } res := s.provider.ResourcesMap[req.TypeName] schemaBlock := s.getResourceSchemaBlock(req.TypeName) @@ -509,6 +514,15 @@ func (s *GRPCProviderServer) ReadResource(_ context.Context, req *proto.ReadReso return resp, nil } + private := make(map[string]interface{}) + if len(req.Private) > 0 { + if err := json.Unmarshal(req.Private, &private); err != nil { + resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, err) + return resp, nil + } + } + instanceState.Meta = private + newInstanceState, err := res.RefreshWithoutUpgrade(instanceState, s.provider.Meta()) if err != nil { resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, err) @@ -551,11 +565,6 @@ func (s *GRPCProviderServer) ReadResource(_ context.Context, req *proto.ReadReso Msgpack: newStateMP, } - // helper/schema did previously handle private data during refresh, but - // core is now going to expect this to be maintained in order to - // persist it in the state. - resp.Private = req.Private - return resp, nil } @@ -645,6 +654,7 @@ func (s *GRPCProviderServer) PlanResourceChange(_ context.Context, req *proto.Pl // description that _shows_ there are no changes. This is always the // prior state, because we force a diff above if this is a new instance. resp.PlannedState = req.PriorState + resp.PlannedPrivate = req.PriorPrivate return resp, nil } @@ -705,6 +715,18 @@ func (s *GRPCProviderServer) PlanResourceChange(_ context.Context, req *proto.Pl Msgpack: plannedMP, } + // encode any timeouts into the diff Meta + t := &schema.ResourceTimeout{} + if err := t.ConfigDecode(res, cfg); err != nil { + resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, err) + return resp, nil + } + + if err := t.DiffEncode(diff); err != nil { + resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, err) + return resp, nil + } + // Now we need to store any NewExtra values, which are where any actual // StateFunc modified config fields are hidden. privateMap := diff.Meta