Add check destroy for aws ebs snapshot copy resource & Fix error handling #9106

teraken0509 · 2019-06-24T15:57:11Z

Community Note

Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
Please do not leave "+1" comments, they generate extra noise for pull request followers and do not help prioritize the request

References #8958

Release note for CHANGELOG:

BUG-FIX:
- resource/aws_ebs_snapshot_copy: fix error handling when resource is gone

Output from acceptance testing:

$ make testacc TESTARGS="-run=TestAccAWSEbsSnapshotCopy_"
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -parallel 20 -run=TestAccAWSEbsSnapshotCopy_ -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAWSEbsSnapshotCopy_basic
=== PAUSE TestAccAWSEbsSnapshotCopy_basic
=== RUN   TestAccAWSEbsSnapshotCopy_withDescription
=== PAUSE TestAccAWSEbsSnapshotCopy_withDescription
=== RUN   TestAccAWSEbsSnapshotCopy_withRegions
=== PAUSE TestAccAWSEbsSnapshotCopy_withRegions
=== RUN   TestAccAWSEbsSnapshotCopy_withKms
=== PAUSE TestAccAWSEbsSnapshotCopy_withKms
=== RUN   TestAccAWSEbsSnapshotCopy_disappears
=== PAUSE TestAccAWSEbsSnapshotCopy_disappears
=== CONT  TestAccAWSEbsSnapshotCopy_basic
=== CONT  TestAccAWSEbsSnapshotCopy_disappears
=== CONT  TestAccAWSEbsSnapshotCopy_withKms
=== CONT  TestAccAWSEbsSnapshotCopy_withDescription
=== CONT  TestAccAWSEbsSnapshotCopy_withRegions
--- PASS: TestAccAWSEbsSnapshotCopy_disappears (73.88s)
--- PASS: TestAccAWSEbsSnapshotCopy_withDescription (74.98s)
--- PASS: TestAccAWSEbsSnapshotCopy_withKms (101.45s)
--- PASS: TestAccAWSEbsSnapshotCopy_basic (105.72s)
--- PASS: TestAccAWSEbsSnapshotCopy_withRegions (109.75s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	109.851s

Add acctest for when resource disappears

bflad

Thanks for these fixes, @kterada0509 👍 The EC2 error code fix here looks good according to https://docs.aws.amazon.com/AWSEC2/latest/APIReference/errors-overview.html

Left one blocking feedback item concerning the usage of return nil in the new CheckDestroy function and some preferred simplifications to use isAWSErr() instead of awserr directly. Please let us know if you have any questions or do not have time to implement the feedback items.

bflad · 2019-06-25T01:47:50Z

aws/resource_aws_ebs_snapshot_copy.go

@@ -116,7 +116,7 @@ func resourceAwsEbsSnapshotCopyRead(d *schema.ResourceData, meta interface{}) er
 		SnapshotIds: []*string{aws.String(d.Id())},
 	}
 	res, err := conn.DescribeSnapshots(req)
