-
Notifications
You must be signed in to change notification settings - Fork 93
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
function: Initial provider defined functions implementation #889
Conversation
Reference: hashicorp/terraform-plugin-go#351 The next versions of the plugin protocol (5.5/6.5) include support for provider defined functions. This change includes initial implementation of that support including: - Temporarily pointing at terraform-plugin-go with provider function support (will be pointed at final terraform-plugin-go release before merge) - New `function` package with all exposed Go types for provider developers to implement provider functions - New `diag` package support for diagnostics with optional function argument information - Implementation of new `GetFunctions` and `CallFunction` RPCs in the internal framework server, protocol 5/6 servers, and data handling between all layers - Initial website documentation This functionality may be released as experimental without compatibility promises until the protocol and Terraform's handling of provider functions is finalized. In that situation, all Go and website documentation will include additional callouts about the experimental nature of the functionality.
…ts/attributes The `AllowNullValue` parameter setting only prevents the value itself from being null, not elements/attributes.
…tribute value testing
…ment/attribute testing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🚀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks awesome! I have some notes in the PR and then one general question.
In a use-case where you have a variadic parameter and you'd like to raise a diagnostic on one specific element..... Is that technically possible? I believe based on the protocol it should be? Should we document this if it is/isn't possible?
Like if I have a function that accepts one variadic IP Address string parameter, could I raise a diagnostic on the second argument:
provider::example::process_ips("127.0.0.1", "not-valid", "192.168.0.1")
@@ -18,16 +18,21 @@ func GetMetadataResponse(ctx context.Context, fw *fwserver.GetMetadataResponse) | |||
} | |||
|
|||
protov6 := &tfprotov5.GetMetadataResponse{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I know this was existing code, but just noticed, should probably be named protov5
or proto
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll submit a followup PR to standardize these a little better to "protoResp" or something.
Co-authored-by: Austin Valle <[email protected]>
Co-authored-by: Austin Valle <[email protected]>
Co-authored-by: Austin Valle <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the thorough review, will poke at these items and get them fixed this afternoon or on Monday.
function/arguments_data.go
Outdated
if fwreflect.IsGenericAttrValue(ctx, target) { | ||
//nolint:forcetypeassert // Type assertion is guaranteed by the above `reflect.IsGenericAttrValue` function | ||
*(target.(*attr.Value)) = attrValue | ||
return nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be continue
-- will fix.
pointer((*bool)(nil)), | ||
pointer("test"), | ||
}, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do. Will also create followup issue to do similar for tfsdk
type handling where this logic came from as well, if they are missing them.
position: 0, | ||
target: new(*bool), | ||
expected: pointer((*bool)(nil)), | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as above.
function/list_parameter_test.go
Outdated
// "CustomType": { | ||
// parameter: function.ListParameter{ | ||
// CustomType: testtypes.ListType{}, | ||
// }, | ||
// expected: testtypes.ListType{}, | ||
// }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I should have left a code comment, and its now been too long for me to exactly remember. Will take a peek as part of fixing other things here.
…ng zero-based and how to handle variadic parameters
Indeed it is. 👍 Provider developers just need to consider the position consolidation done by the framework for the argument data. Its one of the hopefully few downsides to the variadic argument to list data handling approach, but I personally think the benefits (e.g. simplicity to still use Took a swing at documenting how that should be implemented in c4a1246. Please let me know if that is clear or if we should document that differently. |
I agree the list handling looks better and is more Go-like ™️. All the documentation updates look great, thanks! |
Co-authored-by: Austin Valle <[email protected]>
…in-framework into functions
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. |
Reference: hashicorp/terraform-plugin-go#351
The next versions of the plugin protocol (5.5/6.5) include support for provider defined functions. This change includes initial implementation of that support including:
function
package with all exposed Go types for provider developers to implement provider functionsdiag
package support for diagnostics with optional function argument informationGetFunctions
andCallFunction
RPCs in the internal framework server, protocol 5/6 servers, and data handling between all layersThis functionality will be released as technical preview without compatibility promises until Terraform 1.8 is generally available. Go and website documentation include additional callouts about the compatibility of this functionality.