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

Consider Whether Attribute (Or Subset) Should be Exposed in ModifyAttributePlanRequest #389

Closed
bflad opened this issue Jun 21, 2022 · 2 comments
Labels
enhancement New feature or request

Comments

@bflad
Copy link
Member

bflad commented Jun 21, 2022

Module version

v0.9.0

Use-cases

Certain generic attribute plan modifiers will desire performing actions based on the Attribute definition itself. A current use case is RequiresReplace and RequiresReplaceIf, which currently have some checks for the Computed field to preserve some terraform-plugin-sdk/v2 behavior that was present with ForceNew.

Attempted Solutions

Using req.State.Schema.AttributeAtPath(req.AttributePath) to extract the Attribute. This will require logic updates post #81, as the underlying implementation will remain *tftypes.AttributePath until #365 is done.

Proposal

First, we should decide if the top level Attribute definition should be exposed. This would be a brittle implementation detail for framework compatibility and provider developers may make unintended decisions based on exposing everything. The benefit however, is that provider developers do not need to jump through hoops to get the information as the framework would handle all the nitty gritty.

Most (all?) plan modifiers shouldn't need that type of information though, except potentially Computed if they are logically treating that as special for legacy reasons. Another potential future Attribute field is Default attr.Value. If implemented, plan modifiers will most certainly warrant access to this value.

Therefore an initial proposal here would be to only expose targeted Attribute fields, such as Computed or optionally Default if that becomes implemented. It would likely be best to let some of the referenced issues shake out first though, before making any implementation decisions.

References

@bflad bflad added the enhancement New feature or request label Jun 21, 2022
iwarapter pushed a commit to iwarapter/terraform-plugin-framework that referenced this issue Sep 4, 2022
Reference: hashicorp#31
Reference: hashicorp#132
Reference: hashicorp#223
Reference: hashicorp#326
Reference: hashicorp#365
Reference: hashicorp#389

The main goals of this change are:

- Prepare the Go module to support multiple packages that implement concept-specific schema declarations, in particular the `datasource`, `provider`, and `resource` packages
- Continue supporting the existing `tfsdk` package schema implementation with minimal provider developer breaking changes, allowing a deprecation period when the concept-specific schemas are introduced
- Migrate unexported or unintentially exported `tfsdk` schema functionality to internal packages

These goals are accomplished by creating new `internal/fwschema` and `internal/fwxschema` packages, which contain interface types that the provider developer facing packages implement. Currently, the `tfsdk` package is the only implementation, until the design of those concept-specific schema declarations is fully determined. Those designs may include changes to schema implementation details, which cannot be accomplished in the existing `tfsdk` implementation without breaking it and provider developers migrating code to the new packages is a great time to make those types of changes.

The internal interface method design here is purposefully similar to the existing `tfsdk` implementations as we cannot name the methods the same as existing fields and to reduce review burden. After the `tfsdk` implementations are removed, we can consider tidying up the interface methods to drop the `Get` and `Is` method name prefixes.

There are some minor followup changes that will happen either between or during concept-specific schema implementation work, namely:

- Swapping calls `tfsdk` package type `TerraformType()` methods for `AttributeType().TerraformType()` to reduce implementation work for new schema implementations
- Updating unit testing that relies on `tfsdk.Schema` to set up sub-tests via `(*testing.T).Run()` to against multiple implementations

These were not included here to reduce the review burden as they are separate details which can be handled later.
@bflad
Copy link
Member Author

bflad commented Dec 12, 2022

The new resource schema plan modifier for RequiresReplace now does not perform the Computed check. A RequiresReplaceIfConfigured variant is available. As for other potential use cases of attribute-specific information, it is generally recommended to not base plan modifier implementations on those attribute-specific details (e.g. whether or not something is Computed or not, should not be used). If there are other valid use cases out there, please raise a new issue.

@bflad bflad closed this as completed Dec 12, 2022
@github-actions
Copy link

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

1 participant