-
Notifications
You must be signed in to change notification settings - Fork 9.2k
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
provider: Fix d.Set() calls with invalid *time.Time value input #11491
Conversation
Reference: #9954 Previously, these attributes were incorrectly trying to use `*time.Time` as the value input to `d.Set()`. These will return errors if error checking is performed, however our preferred convention of not performing error checking for "simple" attributes means that these silently would skip being added to the Terraform state with the refreshed API value. The `tfproviderlint` `R004` check caught these previously though (which we would like to enable in the future but cannot until these and other failures are addressed): ``` /Users/bflad/src/github.com/terraform-providers/terraform-provider-aws/aws/data_source_aws_acmpca_certificate_authority.go:116:21: R004: ResourceData.Set() incompatible value type: *time.Time /Users/bflad/src/github.com/terraform-providers/terraform-provider-aws/aws/data_source_aws_acmpca_certificate_authority.go:117:22: R004: ResourceData.Set() incompatible value type: *time.Time /Users/bflad/src/github.com/terraform-providers/terraform-provider-aws/aws/resource_aws_acmpca_certificate_authority.go:362:21: R004: ResourceData.Set() incompatible value type: *time.Time /Users/bflad/src/github.com/terraform-providers/terraform-provider-aws/aws/resource_aws_acmpca_certificate_authority.go:363:22: R004: ResourceData.Set() incompatible value type: *time.Time /Users/bflad/src/github.com/terraform-providers/terraform-provider-aws/aws/resource_aws_emr_security_configuration.go:96:25: R004: ResourceData.Set() incompatible value type: *time.Time /Users/bflad/src/github.com/terraform-providers/terraform-provider-aws/aws/resource_aws_iam_service_linked_role.go:140:23: R004: ResourceData.Set() incompatible value type: *time.Time /Users/bflad/src/github.com/terraform-providers/terraform-provider-aws/aws/resource_aws_lambda_event_source_mapping.go:224:25: R004: ResourceData.Set() incompatible value type: *time.Time /Users/bflad/src/github.com/terraform-providers/terraform-provider-aws/aws/resource_aws_organizations_account.go:228:28: R004: ResourceData.Set() incompatible value type: *time.Time /Users/bflad/src/github.com/terraform-providers/terraform-provider-aws/aws/resource_aws_ssm_activation.go:159:27: R004: ResourceData.Set() incompatible value type: *time.Time /Users/bflad/src/github.com/terraform-providers/terraform-provider-aws/aws/resource_aws_ssm_document.go:243:24: R004: ResourceData.Set() incompatible value type: *time.Time ``` Here we update each of these `d.Set()` calls with the appropriate RFC3339 timestamp value (our preferred format in lieu of other API standards). The additional changes for `aws_ssm_activation` are to handle the API automatically setting a default value of 24 hours in advance. We do not want Terraform to return a difference in cases where the configuration does not have a value. Output from acceptance testing: ``` --- PASS: TestAccDataSourceAwsAcmpcaCertificateAuthority_Basic (29.17s) --- PASS: TestAccAwsAcmpcaCertificateAuthority_Basic (23.49s) --- PASS: TestAccAWSEmrSecurityConfiguration_basic (18.22s) --- PASS: TestAccAWSIAMServiceLinkedRole_basic (23.39s) --- PASS: TestAccAWSLambdaEventSourceMapping_kinesis_basic (85.92s) --- PASS: TestAccAWSLambdaEventSourceMapping_sqs_basic (134.73s) --- PASS: TestAccAWSSSMActivation_expirationDate (24.95s) --- PASS: TestAccAWSSSMActivation_basic (29.94s) --- PASS: TestAccAWSSSMDocument_basic (21.52s) ```
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.
🎉
--- PASS: TestAccDataSourceAwsAcmpcaCertificateAuthority_Basic (26.94s)
--- PASS: TestAccAwsAcmpcaCertificateAuthority_Basic (22.13s)
--- PASS: TestAccAWSEmrSecurityConfiguration_basic (21.30s)
--- PASS: TestAccAWSIAMServiceLinkedRole_basic (34.00s)
--- PASS: TestAccAWSLambdaEventSourceMapping_kinesis_basic (77.03s)
--- PASS: TestAccAWSLambdaEventSourceMapping_sqs_basic (89.11s)
--- PASS: TestAccAWSSSMActivation_expirationDate (29.31s)
--- PASS: TestAccAWSSSMActivation_basic (29.68s)
--- PASS: TestAccAWSSSMDocument_basic (8.51s)
@@ -17,6 +17,8 @@ import ( | |||
"github.com/hashicorp/terraform-plugin-sdk/terraform" | |||
) | |||
|
|||
const rfc3339RegexPattern = `^[0-9]{4}-(0[1-9]|1[012])-(0[1-9]|[12][0-9]|3[01])[Tt]([01][0-9]|2[0-3]):[0-5][0-9]:[0-5][0-9](\.[0-9]+)?([Zz]|([+-]([01][0-9]|2[0-3]):[0-5][0-9]))$` |
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.
🙀 🙂
This has been released in version 2.46.0 of the Terraform AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template for triage. Thanks! |
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 feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks! |
Community Note
Reference: #9954
Release note for CHANGELOG:
Previously, these attributes were incorrectly trying to use
*time.Time
as the value input tod.Set()
. These will return errors if error checking is performed, however our preferred convention of not performing error checking for "simple" attributes means that these silently would skip being added to the Terraform state with the refreshed API value.The
tfproviderlint
R004
check caught these previously though (which we would like to enable in the future but cannot until these and other failures are addressed):Here we update each of these
d.Set()
calls with the appropriate RFC3339 timestamp value, which is our preferred format in lieu of other API standards.The additional change for
aws_ssm_activation
is to handle the API automatically setting a default value of 24 hours in advance. Since Terraform was not able to properly refresh the state with the API value previously, it would never perform drift detection. We do not want Terraform to return a difference in cases where the configuration does not have a value, so we mark the attribute withComputed: true
.Output from acceptance testing: