Skip to content
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/aws: aws_ami: handle deletion of AMIs #9721

Merged
merged 1 commit into from
Oct 31, 2016
Merged

provider/aws: aws_ami: handle deletion of AMIs #9721

merged 1 commit into from
Oct 31, 2016

Conversation

apparentlymart
Copy link
Contributor

Previously this resource (and, by extension, the aws_ami_copy and aws_ami_from_instance resources that share much of its implementation) was handling correctly the case where an AMI had been recently deregistered, and was thus still returned from the API, but not correctly dealing with the situation where the AMI has been removed altogether.

Now we additionally handle the NotFound error returned by the API when we request a non-existent AMI, and remove the AMI from the state in the same way we do for deregistered AMIs.

(I'm sorry to say that I've been sitting on this one for a while and just -targeting around the issues this was causing in my environment. 😞)

Previously this resource (and, by extension, the aws_ami_copy and
aws_ami_from_instance resources that share much of its implementation)
was handling correctly the case where an AMI had been recently
deregistered, and was thus still returned from the API, but not correctly
dealing with the situation where the AMI has been removed altogether.

Now we additionally handle the NotFound error returned by the API when
we request a non-existent AMI, and remove the AMI from the state in the
same way we do for deregistered AMIs.
@stack72
Copy link
Contributor

stack72 commented Oct 30, 2016

His looks good - would the addition of a _disappears style test prove that this works?

@apparentlymart
Copy link
Contributor Author

apparentlymart commented Oct 30, 2016

@stack72 It's not easy to test this in the usual way a _disappears test works, because an AMI doesn't get into this situation (not existing at all) for some time after it's deregistered; it hangs around in the "deregistered" state for a while first. (This was what caused this bug to not be noticed in the first place... I'd tested it manually and didn't realize that I was only hitting the "deregistered" codepath, and not the "doesn't exist at all" codepath.)

I tested this myself here by creating a Terraform state with an intentionally-incorrect AMI id in it and running terraform refresh. I could do a similar thing here but I'm not sure there's precedent for an acceptance test to create a "fake state" and then try to do operations on it, and not sure how that would fit in to the resource.Test framework. Are there any existing examples of this out there?

@stack72
Copy link
Contributor

stack72 commented Oct 30, 2016

@apparentlymart makes sense :) In that case, LGTM!

@stack72 stack72 merged commit ea0bc04 into hashicorp:master Oct 31, 2016
gusmat pushed a commit to gusmat/terraform that referenced this pull request Dec 6, 2016
Previously this resource (and, by extension, the aws_ami_copy and
aws_ami_from_instance resources that share much of its implementation)
was handling correctly the case where an AMI had been recently
deregistered, and was thus still returned from the API, but not correctly
dealing with the situation where the AMI has been removed altogether.

Now we additionally handle the NotFound error returned by the API when
we request a non-existent AMI, and remove the AMI from the state in the
same way we do for deregistered AMIs.
@ghost
Copy link

ghost commented Apr 20, 2020

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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Apr 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants