Add support for the cross resource type move operation in the proto schema #34480
Conversation
…ross resource type RPCs
There was a problem hiding this comment.
Optimistically approving as overall looks good to me, just with some minor considerations. 🚀
Regarding the unknown schema state encoding/decoding, if you peek at the UpgradeResourceState RPC handling, you'll see what I believe to be a similar problem there. That RPC opts to send this data in its "raw state" form (JSON of flat map or existing state data), which its up to the provider side to implement the appropriate decoding. Not sure if that implementation detail something worth using here as well and apologies I missed that previously.
| } | ||
| } | ||
|
|
||
| message MoveResourceState { |
There was a problem hiding this comment.
Minor, open question: We haven't shipped even an alpha with protocol version 5.5/6.5 and functions support. Should we just roll this into the existing protocol definitions?
internal/plugin/grpc_provider.go
Outdated
| targetType, ok := schema.ResourceTypes[r.TargetTypeName] | ||
| if !ok { | ||
| resp.Diagnostics = resp.Diagnostics.Append(fmt.Errorf("unknown resource type %q", r.TargetTypeName)) | ||
| return resp | ||
| } |
There was a problem hiding this comment.
Nit: Should this validation occur before the provider is called or other computational effort happens?
There was a problem hiding this comment.
Yep, it will be. I've added a comment explaining this, but I've left the check in anyway. In case something goes wrong earlier, we can still return an error here instead of crashing.
| // SourceProviderAddress is the address of the provider that the resource | ||
| // is being moved from. |
There was a problem hiding this comment.
Nit: Might be good to include these field-level comments in the protocol definition so it is more self-documenting. 👍
internal/providers/provider.go
Outdated
| // SourceStateFlatmap is the legacy flatmap encoding. | ||
| // Only one of these fields may be set for the move request. | ||
| SourceStateJSON []byte | ||
| SourceStateFlatmap map[string]string |
There was a problem hiding this comment.
I would drop this field, as there's no way a new provider should ever see (or be equipped to deal with) the pre-v0.12 encoding
liamcervante
force-pushed
the
liamcervante/cross-resource-move-protos
branch
from
92de399 to
3635724
Compare
|
Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch. |
|
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. |
Update proto schema and provider interfaces with support for moved across resource type RPCs.
Target Release
1.8.0-alpha
Draft CHANGELOG entry
N/A, will write a proper entry for the whole moved-across-resource-type change once completed.