Add actions to provider interface #37006
Conversation
|
Will the version of the protocol version stay at 6.10? |
f3d5ac9 to
0b0cfdd
Compare
| return resp | ||
| } | ||
|
|
||
| func (p *GRPCProvider) InvokeAction(r providers.InvokeActionRequest) (resp providers.InvokeActionResponse) { |
There was a problem hiding this comment.
This one is just a genuine question I'm curious about, not a review:
How will the action event stream get cancelled from the terraform side (ie user ctrl-c or a timeout)? Does that get handled by the provider when tf calls Stop()?
There was a problem hiding this comment.
I would have assumed it stops when we call Stop and the provider winds down, but it's sth we should test for sure
There was a problem hiding this comment.
The framework shouldbe connecting it's global context to interrupt the call when Stop is called, but Terraform can't control that.
For managed resources we don't want to directly interrupt them from the CLI because they may have state to save, and so we wait for the provider to do the cancelation and return whatever info it can. Since list resources can't have side effects however, we might want to someday pipe through our cancellation context here, but I don't think that's required for now, and would require some new plumbing.
There was a problem hiding this comment.
Still mostly nitpicky, I added some minor performance-related comments but I won't block you on them (I spent my last year at grafana staring at flame graphs). I think this looks good overall, but I'd feel better if you get another +1 from someone else (not because I think there are problems, because I'm still re-acquainting myself with terraform)
|
|
||
| linkedResources := make([]providers.LinkedResourcePlan, 0, len(lrs)) | ||
|
|
||
| if len(lrs) != len(linkedResourceSchema) { |
There was a problem hiding this comment.
not a big deal for this particular method, but I'd suggest moving this check to just before you allocate the array, so we don't bother taking the time/memory to allocate then gc it if we return immediately.
internal/plugin/grpc_provider.go
Outdated
|
|
||
| func protoToLinkedResourceResults(schema providers.GetProviderSchemaResponse, linkedResourceSchema []providers.LinkedResourceSchema, lrs []*proto.InvokeAction_Event_Completed_LinkedResource) ([]providers.LinkedResourceResult, error) { | ||
|
|
||
| linkedResources := make([]providers.LinkedResourceResult, 0, len(lrs)) |
There was a problem hiding this comment.
Same here - allocate the array after the potential short circuit exit
| LinkedResources []LinkedResourceSchema | ||
| } | ||
| type ActionSchema struct { | ||
| ConfigSchema *configschema.Block |
There was a problem hiding this comment.
Do we need an ActionSchema Version here, similar to what's in the Schema, or did we decide that's not necessary for actions?
Version int64
There was a problem hiding this comment.
We decided it's not necessary because there is no state associated with actions and therefore also no upgrading, right @jbardin?
| Linked *LinkedAction | ||
| } | ||
|
|
||
| func (a ActionSchema) LinkedResources() []LinkedResourceSchema { |
There was a problem hiding this comment.
More nitpicky stuff - fine to leave this as-is, but I want to see what you think:
Did you consider returning nil for Unlinked actions? That is a simpler check than checking the length of the returned value, and gives us a(nother) easy way to know we're dealing with an unlinked action.
Do you expect that terraform will already know when it's dealing with an unlinked action, and therefore won't bother calling this method, or do you expect it gets called on every action? If it's the latter, I'd suggested making the return value a pointer so we can return nil for unlinked.
There was a problem hiding this comment.
I liked the handling of returning an empty list because from the provider framework side of things this is all we really care about. Is the number of linked actions we try to send the same number as the schema defines.
I would say there should be a different way of getting to know which type of action you are working with through the schema, but I didn't need it for this work so I didn't add it. len(a.LinkedResources) == 0 should never be the check if we talk about an unlinked action or not, we have the oneof type in the schema so we should access that one to know.
|
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. |
This PR adds actions to the provider interface and implements the basic functionality against the provider protocol.
Fixes #
Target Release
1.13.x
CHANGELOG entry