Fix for TypeSet applying with unexpected empty element #1042
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fixes hashicorp#895 When calculating a diff, elements in sets are tracked as a delete of all old attributes with `NewRemoved = true`, and a create of all new attributes. When `DiffSuppressFunc` is used for an attribute, the `ResourceAttrDiff` is replaced with `New = Old` (no-op): https://github.com/hashicorp/terraform-plugin-sdk/blob/fa8d313665945816f6eb6c79182e4abdc1540504/helper/schema/schema.go#L1144 However, this also drops all other metadata about the attr diff, such as `NewRemoved`. This ends up affecting the `InstanceDiff.applyBlockDiff` which expects to drop removed items: https://github.com/hashicorp/terraform-plugin-sdk/blob/e14d3b611f2e257d2a0862e5ea0f90ea96fd5bf8/terraform/diff.go#L207 All attributes of the removed set element get removed here, except those with `DiffSuppressFunc` since the `NewRemoved` attribute was dropped. Instead of dropping the metadata about the attr diff, clone the original attr diff, and just set `New = Old`. The added test fails without this fix: ``` === RUN TestSchemaMap_Diff/30-Set_with_DiffSuppressFunc schema_test.go:3188: expected: *terraform.InstanceDiff{ [...]"rule.80.duration":*terraform.ResourceAttrDiff{Old:"", New:"", NewComputed:false, NewRemoved:true, [...] got: *terraform.InstanceDiff{ [...]"rule.80.duration":*terraform.ResourceAttrDiff{Old:"", New:"", NewComputed:false, [...] NewRemoved:false, [...] ``` Previously, `NewRemoved` was set to false for the sustain field even though it belonged to an element being removed.
|
|
|
Friendly bump, this is an issue that has affected many folks (e.g., #588). I think this issue has found the root-cause, and it would be great to get a review from the team on whether this is the right way to address the issue. |
|
Friendly ping, I've also run into this issue. |
|
Friendly bump, this issue is still causing problems for set usage, can any of the maintainers review this -- it's small change with tests. |
|
Is there a process for getting a review from the maintainers, it'd be great to get this in as it seems to be affecting multiple users. |
13 tasks
This was referenced Nov 20, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #652 (also reported by me in #895)
When calculating a diff, elements in sets are tracked as a delete of all
old attributes with
NewRemoved = true, and a create of all newattributes. When
DiffSuppressFuncis used for an attribute, theResourceAttrDiffis replaced withNew = Old(no-op):terraform-plugin-sdk/helper/schema/schema.go
Line 1144 in fa8d313
However, this also drops all other metadata about the attr diff, such as
NewRemoved. This ends up affecting theInstanceDiff.applyBlockDiffwhich expects to drop removed items:
terraform-plugin-sdk/terraform/diff.go
Line 207 in e14d3b6
All attributes of the removed set element get removed here, except those
with
DiffSuppressFuncsince theNewRemovedattribute was dropped.Instead of dropping the metadata about the attr diff, clone the original
attr diff, and just set
New = Old.The added test fails without this fix:
Previously,
NewRemovedwas set to false for the sustain field even though it belonged to an element being removed.