search

hashicorp / terraform-plugin-go

A low-level Go binding for the Terraform protocol for integrations to be built on top of.
Mozilla Public License 2.0
127 stars 32 forks source link

tfprotov5+tfprotov6: Add deferred action support to related RPCs #403

Closed austinvalle closed 4 months ago

austinvalle commented 4 months ago

Refs: https://github.com/hashicorp/terraform/pull/34880, https://github.com/hashicorp/terraform/pull/35063

This PR introduces the ClientCapabilities/deferral_allowed fields to the relevant request structs and Deferred/Reason to the relevant response structs. The RPCs in-scope for supporting deferred actions are:

The ConfigureProvider RPC also includes ClientCapabilities/deferral_allowed to assist provider logic that needs to automatically setup deferral responses for the PROVIDER_CONFIG_UNKNOWN reason.

bflad commented 4 months ago

Oh and for your explicit open questions -- these both seem like things that can be added now or later, if we want. The logging might be nice and easy to get in right now though. The diagnostic wouldn't be hard to implement, but it'll be one of the first times introducing automatic RPC-level diagnostics that cannot be overridden by provider developers or higher level SDKs. Not sure if there are any concerns about this codebase owning an unavoidable error and its messaging (probably not?) but I have to ask.

austinvalle commented 4 months ago

@bflad Cool, I'll implement the logging since it's straightforward.

As for the diagnostic, I'm 50/50 on adding the error check... On one hand, it's an implicit expectation of the protocol that Terraform core has which is not immediately obvious without documentation. On the other, there are other implicit expectations around combinations of data (although more difficult/expensive to check!).

I'm leaning towards adding the error check, but I'm going to verify with core to ensure that applying this error handling at the protocol level is appropriate. I feel like if they have an error check on their side for this, we should as well.

Both Framework and SDKv2 will have their own error checks for this scenario, so regardless, it will be handled by the higher-level SDK with it's messaging.

austinvalle commented 4 months ago

@bflad I've made all of the changes we discussed so I think it's ready for the PR to be officially reviewed 😆

--- FAIL: Test_EnsureVersionConstantMatchesProtoFile (3.59s) proto_version_test.go:47: protocol version Go variable is different from proto file - expected: 6.6, got: 6.4 proto_version_test.go:48: MAINTAINER NOTE: Update tf6server.protocolVersionMajor and tf6server.protocolVersionMinor to match the proto file.



- Added a diagnostic for invalid deferred responses. I don't see any tests for the `tf{5/6}server` packages, should I add some or is it fine to just test this in `terraform-provider-corner` with a `terraform-plugin-go` provider?
github-actions[bot] commented 3 months ago

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.