-
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
r/aws_{ami,ami_copy,ami_from_instance}: Configurable timeouts. #1811
r/aws_{ami,ami_copy,ami_from_instance}: Configurable timeouts. #1811
Conversation
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.
Hi @jmehnle
thanks for the contribution.
This looks reasonable - just two things to sort out before I click the green merge button:
- Maintainers always modify the Changelog as part of the merging process - this is mainly to avoid conflicts as PRs can sometimes go stale and Changelog changes very often.
- Do you mind passing just the bare minimum to all the waiter functions - i.e.
time.Duration
, not the wholeResourceData
with all fields?
i.e. instead of
resourceAwsAmiWaitForAvailable(d, id, client)
resourceAwsAmiWaitForAvailable(d.Timeout(schema. TimeoutCreate), id, client)
That way you can also remove the ResourceData mocking in the test.
|
…imeout directly rather than entire `ResourceData`.
Anything I can do to help this along and get it merged? |
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.
Thanks for the ping, and sorry for a small delay. This LGTM now.
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! |
Terraform 0.9.0 introduced a 40-minutes "create" timeout to the
aws_ami
,aws_ami_copy
,aws_ami_from_instance
resources. AWS instances with large disk volumes take a long time to snapshot, often more than 40 minutes.Terraform recently also introduced configurable timeouts, but this must be implemented on a per-resource basis. This diff implements configurable timeouts for the
aws_ami
,aws_ami_copy
,aws_ami_from_instance
resources.