From 8e10ed859cb45881a7370b6d01f9e96fb27cbaaa Mon Sep 17 00:00:00 2001 From: Brian Flad Date: Fri, 22 Apr 2022 15:40:20 -0400 Subject: [PATCH 1/2] tfsdk: Added automatic RemoveResource() call after Resource type Delete method execution Reference: https://github.com/hashicorp/terraform-plugin-framework/issues/100 Due to provider developer feedback, the framework will now perform automatic removal of state on successful (no error diagnostic) `Resource` type `Delete` executions. This matches the behavior of terraform-plugin-sdk. Providers can still explicitly call `(State).RemoveResource()` or leave existing implementations, however it is extraneous for most `Delete` logic now. --- .changelog/pending.txt | 7 ++++ tfsdk/resource.go | 4 ++ tfsdk/serve.go | 6 +++ tfsdk/serve_test.go | 84 +++++++++++++++++++++++++++++++++++++++++- tfsdk/state.go | 3 ++ 5 files changed, 102 insertions(+), 2 deletions(-) create mode 100644 .changelog/pending.txt diff --git a/.changelog/pending.txt b/.changelog/pending.txt new file mode 100644 index 000000000..522ab91d1 --- /dev/null +++ b/.changelog/pending.txt @@ -0,0 +1,7 @@ +```release-note:note +tfsdk: Providers may now optionally remove `RemoveResource()` calls from `Resource` type `Delete` methods +``` + +```release-note:enhancement +tfsdk: Added automatic `(DeleteResourceResponse.State).RemoveResource()` call after `Resource` type `Delete` method execution if there are no errors +``` diff --git a/tfsdk/resource.go b/tfsdk/resource.go index c74eea420..99c088b1d 100644 --- a/tfsdk/resource.go +++ b/tfsdk/resource.go @@ -40,6 +40,10 @@ type Resource interface { // Delete is called when the provider must delete the resource. Config // values may be read from the DeleteResourceRequest. + // + // If execution completes without error, the framework will automatically + // call DeleteResourceResponse.State.RemoveResource(), so it can be omitted + // from provider logic. Delete(context.Context, DeleteResourceRequest, *DeleteResourceResponse) // ImportState is called when the provider must import the resource. diff --git a/tfsdk/serve.go b/tfsdk/serve.go index 5a0f94c43..b647eb682 100644 --- a/tfsdk/serve.go +++ b/tfsdk/serve.go @@ -1438,6 +1438,12 @@ func (s *server) applyResourceChange(ctx context.Context, req *tfprotov6.ApplyRe } resource.Delete(ctx, destroyReq, &destroyResp) resp.Diagnostics = destroyResp.Diagnostics + + if !resp.Diagnostics.HasError() { + logging.FrameworkTrace(ctx, "No provider defined Delete errors detected, ensuring State is cleared") + destroyResp.State.RemoveResource(ctx) + } + newState, err := tfprotov6.NewDynamicValue(resourceSchema.TerraformType(ctx), destroyResp.State.Raw) if err != nil { resp.Diagnostics.AddError( diff --git a/tfsdk/serve_test.go b/tfsdk/serve_test.go index 32da7cc24..71add9ffe 100644 --- a/tfsdk/serve_test.go +++ b/tfsdk/serve_test.go @@ -4576,7 +4576,8 @@ func TestServerApplyResourceChange(t *testing.T) { action: "delete", resourceType: testServeResourceTypeOneType, destroy: func(ctx context.Context, req DeleteResourceRequest, resp *DeleteResourceResponse) { - resp.State.Raw = tftypes.NewValue(testServeResourceTypeOneType, nil) + // Removing the state prior to the framework should not generate errors + resp.State.RemoveResource(ctx) }, expectedNewState: tftypes.NewValue(testServeResourceTypeOneType, nil), }, @@ -4592,7 +4593,8 @@ func TestServerApplyResourceChange(t *testing.T) { action: "delete", resourceType: testServeResourceTypeOneType, destroy: func(ctx context.Context, req DeleteResourceRequest, resp *DeleteResourceResponse) { - resp.State.Raw = tftypes.NewValue(testServeResourceTypeOneType, nil) + // Removing the state prior to the framework should not generate errors + resp.State.RemoveResource(ctx) resp.Diagnostics.AddAttributeWarning( tftypes.NewAttributePath().WithAttributeName("created_timestamp"), "This is a warning", @@ -4641,6 +4643,84 @@ func TestServerApplyResourceChange(t *testing.T) { }, }, }, + "one_delete_automatic_removeresource": { + priorState: tftypes.NewValue(testServeResourceTypeOneType, map[string]tftypes.Value{ + "name": tftypes.NewValue(tftypes.String, "hello, world"), + "favorite_colors": tftypes.NewValue(tftypes.List{ElementType: tftypes.String}, []tftypes.Value{ + tftypes.NewValue(tftypes.String, "red"), + }), + "created_timestamp": tftypes.NewValue(tftypes.String, "right now I guess"), + }), + resource: "test_one", + action: "delete", + resourceType: testServeResourceTypeOneType, + destroy: func(ctx context.Context, req DeleteResourceRequest, resp *DeleteResourceResponse) { + // The framework should automatically call resp.State.RemoveResource() + }, + expectedNewState: tftypes.NewValue(testServeResourceTypeOneType, nil), + }, + "one_delete_diags_warning_automatic_removeresource": { + priorState: tftypes.NewValue(testServeResourceTypeOneType, map[string]tftypes.Value{ + "name": tftypes.NewValue(tftypes.String, "hello, world"), + "favorite_colors": tftypes.NewValue(tftypes.List{ElementType: tftypes.String}, []tftypes.Value{ + tftypes.NewValue(tftypes.String, "red"), + }), + "created_timestamp": tftypes.NewValue(tftypes.String, "right now I guess"), + }), + resource: "test_one", + action: "delete", + resourceType: testServeResourceTypeOneType, + destroy: func(ctx context.Context, req DeleteResourceRequest, resp *DeleteResourceResponse) { + // The framework should automatically call resp.State.RemoveResource() + resp.Diagnostics.AddAttributeWarning( + tftypes.NewAttributePath().WithAttributeName("created_timestamp"), + "This is a warning", + "just a warning diagnostic, no behavior changes", + ) + }, + expectedNewState: tftypes.NewValue(testServeResourceTypeOneType, nil), + expectedDiags: []*tfprotov6.Diagnostic{ + { + Severity: tfprotov6.DiagnosticSeverityWarning, + Summary: "This is a warning", + Detail: "just a warning diagnostic, no behavior changes", + Attribute: tftypes.NewAttributePath().WithAttributeName("created_timestamp"), + }, + }, + }, + "one_delete_diags_error_no_automatic_removeresource": { + priorState: tftypes.NewValue(testServeResourceTypeOneType, map[string]tftypes.Value{ + "name": tftypes.NewValue(tftypes.String, "hello, world"), + "favorite_colors": tftypes.NewValue(tftypes.List{ElementType: tftypes.String}, []tftypes.Value{ + tftypes.NewValue(tftypes.String, "red"), + }), + "created_timestamp": tftypes.NewValue(tftypes.String, "right now I guess"), + }), + resource: "test_one", + action: "delete", + resourceType: testServeResourceTypeOneType, + destroy: func(ctx context.Context, req DeleteResourceRequest, resp *DeleteResourceResponse) { + // The framework should NOT automatically call resp.State.RemoveResource() + resp.Diagnostics.AddError( + "This is an error", + "Something went wrong, keep the old state around", + ) + }, + expectedNewState: tftypes.NewValue(testServeResourceTypeOneType, map[string]tftypes.Value{ + "name": tftypes.NewValue(tftypes.String, "hello, world"), + "favorite_colors": tftypes.NewValue(tftypes.List{ElementType: tftypes.String}, []tftypes.Value{ + tftypes.NewValue(tftypes.String, "red"), + }), + "created_timestamp": tftypes.NewValue(tftypes.String, "right now I guess"), + }), + expectedDiags: []*tfprotov6.Diagnostic{ + { + Severity: tfprotov6.DiagnosticSeverityError, + Summary: "This is an error", + Detail: "Something went wrong, keep the old state around", + }, + }, + }, "two_create": { plannedState: tftypes.NewValue(testServeResourceTypeTwoType, map[string]tftypes.Value{ "id": tftypes.NewValue(tftypes.String, "test-instance"), diff --git a/tfsdk/state.go b/tfsdk/state.go index d0340975b..8057ce5be 100644 --- a/tfsdk/state.go +++ b/tfsdk/state.go @@ -326,6 +326,9 @@ func (s State) setAttributeTransformFunc(ctx context.Context, path *tftypes.Attr } // RemoveResource removes the entire resource from state. +// +// If a Resource type Delete method is completed without error, this is +// automatically called on the DeleteResourceResponse.State. func (s *State) RemoveResource(ctx context.Context) { s.Raw = tftypes.NewValue(s.Schema.TerraformType(ctx), nil) } From 0968beffd7e1b9a1c3078fa8f6a0646cb35515d2 Mon Sep 17 00:00:00 2001 From: Brian Flad Date: Fri, 22 Apr 2022 15:43:13 -0400 Subject: [PATCH 2/2] Update CHANGELOG for #301 --- .changelog/{pending.txt => 301.txt} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename .changelog/{pending.txt => 301.txt} (100%) diff --git a/.changelog/pending.txt b/.changelog/301.txt similarity index 100% rename from .changelog/pending.txt rename to .changelog/301.txt