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

Data Source: aws_ami_ids, add sort_ascending flag #5912

Merged
merged 1 commit into from
Sep 18, 2018

Conversation

sargun
Copy link
Contributor

@sargun sargun commented Sep 18, 2018

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.

Changes proposed in this pull request:

  • Add sort_ascending flag to data source ami_ids to optionally sort ascending. It will not

Output from acceptance testing:

$ make testacc TESTARGS="-run=TestAccDataSourceAwsAmiIds_sorted"
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -run=TestAccDataSourceAwsAmiIds_sorted -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccDataSourceAwsAmiIds_sorted
--- PASS: TestAccDataSourceAwsAmiIds_sorted (9.65s)
PASS

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.
@ghost ghost added size/M Managed by automation to categorize the size of a PR. documentation Introduces or discusses updates to documentation. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. labels Sep 18, 2018
@sargun
Copy link
Contributor Author

sargun commented Sep 18, 2018

Since adding a reverse function to Terraform is in purgatory (hashicorp/terraform#18887), I figured might as well add the capability natively here.

@sargun
Copy link
Contributor Author

sargun commented Sep 18, 2018

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.

@sargun
Copy link
Contributor Author

sargun commented Sep 18, 2018

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.

@bflad bflad added the service/ec2 Issues and PRs that pertain to the ec2 service. label Sep 18, 2018
@bflad bflad added this to the v1.37.0 milestone Sep 18, 2018
Copy link
Contributor

@bflad bflad left a 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)

@bflad bflad added the enhancement Requests to existing resources that expand the functionality or scope. label Sep 18, 2018
@bflad bflad merged commit 5eef605 into hashicorp:master Sep 18, 2018
bflad added a commit that referenced this pull request Sep 18, 2018
@sargun sargun deleted the add-sort-flag-to-ami-ids branch September 18, 2018 21:04
@bflad
Copy link
Contributor

bflad commented Sep 19, 2018

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.

@ghost
Copy link

ghost commented Apr 3, 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 feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

@ghost ghost locked and limited conversation to collaborators Apr 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation Introduces or discusses updates to documentation. enhancement Requests to existing resources that expand the functionality or scope. service/ec2 Issues and PRs that pertain to the ec2 service. size/M Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants