-
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
resource/ebs_snapshot: fix crash in case of manual deletion #3462
Conversation
ca19076
to
888e118
Compare
The error code change LGTM according to the EC2 API documentation. Can you add an acceptance test that fails then passes to prevent this in the future and verify your fix? e.g. {
Config: //basic
}
{
PreConfig: func() {
// SDK call to DeleteSnapshot
},
Config: // basic
} We may also want to have an additional check to remove the ID from the state and return nil if |
aws/resource_aws_ebs_snapshot.go
Outdated
return nil | ||
if err != nil { | ||
if isAWSErr(err, "InvalidSnapshot.NotFound", "") { | ||
log.Printf("Snapshot %q Not found - removing from state", d.Id()) |
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.
Nitpick: We tend to prefix such messages with [WARN]
so it's easy to find them in the log.
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.
of coz
a892910
to
672f603
Compare
|
anything else to change before merge? |
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.
LGTM! Thanks so much! 🚀
=== RUN TestAccAWSEBSSnapshot_withDescription
--- PASS: TestAccAWSEBSSnapshot_withDescription (33.29s)
=== RUN TestAccAWSEBSSnapshot_withKms
--- PASS: TestAccAWSEBSSnapshot_withKms (53.12s)
=== RUN TestAccAWSEBSSnapshot_basic
--- PASS: TestAccAWSEBSSnapshot_basic (84.94s)
} | ||
} | ||
|
||
resource "aws_ebs_volume" "test" { | ||
availability_zone = "us-west-2a" | ||
availability_zone = "${data.aws_region.current.name}a" |
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.
Minor nitpick: Not all regions have/will have AZ a
available but this is certainly an improvement (data.aws_availability_zones.current[0].id
is safer)
This has been released in version 1.11.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. |
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! |
Fix #3460
Also enhance error checking in case of any other error.