-
Notifications
You must be signed in to change notification settings - Fork 232
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
helper/schema: Remove timeouts during ImportResourceState #1146
Conversation
Reference: #1145 Reference: hashicorp/terraform#32463 Terraform 1.3.8 and later now correctly handles null values for single nested blocks. This means Terraform will now report differences between a null block and known block with null values. This SDK only supported single nested blocks via its timeouts functionality. This change is a very targeted removal of any potential `timeouts` block values in a resource state from the `ImportResourceState` RPC. Since configuration is not available during that RPC, it is never valid to return any data beyond null for that block. This will prevent unexpected differences on the first plan after import, where Terraform will report the block removal for configurations which do not contain the block. New unit test failure prior to code updates: ``` --- FAIL: TestImportResourceState_Timeouts_Removed (0.00s) /Users/bflad/src/github.com/hashicorp/terraform-plugin-sdk/helper/schema/grpc_provider_test.go:1159: unexpected difference: cty.Value( - { - ty: cty.Type{typeImpl: cty.typeObject{AttrTypes: map[string]cty.Type{...}}}, - v: map[string]any{"id": string("test"), "string_attribute": nil, "timeouts": nil}, - }, + { + ty: cty.Type{typeImpl: cty.typeObject{AttrTypes: map[string]cty.Type{...}}}, + v: map[string]any{ + "id": string("test"), + "string_attribute": nil, + "timeouts": map[string]any{"create": nil, "read": nil}, + }, + }, ) ```
This cannot be verified via provider acceptance testing on Terraform 1.3.8 as $ terraform import aws_security_group.test sg-0329f3c7cf561ad15
aws_security_group.test: Importing from ID "sg-0329f3c7cf561ad15"...
aws_security_group.test: Import prepared!
Prepared aws_security_group for import
aws_security_group.test: Refreshing state... [id=sg-0329f3c7cf561ad15]
Import successful!
The resources that were imported are shown above. These resources are now in
your Terraform state and will henceforth be managed by Terraform.
$ terraform plan
╷
│ Warning: Provider development overrides are in effect
│
│ The following provider development overrides are set in the CLI configuration:
│ - hashicorp/aws in /Users/bflad/go/bin
│
│ The behavior may therefore not match any released version of the provider and applying changes may cause the state to become incompatible with published releases.
╵
aws_security_group.test: Refreshing state... [id=sg-0329f3c7cf561ad15]
No changes. Your infrastructure matches the configuration.
Terraform has compared your real infrastructure against your configuration and found no differences, so no changes are needed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I bet the timeouts are being added in HCL2ValueFromFlatmap
which doesn't make use of the schema, and NormalizeObjectFromLegacySDK
doesn't try to strip them out because it's a technically valid value. One of those functions should have been where this was done originally, but it probably doesn't matter so much at this point.
…eVerify Reference: #576 Reference: #1146 This change reverts #576 as it prevents acceptance testing from detecting Terraform 1.3.8 and later potentially returning an unexpected plan after import involving the `timeouts` block. #1146 fixes the import logic to never include a known value for `timeouts`, which is what the acceptance testing was trying to say when this testing logic was not in place. With this change, but without #1146 on a resource known to have the unexpected plan after import on Terraform 1.3.8: ``` === CONT TestAccVPCSecurityGroup_basic vpc_security_group_test.go:1002: Step 2/2 error running import: ImportStateVerify attributes not equivalent. Difference is shown below. The - symbol indicates attributes missing after import. map[string]string{ + "timeouts.%": "2", } --- FAIL: TestAccVPCSecurityGroup_basic (21.54s) ``` With this change and with #1146: ``` === CONT TestAccVPCSecurityGroup_basic --- PASS: TestAccVPCSecurityGroup_basic (20.23s) ``` Verified both ways on Terraform 1.2.9 as well (as a smoke test for a Terraform version prior to 1.3.8).
…eVerify Reference: hashicorp/terraform-plugin-sdk#1146 Reference: hashicorp/terraform-plugin-sdk#1147 This change reverts hashicorp/terraform-plugin-sdk#576 as it prevents acceptance testing from detecting Terraform 1.3.8 and later potentially returning an unexpected plan after import involving the `timeouts` block. hashicorp/terraform-plugin-sdk#1146 fixes the import logic to never include a known value for `timeouts`, which is what the acceptance testing was trying to say when this testing logic was not in place. With this change, but without hashicorp/terraform-plugin-sdk#1146 on a resource known to have the unexpected plan after import on Terraform 1.3.8: ``` === CONT TestAccVPCSecurityGroup_basic vpc_security_group_test.go:1002: Step 2/2 error running import: ImportStateVerify attributes not equivalent. Difference is shown below. The - symbol indicates attributes missing after import. map[string]string{ + "timeouts.%": "2", } --- FAIL: TestAccVPCSecurityGroup_basic (21.54s) ``` With this change and with hashicorp/terraform-plugin-sdk#1146: ``` === CONT TestAccVPCSecurityGroup_basic --- PASS: TestAccVPCSecurityGroup_basic (20.23s) ``` Verified both ways on Terraform 1.2.9 as well (as a smoke test for a Terraform version prior to 1.3.8).
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
Closes #1145
Reference: hashicorp/terraform#32463
Terraform 1.3.8 and later now correctly handles null values for single nested blocks. This means Terraform will now report differences between a null block and known block with null values. This SDK only supported single nested blocks via its timeouts functionality.
This change is a very targeted removal of any potential
timeouts
block values in a resource state from theImportResourceState
RPC. Since configuration is not available during that RPC, it is never valid to return any data beyond null for that block. This will prevent unexpected differences on the first plan after import, where Terraform will report the block removal for configurations which do not contain the block.New unit test failure prior to code updates: