Skip to content
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

Merged
merged 28 commits into from
Dec 19, 2023
Merged

Conversation

bflad
Copy link
Contributor

@bflad bflad commented Dec 11, 2023

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 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.

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.
@bflad bflad added the enhancement New feature or request label Dec 11, 2023
@bflad bflad added this to the v1.5.0 milestone Dec 11, 2023
@bflad bflad requested a review from a team as a code owner December 11, 2023 17:47
Copy link
Contributor

@bendbennett bendbennett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚀

Copy link
Member

@austinvalle austinvalle left a 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")

internal/fwserver/server.go Show resolved Hide resolved
internal/fwserver/server.go Outdated Show resolved Hide resolved
@@ -18,16 +18,21 @@ func GetMetadataResponse(ctx context.Context, fw *fwserver.GetMetadataResponse)
}

protov6 := &tfprotov5.GetMetadataResponse{
Copy link
Member

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

Copy link
Contributor Author

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.

provider/provider.go Outdated Show resolved Hide resolved
function/definition.go Outdated Show resolved Hide resolved
website/docs/plugin/framework/functions/implementation.mdx Outdated Show resolved Hide resolved
website/docs/plugin/framework/functions/implementation.mdx Outdated Show resolved Hide resolved
website/docs/plugin/framework/functions/returns/index.mdx Outdated Show resolved Hide resolved
bflad and others added 3 commits December 15, 2023 12:50
Co-authored-by: Austin Valle <austinvalle@gmail.com>
Co-authored-by: Austin Valle <austinvalle@gmail.com>
Co-authored-by: Austin Valle <austinvalle@gmail.com>
Copy link
Contributor Author

@bflad bflad left a 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.

Comment on lines 76 to 80
if fwreflect.IsGenericAttrValue(ctx, target) {
//nolint:forcetypeassert // Type assertion is guaranteed by the above `reflect.IsGenericAttrValue` function
*(target.(*attr.Value)) = attrValue
return nil
}
Copy link
Contributor Author

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"),
},
},
Copy link
Contributor Author

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)),
},
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as above.

Comment on lines 230 to 235
// "CustomType": {
// parameter: function.ListParameter{
// CustomType: testtypes.ListType{},
// },
// expected: testtypes.ListType{},
// },
Copy link
Contributor Author

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.

website/docs/plugin/framework/functions/documentation.mdx Outdated Show resolved Hide resolved
@bflad
Copy link
Contributor Author

bflad commented Dec 15, 2023

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?

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 (function.ArgumentsData).Get() and similarity to how Go itself implements variadic argument data) outweighs the consideration. Provider developers can also still opt to just raise a generic diagnostic without argument position.

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.

@austinvalle
Copy link
Member

I agree the list handling looks better and is more Go-like ™️. All the documentation updates look great, thanks!

@bflad bflad merged commit 791e37b into main Dec 19, 2023
24 checks passed
@bflad bflad deleted the functions branch December 19, 2023 17:02
Copy link

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.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants