-
Notifications
You must be signed in to change notification settings - Fork 92
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
[DRAFT] Add support for write only attributes #1044
base: av/ephemeral-resources
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this type come from the schema/attribute instead?
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.
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 |
Hey there @chrismarget-j 👋🏻, thanks for the interest and apologies there isn't a nicely written GH issue to discuss this feature on, this PR can suffice in the meantime 😆. First some background: So the current idea for a write-only attribute is that the prior state will always be
Write-only attributes will only be able to be populated by an ephemeral value, so the practitioner will need to model this new "intent", a very simplified example: variable "new_db_password" {
type = string
default = null
sensitive = true
ephemeral = true
}
resource "aws_db_instance" "db" {
# .. other attributes
# This would be a new write-only attribute
new_password = var.new_db_password
} So with that background, to answer your questions:
So the state value will always be That means that the provider will not be able to determine the previously configured value as it will always be If the user enters a new configuration value (or just sets up the configuration in a way where a value is just consistently provided), then a plan will be kicked off with that attribute's prior state as |
No description provided.