-
Notifications
You must be signed in to change notification settings - Fork 36
Add GenerateResourceConfig RPC
#578
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
base: main
Are you sure you want to change the base?
Conversation
b5fb855 to
0b6f244
Compare
GenerateResourceConfig RPC
austinvalle
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a couple notes on naming/extra funcs to consider, but since there are more PRs coming for protov6, will approve to not block 👍🏻
| // The generate_resource_config capability signals that a provider supports | ||
| // GenerateResourceConfig. | ||
| bool generate_resource_config = 4; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be interesting to flip on, since it's likely providers will have no existing test coverage for generate config (since it's a TF core feature ATM), and it's currently an "all-or-nothing" approach 🤔 (and will likely not write test coverage for resources that don't customize this). Might put providers in a situation where they get bug reports for something that they weren't aware was being enabled for "all" resources.
I don't have a solution, just sharing an observation 😆
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thought from our recent planning meeting - scenario: SDKv2 provider server implements GenerateConfig, but the Framework provider server doesn't. What happens in core?
This could happen if we didn't implement them at the same time (due to priorities), or because a provider doesn't bump both Go module versions at the same time
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The more we talk about this, the more I think that this shouldn't be a temporary interface, rather being required / forcing a build error if all Go modules don't implement this RPC. This mirrors the GetResourceIdentitySchemas RPC, which we didn't choose a build error and ended up with unexpected downstream errors due to providers not implementing it across all provider servers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See https://github.com/hashicorp/terraform-plugin-go/blob/main/.github/CONTRIBUTING.md#temporary-interfaces-for-new-rpcs . I think we could argue this RPC falls under "core will always call it" because of that provider-wide capabilities flag
It's also interesting for any terraform-plugin-go implementations (like kubernetes manifest), this will need to be implemented there as well. That could be a blocker for adoption since it's provider-wide 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All interesting points, if we're making this a required interface should we wait for SDKv2 and Framework to have implementations of this ready before merging? My main concern is blocking the release of any other RPC additions in plugin go that might wrap up earlier like pluggable state store?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, good point 🤔, we can revisit the timing of RPCs merging into main when folks start to get closer to finishing the implementation. We could go with a "whoever finishes first approach" and just plan to separate the releases regardless.
We can revisit once folks start returning from holiday time off 👍🏻
…erate resource config server capabilities flag
d21b223 to
5c31f75
Compare
5c31f75 to
8271d09
Compare
Description
Adding the Generate Resource Config RPC to
tfprotov5Rollback Plan
Changes to Security Controls
Are there any changes to security controls (access controls, encryption, logging) in this pull request? If so, explain.
None