Skip to content
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 #1375

Draft
wants to merge 28 commits into
base: main
Choose a base branch
from

Conversation

SBGoods
Copy link
Contributor

@SBGoods SBGoods commented Sep 10, 2024

No description provided.

Copy link
Member

@austinvalle austinvalle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 null for 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/schema/grpc_provider.go Show resolved Hide resolved
helper/validation/write_only.go Outdated Show resolved Hide resolved
helper/schema/grpc_provider.go Outdated Show resolved Hide resolved
helper/schema/grpc_provider.go Outdated Show resolved Hide resolved
helper/schema/grpc_provider.go Outdated Show resolved Hide resolved
helper/validation/write_only.go Outdated Show resolved Hide resolved
Comment on lines 76 to 77
Detail: fmt.Sprintf("The attribute %s has a WriteOnly version %s available. "+
"Use the WriteOnly version of the attribute when possible.", attrStep.Name, writeOnlyAttributeName),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may want to bikeshed this warning message wording with the provider development teams

return
}

if !attr.value.IsNull() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🤔

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A corner provider test could shine some light on this problem 💭

},
},
},
"map nested block: oldAttribute and writeOnlyAttribute map returns warning diags": {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar question to one I had above, are map nested blocks in SDKv2? Or is this a map attribute equivalent

helper/validation/path.go Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants