[DRAFT] Add support for write only attributes #1375
Conversation
… `PlanResourceChange()`
…plement validation in `ValidateResourceTypeConfig()` RPC
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.
helper/validation/write_only.go
Outdated
| Detail: fmt.Sprintf("The attribute %s has a WriteOnly version %s available. "+ | ||
| "Use the WriteOnly version of the attribute when possible.", attrStep.Name, writeOnlyAttributeName), |
There was a problem hiding this comment.
We may want to bikeshed this warning message wording with the provider development teams
| return | ||
| } | ||
|
|
||
| if !attr.value.IsNull() { |
There was a problem hiding this comment.
Will be interesting to test this, but I have a feeling for practitioners that are already using the resource, the SDK won't be able to completely null this value out. We may have to check the cty.Value against zero values as well 😞 (empty string, zero, etc.)
I don't know off the top of my head if that's valid, but I'm pretty sure there's nothing a provider dev/practitioner can do to null a value in SDKv2 once it has a value 🤔
There was a problem hiding this comment.
A corner provider test could shine some light on this problem 💭
| }, | ||
| }, | ||
| }, | ||
| "map nested block: oldAttribute and writeOnlyAttribute map returns warning diags": { |
There was a problem hiding this comment.
Similar question to one I had above, are map nested blocks in SDKv2? Or is this a map attribute equivalent
Co-authored-by: Austin Valle <[email protected]>
…teRawResourceConfigFuncs`
# Conflicts: # go.mod # go.sum # helper/schema/grpc_provider.go
…tWriteOnly()` for retrieving write-only values during apply
…Data).GetWriteOnly()` for retrieving write-only values during apply" This reverts commit 1479d62.
…ving write-only attributes during apply.
# Conflicts: # go.mod
There was a problem hiding this comment.
Leaving a comment here to make sure we circle back on this before merging.
I think we should consider adding more internal validation to explicitly prevent using schema fields like ForceNew with WriteOnly. One of the awkward parts of write-only attributes is that they always appear in the resource diff when a value is present (since prior state is always null). Combining that with something like ForceNew means that the provider always indicates the resource should be replaced in the plan response.
At the moment, Terraform is ignoring that because the path in requires_replace is a write-only attribute, although I'm not sure if that is on purpose or not. SDKv2 also has some legacy behavior of always adding id to the requires_replace, although Terraform doesn't seem to be acknowledging that either 🤔 .
Overall, it doesn't make sense for write-only attributes to use the ForceNew field since Terraform can't do anything with that information, so restricting it will at least prevent any awkward side effects from occurring. Unforuntately, I don't think we can't restrict CustomizeDiff to not being used with WriteOnly (which can also indicate replacement), as that's the only place a provider can do validation that is aware of any of the resource's prior state (for example, to know if that resource is being created or not).
| // | ||
| // GetRawConfigAt is considered advanced functionality, and | ||
| // familiarity with the Terraform protocol is suggested when using it. | ||
| func (d *ResourceDiff) GetRawConfigAt(valPath cty.Path) (cty.Value, diag.Diagnostics) { |
There was a problem hiding this comment.
I added a duplicate implementation here for resource diff for usage with CustomizeDiff. Most likely usage would be running some type of validation that requires knowing about prior state (which can be checked with (*ResourceDiff).GetRawState)
|
@SBGoods Since we are already "managing" the setting of
|
No description provided.