-	if ec2err, ok := err.(awserr.Error); ok && ec2err.Code() == "InvalidSnapshotID.NotFound" {
+	if ec2err, ok := err.(awserr.Error); ok && ec2err.Code() == "InvalidSnapshot.NotFound" {


These can be simplified using isAWSErr(err, CODE, MESSAGE) in the future, e.g.

Suggested change

if ec2err, ok := err.(awserr.Error); ok && ec2err.Code() == "InvalidSnapshot.NotFound" {

if isAWSErr(err, "InvalidSnapshot.NotFound", "") {

bflad · 2019-06-25T01:48:24Z

aws/resource_aws_ebs_snapshot_copy.go

@@ -153,10 +153,14 @@ func resourceAwsEbsSnapshotCopyDelete(d *schema.ResourceData, meta interface{})
 		}

 		ebsErr, ok := err.(awserr.Error)
-		if ebsErr.Code() == "SnapshotInUse" {
+		if ok && ebsErr.Code() == "SnapshotInUse" {


Leaving as non-blocking feedback for next time.

Suggested change

if ok && ebsErr.Code() == "SnapshotInUse" {

if isAWSErr(err, "SnapshotInUse", "") {

bflad · 2019-06-25T01:48:40Z

aws/resource_aws_ebs_snapshot_copy.go

 			return resource.RetryableError(fmt.Errorf("EBS SnapshotInUse - trying again while it detaches"))
 		}

+		if ok && ebsErr.Code() == "InvalidSnapshot.NotFound" {


Suggested change

if ok && ebsErr.Code() == "InvalidSnapshot.NotFound" {

if isAWSErr(err, "InvalidSnapshot.NotFound", "") {

bflad · 2019-06-25T01:49:05Z

aws/resource_aws_ebs_snapshot_copy_test.go

+			SnapshotIds: []*string{aws.String(rs.Primary.ID)},
+		})
+
+		if ec2err, ok := err.(awserr.Error); ok && ec2err.Code() == "InvalidSnapshot.NotFound" {


Suggested change

if ec2err, ok := err.(awserr.Error); ok && ec2err.Code() == "InvalidSnapshot.NotFound" {

if isAWSErr(err, "InvalidSnapshot.NotFound", "") {

bflad · 2019-06-25T01:49:52Z

aws/resource_aws_ebs_snapshot_copy_test.go

+		})
+
+		if ec2err, ok := err.(awserr.Error); ok && ec2err.Code() == "InvalidSnapshot.NotFound" {
+			return nil


CheckDestroy should not use return nil in the for loop, to ensure all resources are properly checked. 👍

Suggested change

return nil

continue

teraken0509 · 2019-06-25T02:49:51Z

Re-run acctest.

$ make testacc TESTARGS="-run=TestAccAWSEbsSnapshotCopy_"
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -parallel 20 -run=TestAccAWSEbsSnapshotCopy_ -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAWSEbsSnapshotCopy_basic
=== PAUSE TestAccAWSEbsSnapshotCopy_basic
=== RUN   TestAccAWSEbsSnapshotCopy_withDescription
=== PAUSE TestAccAWSEbsSnapshotCopy_withDescription
=== RUN   TestAccAWSEbsSnapshotCopy_withRegions
=== PAUSE TestAccAWSEbsSnapshotCopy_withRegions
=== RUN   TestAccAWSEbsSnapshotCopy_withKms
=== PAUSE TestAccAWSEbsSnapshotCopy_withKms
=== RUN   TestAccAWSEbsSnapshotCopy_disappears
=== PAUSE TestAccAWSEbsSnapshotCopy_disappears
=== CONT  TestAccAWSEbsSnapshotCopy_basic
=== CONT  TestAccAWSEbsSnapshotCopy_withRegions
=== CONT  TestAccAWSEbsSnapshotCopy_withKms
=== CONT  TestAccAWSEbsSnapshotCopy_withDescription
=== CONT  TestAccAWSEbsSnapshotCopy_disappears
--- PASS: TestAccAWSEbsSnapshotCopy_withDescription (83.19s)
--- PASS: TestAccAWSEbsSnapshotCopy_basic (83.71s)
--- PASS: TestAccAWSEbsSnapshotCopy_disappears (98.24s)
--- PASS: TestAccAWSEbsSnapshotCopy_withRegions (101.98s)
--- PASS: TestAccAWSEbsSnapshotCopy_withKms (125.85s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	125.991s

bflad

Looks great, thanks, @kterada0509! 🚀

--- PASS: TestAccAWSEbsSnapshotCopy_basic (49.04s)
--- PASS: TestAccAWSEbsSnapshotCopy_disappears (64.13s)
--- PASS: TestAccAWSEbsSnapshotCopy_withDescription (64.40s)
--- PASS: TestAccAWSEbsSnapshotCopy_withKms (82.87s)
--- PASS: TestAccAWSEbsSnapshotCopy_withRegions (99.52s)

bflad · 2019-06-28T01:49:04Z

This has been released in version 2.17.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!

ghost · 2019-11-03T15:06:50Z

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!

teraken0509 added 2 commits June 25, 2019 00:41

Add CheckDestroy for aws_ebs_snapshot_copy resource

b5888b8

fix error handling when resource NotFound

b49cd80

Add acctest for when resource disappears

teraken0509 requested a review from a team June 24, 2019 15:57

ghost added size/M Managed by automation to categorize the size of a PR. service/ec2 Issues and PRs that pertain to the ec2 service. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. labels Jun 24, 2019

bflad suggested changes Jun 25, 2019

View reviewed changes

bflad self-assigned this Jun 25, 2019

bflad added bug Addresses a defect in current functionality. waiting-response Maintainers are waiting on response from community or contributor. labels Jun 25, 2019

bflad added this to the v2.17.0 milestone Jun 25, 2019

fix to use isAWSErr

874e12f

ghost removed the waiting-response Maintainers are waiting on response from community or contributor. label Jun 25, 2019

bflad approved these changes Jun 25, 2019

View reviewed changes

bflad merged commit 941a12c into hashicorp:master Jun 25, 2019

bflad added a commit that referenced this pull request Jun 25, 2019

Update CHANGELOG for #9106

5e66b29

This was referenced Jul 11, 2019

Breaking change for aws_dx_gateway_association vpn_gateway_id parameter #8773

Closed

DirectConnectClientException","message":"Association ID has an invalid format #9317

Closed

ghost locked and limited conversation to collaborators Nov 3, 2019

teraken0509 deleted the feature/add-check-destroy-for-aws_ebs_snapshot_copy-resource branch March 5, 2020 14:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add check destroy for aws ebs snapshot copy resource & Fix error handling #9106

Add check destroy for aws ebs snapshot copy resource & Fix error handling #9106

teraken0509 commented Jun 24, 2019

bflad left a comment •

edited

Loading

bflad Jun 25, 2019

teraken0509 Jun 25, 2019

bflad Jun 25, 2019

teraken0509 Jun 25, 2019

bflad Jun 25, 2019

teraken0509 Jun 25, 2019

bflad Jun 25, 2019

teraken0509 Jun 25, 2019

bflad Jun 25, 2019

teraken0509 Jun 25, 2019

teraken0509 commented Jun 25, 2019

bflad left a comment

bflad commented Jun 28, 2019

ghost commented Nov 3, 2019

	if ec2err, ok := err.(awserr.Error); ok && ec2err.Code() == "InvalidSnapshot.NotFound" {
	if isAWSErr(err, "InvalidSnapshot.NotFound", "") {

	if ok && ebsErr.Code() == "SnapshotInUse" {
	if isAWSErr(err, "SnapshotInUse", "") {

	if ok && ebsErr.Code() == "InvalidSnapshot.NotFound" {
	if isAWSErr(err, "InvalidSnapshot.NotFound", "") {

Add check destroy for aws ebs snapshot copy resource & Fix error handling #9106

Add check destroy for aws ebs snapshot copy resource & Fix error handling #9106

Conversation

teraken0509 commented Jun 24, 2019

Community Note

bflad left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teraken0509 commented Jun 25, 2019

bflad left a comment

Choose a reason for hiding this comment

bflad commented Jun 28, 2019

ghost commented Nov 3, 2019

bflad left a comment •

edited

Loading