-
Notifications
You must be signed in to change notification settings - Fork 94
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
tfsdk: Support for List and Set Blocks #188
Conversation
Reference: #85 This implementation is analogous to the existing `Attributes` field on `Attribute`. While the framework handles the major differences at the protocol layer during conversion, it also must enforce the constraints of the underlying type system. Some notable features include: * Defining schemas is very similar to `Attributes`. * Accessing and writing `Config`, `Plan`, and `State` data is no different than `Attributes` with the same nesting mode. * Blocks are structural by Terraform's and cty's definition, meaning there is no concept of `Computed`, `Optional`, `Required`, or `Sensitive`. Checks are in place to enforce these constraints. The primary purpose for supporting blocks is to allow previously existing schemas defined by the older Terraform Plugin SDK to not require practitioner breaking changes upon migrating to the framework (except the protocol version and therefore the minimum Terraform CLI version). This also allows this framework to be muxed with the older framework since the provider schema must match. It is expected that over time any schema definitions including `Blocks` will migrate to `Attributes`. Provider developers should always opt for `Attributes` in new schema definitions.
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.
Start of a review, getting distracted so going to leave the comments I have, but still have to look through a lot of this, sorry.
…attr.Value).Type() in Block conversion errors
I've addressed the first round of feedback. I've created #222 for plan modification so we can either try to include that in this first round of support or later on since that's another large code chunk. |
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.
Some final minor comments, but on the whole looks good to me.
A lot going on here, and it's really bringing home for me how badly #215 is needed. But I think I'm as confident as reasonably possible in the changes.
I think there's a lingering question in my mind of whether we want blocks to match nested attributes in using enum functions (e.g. tfsdk.ListNestedAttributes
but ListNestedBlocks
instead) just for uniformity, but I'm 🤔 about that design and whether we want to keep it, anyways, so I think that's always something we can tackle later, if we need to.
Awesome work.
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.
Let's do it 😎
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 #85
The primary purpose for supporting blocks is to allow previously existing schemas defined by the older Terraform Plugin SDK to not require practitioner breaking changes upon migrating to the framework (except the protocol version and therefore the minimum Terraform CLI version unless a tfprotov5 server implementation is added to this framework as well). This also allows this framework to be muxed with the older framework since the provider schema must match. It is expected that over time any schema definitions including
Blocks
will migrate toAttributes
.Provider developers should always opt for
Attributes
in new schema definitions.