-
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
Data Source: aws_ami_ids, add sort_ascending flag #5912
Conversation
This adds the sort ascending flag by creation time to aws_ami_ids, to allow for a list which only ever adds items at the end. This is useful for example, when you have something like this: data "aws_ami_ids" "agent_amis" { owners = ["self"] filter { name = "name" values = ["foo-*"] } } data "aws_region" "current" {} resource "aws_ami_launch_permission" "launch_permissions_1234" { count = "${length(data.aws_ami_ids.agent_amis.ids)}" image_id = "${data.aws_ami_ids.agent_amis.ids[count.index]}" account_id = "1234" } Otherwise, every time a new AMI is added that matches "foo-*", it must "recreate" the launch permissions. Although this may be trivial for launch permissions, on something like AMI copy, it can wreak havoc. It also switches the acceptance test to use two AWS AMIs rather than baking our own.
Since adding a reverse function to Terraform is in purgatory (hashicorp/terraform#18887), I figured might as well add the capability natively here. |
This is the same kind of issue that's presented in #155, but because AMIs can be "captured" in time, it avoids this problem, as long as you never delete an AMI. |
Also, let me know if you want me to change the acceptance test back, or split that into a separate PR. Waiting for a VM to spin up, and for snapshotting is a pain for waiting for the test to pass, and AFAIK, Amazon Linux is a pretty reliable AMI, as in it hasn't been deleted since 2011. |
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 @sargun! 🚀
AWS Commercial:
3 tests passed (all tests)
--- PASS: TestAccDataSourceAwsAmiIds_basic (5.75s)
--- PASS: TestAccDataSourceAwsAmiIds_sorted (10.89s)
--- PASS: TestAccDataSourceAwsAmiIds_empty (14.25s)
AWS GovCloud (US):
3 tests passed (all tests)
--- PASS: TestAccDataSourceAwsAmiIds_basic (1.22s)
--- PASS: TestAccDataSourceAwsAmiIds_sorted (2.37s)
--- PASS: TestAccDataSourceAwsAmiIds_empty (3.03s)
This has been released in version 1.37.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! |
This adds the sort ascending flag by creation time to
aws_ami_ids, to allow for a list which only ever adds
items at the end.
This is useful for example, when you have something like this:
Otherwise, every time a new AMI is added that matches "foo-*",
it must "recreate" the launch permissions. Although this may
be trivial for launch permissions, on something like AMI
copy, it can wreak havoc.
Changes proposed in this pull request:
sort_ascending
flag to data sourceami_ids
to optionally sort ascending. It will notOutput from acceptance testing: