tfsdk: Move Resource ImportState method to optional ResourceWithImportState interface #297
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…tState interface Reference: #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.
detro
approved these changes
Apr 25, 2022
Comment on lines
+89
to
+107
| 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 | ||
| } |
| // 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 { |
There was a problem hiding this comment.
perhaps nit-picking, but shouldn't this interface contain Resource, which would allow for validation that an implementation is fully implementing both using a single line? e.g.
var _ ResourceWithImportState = MyResource{}
# which also implies
# var _ Resource = MyResource{}
bflad
added a commit
that referenced
this pull request
May 3, 2022
bflad
added a commit
that referenced
this pull request
May 3, 2022
|
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Reference: #160
Due to provider developer feedback, it has been suggested to make the current
ResourceinterfaceImportStatemethod optional. This change accomplishes that by moving the existing method signature to a newResourceWithImportStateinterface. This has the add-on effect of allowing future documentation generation easily determine whether resource import support is implemented, by type checking against the new interface.This also deprecates the
ResourceImportStateNotImplemented()function. Providers can now signal that a resource does not support import by omitting theImportStatemethod 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.