-
Notifications
You must be signed in to change notification settings - Fork 227
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
Apply results in an incorrect Update for TypeSet with an unexpected empty element #895
Comments
Same matter for me |
Similar: #588. |
Hi @prashantv 👋 Thank you for raising this and sorry you ran into trouble here. This is certainly unexpected behavior. It appears this is a similar class of problem as #652, so to consolidate efforts and discussions, I'm going to close this issue in preference of the existing one. Please follow (and potentially 👍 react to) that issue for further updates. |
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. |
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.
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.
SDK version
Relevant provider source code
Repo with a full provider + repro instructions: https://github.com/prashantv/terraform-set-null-repro
Relevant provider code:
https://github.com/prashantv/terraform-set-null-repro/blob/main/internal/provider/service_resource.go
Terraform Configuration Files
This configuration refers to a dummy resource that was created to repro the issue without any external dependencies.
See https://github.com/prashantv/terraform-set-null-repro for details.
To reproduce the issue, namespace is changed to some other value, and then applied. The apply results in an unexpected job set that contains 2 elements, where one is empty.
Debug Output
https://gist.github.com/prashantv/b7847ee5e35fa0167b7580cd10598faf
Expected Behavior
Apply to send the provider a single element in the "job" set.
Actual Behavior
Apply causes an Update that has 2 elements in the "job" set, where the first is empty, formatting the panic message:
Steps to Reproduce
To reproduce this issue, a repository is created with a specific dummy provider (that has no external dependencies), and with exact repro steps,
https://github.com/prashantv/terraform-set-null-repro
The custom provider is required, as this issue requires a specific setup:
References
Originally filed on the terraform repo, but directed to the SDK,
hashicorp/terraform#30581
Additional Information
In case it helps anyone else, we mitigated this issue by using a
TypeList
instead ofTypeSet
, so a field change doesn't result in the whole object changing, which seems to avoid the specific bug.The text was updated successfully, but these errors were encountered: