all: Add support for write-only attributes#1375
Conversation
… `PlanResourceChange()`
…plement validation in `ValidateResourceTypeConfig()` RPC
austinvalle
left a comment
austinvalle
left a comment
There was a problem hiding this comment.
All of this is looking awesome, great work! I left some initial comments, but in addition to that:
- I'm wondering what will eventually show up in the plan renderer for Terraform... since we are planning non-null values that will eventually be set to null. Feels like it should either explicitly mention that the value is write-only, or we should be planning
nullfor write-only attributes as well. We should make a note to come back to this once core gets further along. - It'd be cool to get some corner testing for SDKv2 to catch any regressions we might introduce in the future
- I'd love to run this development branch against a major cloud TF provider (aws/gcp/azure) if we can get a hand from their teams finding the appropriate CI jobs to do so. They have so many SDKv2 schemas + tests that it would be a good smoke test.
- We would need them to update their plugin-go/mux/etc, so you might need to adjust the plugin-go branch to make that all compile.
Co-authored-by: Austin Valle <austinvalle@gmail.com>
…teRawResourceConfigFuncs`
# Conflicts: # go.mod # go.sum # helper/schema/grpc_provider.go
SBGoods
changed the title
SBGoods
changed the title
| // | ||
| // GetRawConfigAt is considered advanced functionality, and | ||
| // familiarity with the Terraform protocol is suggested when using it. | ||
| func (d *ResourceData) GetRawConfigAt(valPath cty.Path) (cty.Value, diag.Diagnostics) { |
There was a problem hiding this comment.
I would check that this works for nested blocks; specifically traversing across cty.SetVal values is weird and I think cty.Walk is going to fail there.
There was a problem hiding this comment.
Good callout! I've added more test cases in commit d1d4c14. The Set and Set nested block traversal test cases are passing 👍🏾
| var oldAttrs []attribute | ||
|
|
||
| err := cty.Walk(req.RawConfig, func(path cty.Path, value cty.Value) (bool, error) { | ||
| if PathMatches(path, oldAttribute) { |
There was a problem hiding this comment.
should all cty.Walk' calls use PathMatches`? Or are they expected to only have paths from what should be matching objects?
There was a problem hiding this comment.
I'm using PathMatches as a quick and dirty equivalent to the framework's path expressions to indicate locations in the schema (as opposed to schema-based data like config).
The cty.Walk in GetRawConfigAt() could also use PathMatches although it's not entirely clear what exactly the resulting cty.Value should be when specifying "any" nested block in the path, so I'm leaning on waiting for a feature request asking for such functionality. The only downside is that cty.Path now works differently in the SDKv2 depending on the function it's being used for which could be confusing.
austinvalle
left a comment
austinvalle
left a comment
There was a problem hiding this comment.
Looks good! I left some comments, mostly just considerations for documentation. (will re-approve if there are more changes pushed to the PR)
rkoron007
left a comment
rkoron007
left a comment
There was a problem hiding this comment.
I see there are a bunch of suggestions here, so I kept my review light. Happy to go through this again after you get a chance to review Austin's comments! ✨
Co-authored-by: Austin Valle <austinvalle@gmail.com>
Co-authored-by: Austin Valle <austinvalle@gmail.com>
5 participants
Adds support for new write-only attribute feature in Terraform
v1.11or higher. Write-only attributes are managed resource attributes whose values are not saved to the Terraform plan or state artifacts and can accept ephemeral values.A provider can declare an attribute as write-only in the schema using the
WriteOnlyfield. Write-only attributes values are only available in the configuration, and the prior state, planned state, and final state values for write-only attributes should always benull.