From 706ef186f307acd65ada09cbeee2cdc84f7f502b Mon Sep 17 00:00:00 2001 From: Brian Flad Date: Thu, 21 Apr 2022 16:44:41 -0400 Subject: [PATCH 1/3] tfsdk: Move Resource ImportState method to optional ResourceWithImportState interface Reference: https://github.com/hashicorp/terraform-plugin-framework/issues/160 Due to provider developer feedback, it has been suggested to make the current `Resource` interface `ImportState` method optional. This change accomplishes that by moving the existing method signature to a new `ResourceWithImportState` interface. This also deprecates the `ResourceImportStateNotImplemented()` function. Providers can now signal that a resource does not support import by omitting the `ImportState` method and the framework will automatically return an error diagnostic. From a quick GitHub search, it appeared there was only one usage of a custom error message outside the framework testing. However, it was only being used to include the resource type in the message and was of no real utility over the generic messaging. A code comment is left with a suggested implementation, should there be a feature request for customized messaging. --- .changelog/pending.txt | 11 +++ tfsdk/resource.go | 13 +--- tfsdk/resource_import.go | 16 ++++ tfsdk/serve_import.go | 25 +++++- tfsdk/serve_import_test.go | 17 +++- tfsdk/serve_provider_test.go | 1 + ..._resource_attribute_plan_modifiers_test.go | 4 - .../serve_resource_config_validators_test.go | 3 - ...ource_import_state_not_implemented_test.go | 77 +++++++++++++++++++ tfsdk/serve_resource_one_test.go | 4 - tfsdk/serve_resource_three_test.go | 4 - tfsdk/serve_resource_two_test.go | 4 - ...serve_resource_upgrade_state_empty_test.go | 3 - tfsdk/serve_resource_upgrade_state_test.go | 3 - tfsdk/serve_resource_validate_config_test.go | 3 - tfsdk/serve_test.go | 2 + 16 files changed, 151 insertions(+), 39 deletions(-) create mode 100644 .changelog/pending.txt create mode 100644 tfsdk/serve_resource_import_state_not_implemented_test.go diff --git a/.changelog/pending.txt b/.changelog/pending.txt new file mode 100644 index 00000000..c1003067 --- /dev/null +++ b/.changelog/pending.txt @@ -0,0 +1,11 @@ +```release-note:note +tfsdk: The `Resource` interface no longer requires the `ImportState` method. A separate `ResourceWithImportState` interface now defines the same `ImportState` method. +``` + +```release-note:note +tfsdk: The `ResourceImportStateNotImplemented()` function has been deprecated. Instead, the `ImportState` method can be removed from the `Resource` and the framework will automatically return an error diagnostic if import is attempted. +``` + +```release-note:enhancement +tfsdk: Added `ResourceWithImportState` interface, which allows `Resource` implementations to optionally define the `ImportState` method. +``` diff --git a/tfsdk/resource.go b/tfsdk/resource.go index c74eea42..5631ccf1 100644 --- a/tfsdk/resource.go +++ b/tfsdk/resource.go @@ -19,6 +19,10 @@ type ResourceType interface { // Resource represents a resource instance. This is the core interface that all // resources must implement. +// +// It is also conventional for resources to implement the +// ResourceWithImportState interface, which enables practitioners to import +// existing infrastructure into Terraform. type Resource interface { // Create is called when the provider must create a new resource. Config // and planned state values should be read from the @@ -41,15 +45,6 @@ type Resource interface { // Delete is called when the provider must delete the resource. Config // values may be read from the DeleteResourceRequest. Delete(context.Context, DeleteResourceRequest, *DeleteResourceResponse) - - // ImportState is called when the provider must import the resource. - // - // If import is not supported, it is recommended to use the - // ResourceImportStateNotImplemented() call in this method. - // - // If setting an attribute with the import identifier, it is recommended - // to use the ResourceImportStatePassthroughID() call in this method. - ImportState(context.Context, ImportResourceStateRequest, *ImportResourceStateResponse) } // ResourceWithModifyPlan represents a resource instance with a ModifyPlan diff --git a/tfsdk/resource_import.go b/tfsdk/resource_import.go index 4c166c37..f2201891 100644 --- a/tfsdk/resource_import.go +++ b/tfsdk/resource_import.go @@ -6,10 +6,26 @@ import ( "github.com/hashicorp/terraform-plugin-go/tftypes" ) +// Optional interface on top of Resource that enables provider control over +// the ImportResourceState RPC. This RPC is called by Terraform when the +// `terraform import` command is executed. Afterwards, the ReadResource RPC +// is executed to allow providers to fully populate the resource state. +type ResourceWithImportState interface { + // ImportState is called when the provider must import the state of a + // resource instance. This method must return enough state so the Read + // method can properly refresh the full resource. + // + // If setting an attribute with the import identifier, it is recommended + // to use the ResourceImportStatePassthroughID() call in this method. + ImportState(context.Context, ImportResourceStateRequest, *ImportResourceStateResponse) +} + // ResourceImportStateNotImplemented is a helper function to return an error // diagnostic about the resource not supporting import. The details defaults // to a generic message to contact the provider developer, but can be // customized to provide specific information or recommendations. +// +// Deprecated: Remove the ImportState method from the Resource instead. func ResourceImportStateNotImplemented(ctx context.Context, details string, resp *ImportResourceStateResponse) { if details == "" { details = "This resource does not support import. Please contact the provider developer for additional information." diff --git a/tfsdk/serve_import.go b/tfsdk/serve_import.go index da8ec5c3..1eeea0ad 100644 --- a/tfsdk/serve_import.go +++ b/tfsdk/serve_import.go @@ -4,6 +4,7 @@ import ( "context" "github.com/hashicorp/terraform-plugin-framework/diag" + "github.com/hashicorp/terraform-plugin-framework/internal/logging" "github.com/hashicorp/terraform-plugin-go/tfprotov6" "github.com/hashicorp/terraform-plugin-go/tftypes" ) @@ -85,6 +86,26 @@ func (s *server) importResourceState(ctx context.Context, req *tfprotov6.ImportR return } + resourceWithImportState, ok := resource.(ResourceWithImportState) + + if !ok { + // If there is a feature request for customizing this messaging, + // provider developers can implement a ImportState method that + // immediately returns a custom error diagnostic. + // + // However, implementing the ImportState method could cause issues + // with automated documentation generation, which likely would check + // if the resource implements the ResourceWithImportState interface. + // Instead, a separate "ResourceWithoutImportState" interface could be + // created with a method such as: + // ImportNotImplementedMessage(context.Context) string. + resp.Diagnostics.AddError( + "Resource Import Not Implemented", + "This resource does not support import. Please contact the provider developer for additional information.", + ) + return + } + emptyState := tftypes.NewValue(resourceSchema.TerraformType(ctx), nil) importReq := ImportResourceStateRequest{ ID: req.ID, @@ -96,7 +117,9 @@ func (s *server) importResourceState(ctx context.Context, req *tfprotov6.ImportR }, } - resource.ImportState(ctx, importReq, &importResp) + logging.FrameworkDebug(ctx, "Calling provider defined ImportState") + resourceWithImportState.ImportState(ctx, importReq, &importResp) + logging.FrameworkDebug(ctx, "Called provider defined ImportState") resp.Diagnostics.Append(importResp.Diagnostics...) if resp.Diagnostics.HasError() { diff --git a/tfsdk/serve_import_test.go b/tfsdk/serve_import_test.go index c6d7ad87..0e43bec4 100644 --- a/tfsdk/serve_import_test.go +++ b/tfsdk/serve_import_test.go @@ -155,6 +155,21 @@ func TestServerImportResourceState(t *testing.T) { }, }, }, + "TypeName-ImportState-not-implemented": { + req: &tfprotov6.ImportResourceStateRequest{ + ID: "test", + TypeName: "test_import_state_not_implemented", + }, + resp: &tfprotov6.ImportResourceStateResponse{ + Diagnostics: []*tfprotov6.Diagnostic{ + { + Summary: "Resource Import Not Implemented", + Severity: tfprotov6.DiagnosticSeverityError, + Detail: "This resource does not support import. Please contact the provider developer for additional information.", + }, + }, + }, + }, } for name, tc := range tests { @@ -177,7 +192,7 @@ func TestServerImportResourceState(t *testing.T) { return } - if s.importResourceStateCalledResourceType != tc.req.TypeName { + if tc.req.TypeName == "test_import_state" && s.importResourceStateCalledResourceType != tc.req.TypeName { t.Errorf("Called wrong resource. Expected to call %q, actually called %q", tc.req.TypeName, s.importResourceStateCalledResourceType) return } diff --git a/tfsdk/serve_provider_test.go b/tfsdk/serve_provider_test.go index 907c17d8..477e58fd 100644 --- a/tfsdk/serve_provider_test.go +++ b/tfsdk/serve_provider_test.go @@ -639,6 +639,7 @@ func (t *testServeProvider) GetResources(_ context.Context) (map[string]Resource "test_attribute_plan_modifiers": testServeResourceTypeAttributePlanModifiers{}, "test_config_validators": testServeResourceTypeConfigValidators{}, "test_import_state": testServeResourceTypeImportState{}, + "test_import_state_not_implemented": testServeResourceTypeImportStateNotImplemented{}, "test_upgrade_state": testServeResourceTypeUpgradeState{}, "test_upgrade_state_empty": testServeResourceTypeUpgradeStateEmpty{}, "test_upgrade_state_not_implemented": testServeResourceTypeUpgradeStateNotImplemented{}, diff --git a/tfsdk/serve_resource_attribute_plan_modifiers_test.go b/tfsdk/serve_resource_attribute_plan_modifiers_test.go index ab6aab4a..acae0ee7 100644 --- a/tfsdk/serve_resource_attribute_plan_modifiers_test.go +++ b/tfsdk/serve_resource_attribute_plan_modifiers_test.go @@ -342,7 +342,3 @@ func (r testServeAttributePlanModifiers) Update(ctx context.Context, req UpdateR func (r testServeAttributePlanModifiers) Delete(ctx context.Context, req DeleteResourceRequest, resp *DeleteResourceResponse) { // Intentionally blank. Not expected to be called during testing. } - -func (r testServeAttributePlanModifiers) ImportState(ctx context.Context, req ImportResourceStateRequest, resp *ImportResourceStateResponse) { - ResourceImportStateNotImplemented(ctx, "Not expected to be called during testing.", resp) -} diff --git a/tfsdk/serve_resource_config_validators_test.go b/tfsdk/serve_resource_config_validators_test.go index d25b7081..b97009ba 100644 --- a/tfsdk/serve_resource_config_validators_test.go +++ b/tfsdk/serve_resource_config_validators_test.go @@ -71,9 +71,6 @@ func (r testServeResourceConfigValidators) Update(ctx context.Context, req Updat func (r testServeResourceConfigValidators) Delete(ctx context.Context, req DeleteResourceRequest, resp *DeleteResourceResponse) { // Intentionally blank. Not expected to be called during testing. } -func (r testServeResourceConfigValidators) ImportState(ctx context.Context, req ImportResourceStateRequest, resp *ImportResourceStateResponse) { - ResourceImportStateNotImplemented(ctx, "Not expected to be called during testing.", resp) -} func (r testServeResourceConfigValidators) ConfigValidators(ctx context.Context) []ResourceConfigValidator { r.provider.validateResourceConfigCalledResourceType = "test_config_validators" diff --git a/tfsdk/serve_resource_import_state_not_implemented_test.go b/tfsdk/serve_resource_import_state_not_implemented_test.go new file mode 100644 index 00000000..3e5d0f5b --- /dev/null +++ b/tfsdk/serve_resource_import_state_not_implemented_test.go @@ -0,0 +1,77 @@ +package tfsdk + +import ( + "context" + "fmt" + + "github.com/hashicorp/terraform-plugin-framework/diag" + "github.com/hashicorp/terraform-plugin-framework/types" + "github.com/hashicorp/terraform-plugin-go/tfprotov6" + "github.com/hashicorp/terraform-plugin-go/tftypes" +) + +type testServeResourceTypeImportStateNotImplemented struct{} + +func (dt testServeResourceTypeImportStateNotImplemented) GetSchema(_ context.Context) (Schema, diag.Diagnostics) { + return Schema{ + Attributes: map[string]Attribute{ + "id": { + Type: types.StringType, + Computed: true, + }, + }, + }, nil +} + +func (dt testServeResourceTypeImportStateNotImplemented) NewResource(_ context.Context, p Provider) (Resource, diag.Diagnostics) { + provider, ok := p.(*testServeProvider) + if !ok { + prov, ok := p.(*testServeProviderWithMetaSchema) + if !ok { + panic(fmt.Sprintf("unexpected provider type %T", p)) + } + provider = prov.testServeProvider + } + return testServeResourceImportStateNotImplemented{ + provider: provider, + }, nil +} + +var testServeResourceTypeImportStateNotImplementedSchema = &tfprotov6.Schema{ + Block: &tfprotov6.SchemaBlock{ + Attributes: []*tfprotov6.SchemaAttribute{ + { + Name: "id", + Computed: true, + Type: tftypes.String, + }, + }, + }, +} + +var testServeResourceTypeImportStateNotImplementedTftype = tftypes.Object{ + AttributeTypes: map[string]tftypes.Type{ + "id": tftypes.String, + }, +} + +type testServeResourceImportStateNotImplementedData struct { + Id string `tfsdk:"id"` +} + +type testServeResourceImportStateNotImplemented struct { + provider *testServeProvider +} + +func (r testServeResourceImportStateNotImplemented) Create(ctx context.Context, req CreateResourceRequest, resp *CreateResourceResponse) { + // Intentionally blank. Not expected to be called during testing. +} +func (r testServeResourceImportStateNotImplemented) Read(ctx context.Context, req ReadResourceRequest, resp *ReadResourceResponse) { + // Intentionally blank. Not expected to be called during testing. +} +func (r testServeResourceImportStateNotImplemented) Update(ctx context.Context, req UpdateResourceRequest, resp *UpdateResourceResponse) { + // Intentionally blank. Not expected to be called during testing. +} +func (r testServeResourceImportStateNotImplemented) Delete(ctx context.Context, req DeleteResourceRequest, resp *DeleteResourceResponse) { + // Intentionally blank. Not expected to be called during testing. +} diff --git a/tfsdk/serve_resource_one_test.go b/tfsdk/serve_resource_one_test.go index 7872b912..9348c16a 100644 --- a/tfsdk/serve_resource_one_test.go +++ b/tfsdk/serve_resource_one_test.go @@ -125,7 +125,3 @@ func (r testServeResourceOne) Delete(ctx context.Context, req DeleteResourceRequ r.provider.applyResourceChangeCalledAction = "delete" r.provider.deleteFunc(ctx, req, resp) } - -func (r testServeResourceOne) ImportState(ctx context.Context, req ImportResourceStateRequest, resp *ImportResourceStateResponse) { - ResourceImportStateNotImplemented(ctx, "Not expected to be called during testing.", resp) -} diff --git a/tfsdk/serve_resource_three_test.go b/tfsdk/serve_resource_three_test.go index 1c63cb7a..0f50b725 100644 --- a/tfsdk/serve_resource_three_test.go +++ b/tfsdk/serve_resource_three_test.go @@ -136,7 +136,3 @@ func (r testServeResourceThree) Update(ctx context.Context, req UpdateResourceRe func (r testServeResourceThree) Delete(ctx context.Context, req DeleteResourceRequest, resp *DeleteResourceResponse) { // Intentionally blank. Not expected to be called during testing. } - -func (r testServeResourceThree) ImportState(ctx context.Context, req ImportResourceStateRequest, resp *ImportResourceStateResponse) { - ResourceImportStateNotImplemented(ctx, "Not expected to be called during testing.", resp) -} diff --git a/tfsdk/serve_resource_two_test.go b/tfsdk/serve_resource_two_test.go index da9600a3..ce8346e5 100644 --- a/tfsdk/serve_resource_two_test.go +++ b/tfsdk/serve_resource_two_test.go @@ -207,10 +207,6 @@ func (r testServeResourceTwo) Delete(ctx context.Context, req DeleteResourceRequ r.provider.deleteFunc(ctx, req, resp) } -func (r testServeResourceTwo) ImportState(ctx context.Context, req ImportResourceStateRequest, resp *ImportResourceStateResponse) { - ResourceImportStateNotImplemented(ctx, "Not expected to be called during testing.", resp) -} - func (r testServeResourceTwo) ModifyPlan(ctx context.Context, req ModifyResourcePlanRequest, resp *ModifyResourcePlanResponse) { r.provider.planResourceChangePriorStateValue = req.State.Raw r.provider.planResourceChangePriorStateSchema = req.State.Schema diff --git a/tfsdk/serve_resource_upgrade_state_empty_test.go b/tfsdk/serve_resource_upgrade_state_empty_test.go index 993536e6..750ad0f5 100644 --- a/tfsdk/serve_resource_upgrade_state_empty_test.go +++ b/tfsdk/serve_resource_upgrade_state_empty_test.go @@ -87,9 +87,6 @@ func (r testServeResourceUpgradeStateEmpty) Update(ctx context.Context, req Upda func (r testServeResourceUpgradeStateEmpty) Delete(ctx context.Context, req DeleteResourceRequest, resp *DeleteResourceResponse) { // Intentionally blank. Not expected to be called during testing. } -func (r testServeResourceUpgradeStateEmpty) ImportState(ctx context.Context, req ImportResourceStateRequest, resp *ImportResourceStateResponse) { - ResourceImportStateNotImplemented(ctx, "intentionally not implemented", resp) -} func (r testServeResourceUpgradeStateEmpty) UpgradeState(ctx context.Context) map[int64]ResourceStateUpgrader { return nil diff --git a/tfsdk/serve_resource_upgrade_state_test.go b/tfsdk/serve_resource_upgrade_state_test.go index 8dc233fb..27c72773 100644 --- a/tfsdk/serve_resource_upgrade_state_test.go +++ b/tfsdk/serve_resource_upgrade_state_test.go @@ -123,9 +123,6 @@ func (r testServeResourceUpgradeState) Update(ctx context.Context, req UpdateRes func (r testServeResourceUpgradeState) Delete(ctx context.Context, req DeleteResourceRequest, resp *DeleteResourceResponse) { // Intentionally blank. Not expected to be called during testing. } -func (r testServeResourceUpgradeState) ImportState(ctx context.Context, req ImportResourceStateRequest, resp *ImportResourceStateResponse) { - ResourceImportStateNotImplemented(ctx, "intentionally not implemented", resp) -} func (r testServeResourceUpgradeState) UpgradeState(ctx context.Context) map[int64]ResourceStateUpgrader { r.provider.upgradeResourceStateCalledResourceType = "test_upgrade_state" diff --git a/tfsdk/serve_resource_validate_config_test.go b/tfsdk/serve_resource_validate_config_test.go index 9ba823af..41a40fb0 100644 --- a/tfsdk/serve_resource_validate_config_test.go +++ b/tfsdk/serve_resource_validate_config_test.go @@ -71,9 +71,6 @@ func (r testServeResourceValidateConfig) Update(ctx context.Context, req UpdateR func (r testServeResourceValidateConfig) Delete(ctx context.Context, req DeleteResourceRequest, resp *DeleteResourceResponse) { // Intentionally blank. Not expected to be called during testing. } -func (r testServeResourceValidateConfig) ImportState(ctx context.Context, req ImportResourceStateRequest, resp *ImportResourceStateResponse) { - ResourceImportStateNotImplemented(ctx, "Not expected to be called during testing.", resp) -} func (r testServeResourceValidateConfig) ValidateConfig(ctx context.Context, req ValidateResourceConfigRequest, resp *ValidateResourceConfigResponse) { r.provider.validateResourceConfigCalledResourceType = "test_validate_config" diff --git a/tfsdk/serve_test.go b/tfsdk/serve_test.go index 32da7cc2..81d80f07 100644 --- a/tfsdk/serve_test.go +++ b/tfsdk/serve_test.go @@ -314,6 +314,7 @@ func TestServerGetProviderSchema(t *testing.T) { "test_attribute_plan_modifiers": testServeResourceTypeAttributePlanModifiersSchema, "test_config_validators": testServeResourceTypeConfigValidatorsSchema, "test_import_state": testServeResourceTypeImportStateSchema, + "test_import_state_not_implemented": testServeResourceTypeImportStateNotImplementedSchema, "test_upgrade_state": testServeResourceTypeUpgradeStateSchema, "test_upgrade_state_empty": testServeResourceTypeUpgradeStateEmptySchema, "test_upgrade_state_not_implemented": testServeResourceTypeUpgradeStateNotImplementedSchema, @@ -352,6 +353,7 @@ func TestServerGetProviderSchemaWithProviderMeta(t *testing.T) { "test_attribute_plan_modifiers": testServeResourceTypeAttributePlanModifiersSchema, "test_config_validators": testServeResourceTypeConfigValidatorsSchema, "test_import_state": testServeResourceTypeImportStateSchema, + "test_import_state_not_implemented": testServeResourceTypeImportStateNotImplementedSchema, "test_upgrade_state": testServeResourceTypeUpgradeStateSchema, "test_upgrade_state_empty": testServeResourceTypeUpgradeStateEmptySchema, "test_upgrade_state_not_implemented": testServeResourceTypeUpgradeStateNotImplementedSchema, From a094085d6dd52ae5acf6bea12b485714c6e2b275 Mon Sep 17 00:00:00 2001 From: Brian Flad Date: Thu, 21 Apr 2022 16:51:59 -0400 Subject: [PATCH 2/3] tfsdk: Remove unused code --- ...serve_resource_import_state_not_implemented_test.go | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/tfsdk/serve_resource_import_state_not_implemented_test.go b/tfsdk/serve_resource_import_state_not_implemented_test.go index 3e5d0f5b..c8173de6 100644 --- a/tfsdk/serve_resource_import_state_not_implemented_test.go +++ b/tfsdk/serve_resource_import_state_not_implemented_test.go @@ -49,16 +49,6 @@ var testServeResourceTypeImportStateNotImplementedSchema = &tfprotov6.Schema{ }, } -var testServeResourceTypeImportStateNotImplementedTftype = tftypes.Object{ - AttributeTypes: map[string]tftypes.Type{ - "id": tftypes.String, - }, -} - -type testServeResourceImportStateNotImplementedData struct { - Id string `tfsdk:"id"` -} - type testServeResourceImportStateNotImplemented struct { provider *testServeProvider } From 5050977a0e595228fd3bd2ebe4317d093ae3c3cc Mon Sep 17 00:00:00 2001 From: Brian Flad Date: Thu, 21 Apr 2022 16:52:06 -0400 Subject: [PATCH 3/3] Update CHANGELOG for #297 --- .changelog/{pending.txt => 297.txt} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename .changelog/{pending.txt => 297.txt} (100%) diff --git a/.changelog/pending.txt b/.changelog/297.txt similarity index 100% rename from .changelog/pending.txt rename to .changelog/297.txt