service/s3: Add d.IsNewResource checks and standardize retry logic #18537
Conversation
Reference: #10587 Reference: #16796 Output from acceptance testing: ``` --- PASS: TestAccAWSS3BucketAnalyticsConfiguration_basic (38.97s) --- PASS: TestAccAWSS3BucketAnalyticsConfiguration_removed (56.16s) --- PASS: TestAccAWSS3BucketAnalyticsConfiguration_updateBasic (87.68s) --- PASS: TestAccAWSS3BucketAnalyticsConfiguration_WithFilter_Empty (12.16s) --- PASS: TestAccAWSS3BucketAnalyticsConfiguration_WithFilter_MultipleTags (60.16s) --- PASS: TestAccAWSS3BucketAnalyticsConfiguration_WithFilter_Prefix (60.97s) --- PASS: TestAccAWSS3BucketAnalyticsConfiguration_WithFilter_PrefixAndTags (60.97s) --- PASS: TestAccAWSS3BucketAnalyticsConfiguration_WithFilter_Remove (61.00s) --- PASS: TestAccAWSS3BucketAnalyticsConfiguration_WithFilter_SingleTag (60.97s) --- PASS: TestAccAWSS3BucketAnalyticsConfiguration_WithStorageClassAnalysis_Default (39.98s) --- PASS: TestAccAWSS3BucketAnalyticsConfiguration_WithStorageClassAnalysis_Empty (12.14s) --- PASS: TestAccAWSS3BucketAnalyticsConfiguration_WithStorageClassAnalysis_Full (39.60s) --- PASS: TestAccAWSS3BucketInventory_basic (26.27s) --- PASS: TestAccAWSS3BucketInventory_encryptWithSSES3 (26.34s) --- PASS: TestAccAWSS3BucketInventory_encryptWithSSEKMS (26.45s) --- PASS: TestAccAWSS3BucketMetric_basic (26.21s) --- PASS: TestAccAWSS3BucketMetric_WithEmptyFilter (2.25s) --- PASS: TestAccAWSS3BucketMetric_WithFilterMultipleTags (46.71s) --- PASS: TestAccAWSS3BucketMetric_WithFilterPrefix (45.51s) --- PASS: TestAccAWSS3BucketMetric_WithFilterPrefixAndMultipleTags (46.55s) --- PASS: TestAccAWSS3BucketMetric_WithFilterPrefixAndSingleTag (45.88s) --- PASS: TestAccAWSS3BucketMetric_WithFilterSingleTag (46.49s) --- PASS: TestAccAWSS3BucketNotification_LambdaFunction (45.26s) --- PASS: TestAccAWSS3BucketNotification_LambdaFunction_LambdaFunctionArn_Alias (40.24s) --- PASS: TestAccAWSS3BucketNotification_Queue (27.96s) --- PASS: TestAccAWSS3BucketNotification_Topic (28.53s) --- PASS: TestAccAWSS3BucketNotification_Topic_Multiple (29.23s) --- PASS: TestAccAWSS3BucketNotification_update (49.26s) --- PASS: TestAccAWSS3BucketPublicAccessBlock_basic (29.52s) --- PASS: TestAccAWSS3BucketPublicAccessBlock_BlockPublicAcls (68.84s) --- PASS: TestAccAWSS3BucketPublicAccessBlock_BlockPublicPolicy (68.21s) --- PASS: TestAccAWSS3BucketPublicAccessBlock_disappears (26.48s) --- PASS: TestAccAWSS3BucketPublicAccessBlock_disappears_Bucket (18.16s) --- PASS: TestAccAWSS3BucketPublicAccessBlock_IgnorePublicAcls (68.18s) --- PASS: TestAccAWSS3BucketPublicAccessBlock_RestrictPublicBuckets (68.83s) ```
bflad
added
the
technical-debt
ghost
added
size/L
| @@ -147,19 +148,23 @@ func resourceAwsS3BucketAnalyticsConfigurationPut(d *schema.ResourceData, meta i | |||
|
|
|||
| err := resource.Retry(1*time.Minute, func() *resource.RetryError { | |||
| _, err := s3conn.PutBucketAnalyticsConfiguration(input) | |||
|
|
|||
| if tfawserr.ErrCodeEquals(err, s3.ErrCodeNoSuchBucket) { | |||
There was a problem hiding this comment.
open q: why move this if-logic outside of the if err != nil check below? just curious b/c i've caught myself writing these types of checks w/in an if err != nil especially in some Delete CRUD ops of resources and i think my rationale at the time was that it didn't seem to make sense to pass the err around if it's nil but maybe that goes against our usual patterns...and i do like the visibility of the error check as you have now, rather than hiding within an if-block 😄
There was a problem hiding this comment.
Great question! It's something I picked up from https://medium.com/@matryer/line-of-sight-in-code-186dd7cdea88
In this case since it is the unhappy path being nested under the if err != nil, it is probably okay to be nested if we wanted. Prior to these AWS SDK Go error checking helpers it used to be that this logic had to be nested (or duplicate the err != nil check) for the type assertions, but now that this detail is tucked away, it this allows for either style. 👍
There was a problem hiding this comment.
Ohh very cool, thanks! def sounds familiar coming from ruby land (and Sandi Metz's principles)
aws/resource_aws_s3_bucket_metric.go
Outdated
| if err != nil { | ||
| return fmt.Errorf("Error putting S3 metric configuration: %s", err) | ||
| return fmt.Errorf("error putting S3 Bucket Metric Configuration: %w", err) |
There was a problem hiding this comment.
Nit: i noticed below Metrics is used instead of Metric so for consistency maybe use the plural here as well..unless there's an intended diff?
There was a problem hiding this comment.
Great catch. I will make them consistent to Metrics in a followup commit here. 👍 One day we'll create and use a Go type that will centralize all this information for error reporting, etc. 😍
&resource{
FriendlyName: "S3 Bucket Metrics Configuration",
// ...
}
…guration friendly name in logging and errors
|
This has been released in version 3.36.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! |
ghost
locked as resolved and limited conversation to collaborators
Community Note
Closes #10587
Reference: #16796
Output from acceptance testing: