helper/schema: Add SchemaFunc field to Resource type #1218
Conversation
Reference: #1217 This change introduces a new `SchemaFunc` field and `SchemaMap` method to the `Resource` type. Currently, the `Schema` field data of all `Resource` is held in memory for the lifecycle of the provider server, which is problematic for providers with many resources and/or larger schemas in resources. The new field enables provider developers to swap pieces of resident memory usage for the slight additional computation time of reconstituting the data when necessary. Callers directly referencing the exported `Schema` field should switch to referencing the `SchemaMap` method, which returns the result of `SchemaFunc` or `Schema` in that preference order. To ensure internal usage was migrated to the new method, this change was performed by temporarily commenting out the `Schema` field itself with broken references in non-testing code migrated to the method. The `Schema` field is not marked as deprecated via Go documentation comment as this would introduce a major ecosystem burden to migrate with generally little benefit for most use cases. The `Resource` type `InternalValidate` method has been updated to return an error if both `Schema` and `SchemaFunc` are defined. Provider developers are encouraged to migrate resources to the terraform-plugin-framework, as it already behaves in a manner similar to `SchemaFunc` by nature of resource schema data being behind a method call, amongst many of the other benefits outlined at https://developer.hashicorp.com/terraform/plugin/framework-benefits.
| // The keys of this map are the names used in a practitioner configuration, | ||
| // such as the attribute or block name. The values describe the structure | ||
| // and type information of that attribute or block. | ||
| SchemaFunc func() map[string]*Schema |
There was a problem hiding this comment.
Is it excessive to request a context.Context argument here?
There was a problem hiding this comment.
Hmm, maybe it is as I see that callers don't have a context.Context.
There was a problem hiding this comment.
Usually I'd be all about it, but we would need to verify it can be properly passed from RPC logic all the way through to calling these functions. I'm curious about the expected need for it though? e.g. Is there a need to log anything in schema definitions?
Most provider code that I've run across with "schema functions" typically use function signatures without it. For some examples of what I mean:
- https://github.com/hashicorp/terraform-provider-aws/blob/4d0bdf093f90dc731c71b9c115c8bea2546c5ce2/internal/service/quicksight/schema/visual.go#L18
- https://github.com/hashicorp/terraform-provider-azurerm/blob/cd36aef1a2f87c1858fc1e0c08558cf2fb2f39e6/internal/services/compute/linux_virtual_machine_resource.go#L86
- https://github.com/cloudflare/terraform-provider-cloudflare/blob/553ce9fe3b3aa78677258d2af451d0b825ad2a16/internal/sdkv2provider/schema_cloudflare_record.go#L12
So adding the context will then require logic updates in those to pass the upstream context through all downstream schema function calls.
If having a proper context available is important for a use case, I'd suggest migrating the resource to the terraform-plugin-framework instead, which does already thread the context through properly, rather than spending additional time/effort on updating sdk-based code.
There was a problem hiding this comment.
No, I had no concrete use case in mind and there are other "Func"s that don't have a Context argument (e.g. schema.StateFunc), so please ignore my ramblings 😄.
There was a problem hiding this comment.
I took a quick peek at what it might take to support context.Context properly through here and unfortunately the internals meander into a lot of various functions/methods without it, including quite a few exported methods that would require breaking changes. While it would generally be possible to create duplicate, context-aware counterparts, I think its a whole lot of lift.
|
LGTM 🚀. |
|
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. |
Closes #1217
This change introduces a new
SchemaFuncfield andSchemaMapmethod to theResourcetype. Currently, theSchemafield data of allResourceis held in memory for the lifecycle of the provider server, which is problematic for providers with many resources and/or larger schemas in resources. The new field enables provider developers to swap pieces of resident memory usage for the slight additional computation time of reconstituting the data when necessary.Callers directly referencing the exported
Schemafield should switch to referencing theSchemaMapmethod, which returns the result ofSchemaFuncorSchemain that preference order. To ensure internal usage was migrated to the new method, this change was performed by temporarily commenting out theSchemafield itself with broken references in non-testing code migrated to the method. TheSchemafield is not marked as deprecated via Go documentation comment as this would introduce a major ecosystem burden to migrate with generally little benefit for most use cases.The
ResourcetypeInternalValidatemethod has been updated to return an error if bothSchemaandSchemaFuncare defined.Provider developers are encouraged to migrate resources to the terraform-plugin-framework, as it already behaves in a manner similar to
SchemaFuncby nature of resource schema data being behind a method call, amongst many of the other benefits outlined at https://developer.hashicorp.com/terraform/plugin/framework-benefits.