[DRAFT] Add support for write only attributes #1044
Conversation
…r/metaschema` packages
…oods/write-only-attributes # Conflicts: # go.mod # go.sum
…ResourceConfig` RPC
…aSourceConfig`, `ValidateEphemeralResourceConfig`, and `ValidateProviderResourceConfig` RPCs
There was a problem hiding this comment.
Great work @SBGoods! I left some comments but overall this is looking good, once the core support lands we can start verifying some of the edge cases
|
|
||
| // IsWriteOnly should return true if the attribute configuration value is | ||
| // write-only. This is named differently than WriteOnly to prevent a | ||
| // conflict with the tfsdk.Attribute field name. |
There was a problem hiding this comment.
nit: should probably drop a note here that write-only is a managed-resource schema concept only 👍🏻
| @@ -185,3 +186,8 @@ func (a BoolAttribute) IsRequired() bool { | |||
| func (a BoolAttribute) IsSensitive() bool { | |||
| return a.Sensitive | |||
| } | |||
|
|
|||
| // IsWriteOnly returns false as write-only attributes are not supported in ephemeral resource schemas. | |||
There was a problem hiding this comment.
nit: Could be helpful to reword this and potentially other/future docs (also relevant to provider/schema and provider/metaschema)
Rather than "not supported", I'd say specifically they are "not relevant" to ephemeral / provider schemas, as these schemas describe data that is explicitly not saved to any artifact.
| @@ -188,3 +189,8 @@ func (a Float64Attribute) IsRequired() bool { | |||
| func (a Float64Attribute) IsSensitive() bool { | |||
| return a.Sensitive | |||
| } | |||
|
|
|||
| // IsWriteOnly returns false as write-only attributes are not supported in ephemeral resource schemas. | |||
There was a problem hiding this comment.
nit: not a big deal but I see some of the ephemeral attributes don't have new tests. In general it's probably not super important for these to have tests but since we already do that testing for providers/is_computed, probably fine to keep it 👍🏻
| @@ -43,6 +43,8 @@ func ApplyResourceChangeRequest(ctx context.Context, proto5 *tfprotov5.ApplyReso | |||
| Resource: resource, | |||
| } | |||
|
|
|||
| fw.ClientCapabilities = ApplyResourceChangeClientCapabilities(proto5.ClientCapabilities) | |||
There was a problem hiding this comment.
nit: This can just go in the struct literal above since there are no diagnostics to handle (similar note to any other client capabilities)
| "test_attribute": resourceschema.BoolAttribute{ | ||
| Computed: true, | ||
| WriteOnly: true, | ||
| }, |
There was a problem hiding this comment.
nit: I know this won't hit the validation logic that would typically prevent this, just thought it was weird to have this in a test since it's not valid 😆
| "Error modifying state", | ||
| "There was an unexpected error modifying the NewState. This is always a problem with the provider. Please report the following to the provider developer:\n\n"+err.Error(), |
There was a problem hiding this comment.
| "Error modifying state", | |
| "There was an unexpected error modifying the NewState. This is always a problem with the provider. Please report the following to the provider developer:\n\n"+err.Error(), | |
| "Error Modifying State", | |
| "There was an unexpected error modifying the NewState. This is always a problem with the provider. Please report the following to the provider developer:\n\n"+err.Error(), |
| "Error modifying state", | ||
| "There was an unexpected error modifying the NewState. This is always a problem with the provider. Please report the following to the provider developer:\n\n"+err.Error(), |
There was a problem hiding this comment.
| "Error modifying state", | |
| "There was an unexpected error modifying the NewState. This is always a problem with the provider. Please report the following to the provider developer:\n\n"+err.Error(), | |
| "Error Modifying State", | |
| "There was an unexpected error modifying the NewState. This is always a problem with the provider. Please report the following to the provider developer:\n\n"+err.Error(), |
| "github.com/hashicorp/terraform-plugin-framework/types/basetypes" | ||
| ) | ||
|
|
||
| func NullifyWriteOnlyAttributes(ctx context.Context, resourceSchema fwschema.Schema) func(*tftypes.AttributePath, tftypes.Value) (tftypes.Value, error) { |
There was a problem hiding this comment.
nit: maybe a comment here or above the calling of this function on why this exists. As this is something a provider developer technically could do on their own, but we're handling it for them. Terraform core will (or should eventually) throw a data consistency error if these values are non-null, so we're just ensuring that's the case.
| } | ||
|
|
||
| // Value type from new state to create null with | ||
| newValueType := val.Type() |
There was a problem hiding this comment.
Should this type come from the schema/attribute instead?
There was a problem hiding this comment.
Similar note to the other tests, for logic like this, would be nice to cover the full range of nested attributes/block combinations if reasonable.
Would also be nice to see tests related to custom types, like for example, it's possible to encounter tuple values with dynamic types, a custom type could return a collection with a dynamic element type, etc.
|
This will be super helpful. I'm looking forward to it being available. When a plan is being generated, will the state value be used in place of a value picked up during Put another, way, if a user amends the configuration of a write-only attribute, terraform will plan to call Will |
No description provided.