Adding Provider-Defined Function Testing #210
Conversation
…go.cty is released (#202)
| "bool": { | ||
| Parameters: []*tfprotov6.FunctionParameter{ | ||
| { | ||
| Name: "param", |
There was a problem hiding this comment.
During the initial configuration of the Server() func to add functions, the Name field of the parameter was omitted, giving rise to the following error during execution of the TestAccV6FunctionBool test:
=== RUN TestAccV6FunctionBool
function_bool_test.go:17: Step 1/1 error: Error running pre-apply plan: exit status 1
Error: Call to unknown function
on terraform_plugin_test.tf line 13, in output "test":
13: value = provider::corner::bool(true)
There are no functions in namespace "provider::corner::".
--- FAIL: TestAccV6FunctionBool (0.55s)Executing the test with TF_LOG=TRACE did not reveal that the issue was a consequence of the Name field being an empty string (refer to log output).
This is unlikely to be an issue from the standpoint of provider developers adding provider-defined functions as terraform-plugin-framework automatically adds the string "param" to function parameter Name fields, if the field is unpopulated, but I thought it might be worth mentioning in case the error generated by Terraform should be more explicit.
There was a problem hiding this comment.
Would you be able to double check this with the latest alpha or beta1 (potentially releasing later today)? There was some work done in core to improve various function errors, e.g. hashicorp/terraform#34683
If it still doesn't seem reasonable, I would raise an issue in that issue tracker! 😄
There was a problem hiding this comment.
Latest terraform-plugin-framework contains fix for: * hashicorp/terraform-plugin-framework#914 * hashicorp/terraform-plugin-framework#919
| // Using 9223372036854775808, the smallest number that can't be represented as an int64, | ||
| // results in an Terraform error where [Planned value does not match config value for cty.NumberIntVal], | ||
| // which is related to a bug in go.cty relating to [Large integer comparisons and msgpack encoding]. | ||
| // A value of 9223372036854775809 is used for the meanwhile. | ||
| // | ||
| // [Planned value does not match config value for cty.NumberIntVal]: https://github.com/hashicorp/terraform/issues/34589 | ||
| // [Large integer comparisons and msgpack encoding]: https://github.com/zclconf/go-cty/pull/176 |
There was a problem hiding this comment.
I super love/appreciate this additional code comment context! Looks like this has been closed out upstream and should be resolved as of Terraform 1.8.0-beta1, which I think means we can remove this comment now, but curious if you think this context needs to hang out anywhere?
There was a problem hiding this comment.
Agree on the removal. I've also reverted the integer that's used in the test to 9223372036854775808.
I don't think we need to explicitly add anything relating to this issue/discovery within the terraform-provider-corner repository, and there's an issue which captures the context in terraform-plugin-framework, which I believe should be sufficient.
| @@ -24,6 +24,7 @@ func TestSchemaResource_Float64Attribute_Precision(t *testing.T) { | |||
| Config: `resource "framework_float64_precision" "test" { | |||
| float64_attribute = 1 - 0.99 | |||
| }`, | |||
| //nolint:staticcheck //Deprecated functions | |||
There was a problem hiding this comment.
Super nit: I think these can be removed for now? 😄
| "github.com/hashicorp/terraform-provider-corner/internal/backend" | ||
| ) | ||
|
|
||
| var ( | ||
| _ provider.ProviderWithFunctions = (*testProvider)(nil) |
There was a problem hiding this comment.
FYI: This can be added to framework5provider for consistency 👍
| tfversion.SkipBelow(tfversion.Version1_8_0), | ||
| }, | ||
| ProtoV5ProviderFactories: map[string]func() (tfprotov5.ProviderServer, error){ | ||
| //nolint:unparam // False positive in unparam related to map: https://github.com/mvdan/unparam/issues/40 |
There was a problem hiding this comment.
❤️ having the actual false positive issue link here -- I think I've typically said something in the nolint comment about the error return being part of the required function signature in the past, but this is much clearer.
|
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: #202