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

Type Block need handle attribute marked as removed contionally. #359

Open
wants to merge 2 commits into
base: v1-maint
Choose a base branch
from

Conversation

magodo
Copy link

@magodo magodo commented Mar 16, 2020

In some provider, e.g. AzureRM, TypeSet is use quite prevalent to
hold a set of sub-resources (e.g. security_rule in azurerm_network_security_group).
The reason for using TypeSet instead of TypeList might because service
return a collection of info with arbitrary order, or other reasons.

In this case: Given a slice of nested blocks, if one of the nested block (B1)
attributes is optional and has no default value, and user didn't specify a value
for it. Then if another nested block (B2) changed, which triggers some diff,
then B1 will also be replaced. That is because the optional attribute
contribute a diff of "zero value" -> null, which changed the hash of
the block.

This fix is to carefully handle this case. We keep attribute marked
as NewRemoved after a diffString only when all the attributes are
marked as such. Otherwise, as long as one attribute is not marked to be
removed, that means this block will be kept. Then we will manipulate the
attributes in this block, which being marked as removed just because
user didn't specify a value and the original value is the zero value.

This keeps consistent as other Resource types (e.g. List Resource).
(Though those type just remove the attribute from diff set, while for
Set we need to return the complete state as we will not depend on the
old state during diff apply).

Fixes:

@magodo
Copy link
Author

magodo commented Mar 16, 2020

This should fix hashicorp/terraform-provider-azurerm#5819.

@paultyng paultyng added the bug Something isn't working label Mar 20, 2020
@appilon
Copy link
Contributor

appilon commented Apr 23, 2020

We have recently pushed V2's history to master, V2 is now the mainline of development, I am retargeting your PR to v1-maint. Apologies for any inconvenience, for more information on V2 of the SDK, please see our discuss post

@appilon appilon changed the base branch from master to v1-maint April 23, 2020 18:24
@magodo magodo force-pushed the set_resource_manipulate_removed branch from c2ff9a0 to dc7dc61 Compare July 30, 2020 09:48
In some provider, e.g. AzureRM, TypeSet is use quite prevalent to
hold a set of sub-resources (e.g. security_rule in azurerm_network_security_group).
The reason for using TypeSet instead of TypeList might because service
return a collection of info with arbitrary order, or other reasons.

In this case: Given a slice of nested blocks, if one of the nested block (B1)
attributes is optional and has no default value, and user didn't specify a value
for it. Then if another nested block (B2) changed, which triggers some diff,
then B1 will also be replaced. That is because the optional attribute
contribute a diff of "zero value" -> `null`, which changed the hash of
the block.

This fix is to carefully handle this case. We keep attribute marked
as `NewRemoved` after a `diffString` only when all the attributes are
marked as such. Otherwise, as long as one attribute is not marked to be
removed, that means this block will be kept. Then we will manipulate the
attributes in this block, which being marked as removed just because
user didn't specify a value and the original value is the zero value.

This keeps consistent as other Resource types (e.g. List Resource).
(Though those type just remove the attribute from diff set, while for
Set we need to return the complete state as we will not depend on the
old state during diff apply).
@magodo magodo force-pushed the set_resource_manipulate_removed branch from dc7dc61 to b6e3d58 Compare July 30, 2020 09:53
The state representation will only store the primary values, hence
we only need to take care of those.
@hashicorp-cla
Copy link

hashicorp-cla commented Mar 12, 2022

CLA assistant check
All committers have signed the CLA.

@bflad bflad self-assigned this Mar 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants