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 Add support for aws_subnet_ids as a data source #13188

Merged

Conversation

cwood
Copy link
Contributor

@cwood cwood commented Mar 30, 2017

Fixes #9843

@apparentlymart
Copy link
Contributor

Hi @cwood! Thanks for working on this.

We didn't initially implement these "plural" data sources because Terraform's core currently makes it hard to wrangle lists of complex objects like this. This is why, for example, the aws_availability_zones data source returns a list of names rather than a list of complex objects.

I certainly agree that being able to list subnets matching constraints would be useful, but I'm not sure that this particular design would be very usable with today's language limitations. For example, there isn't a good way to express "all of the id attribute values from the members of this list" in the interpolation language.

Perhaps in the short term it'd be useful to have an aws_subnet_ids resource that, similarly to aws_availability_zones, just returns a list of strings that are the ids of the matched subnet, which could then be used with the existing aws_subnet data source to get the result I think you were looking for here:

data "aws_subnet_ids" "example" {
  vpc_id = "${var.vpc_id}"
}

data "aws_subnet" "example" {
  count = "${length(data.aws_subnet_ids.example.ids)}"
  id = "${aws_subnet_ids.example.ids[count.index]}"
}

output "subnet_cidr_blocks" {
  value = ["${data.aws_subnet.example.*.cidr_block}"]
}

What do you think?

@cwood
Copy link
Contributor Author

cwood commented Mar 30, 2017

That's honestly fine with me. I only want subnet_ids anyway and to use a vpc_id

@Derek-Ashmore
Copy link

++1 -- Thanks for considering this. The lack of either aws_subnets or aws_subnet_ids is causing a lot of code bloat for me.

@apparentlymart
Copy link
Contributor

Thanks for the update @cwood. Looking good now!

The only other thing (which I'm sorry I didn't catch on the first pass) is that we should probably have an acceptance test for this. Acceptance tests for data sources can be kinda weird, but I think we could get away here with something that applies a config that creates a VPC and a couple subnets and then uses your data source to retrieve the subnets from that VPC and verifies that it gets back the two subnet ids we expect.

(It looks like Travis caught some compile-time issues, too...)

@cwood
Copy link
Contributor Author

cwood commented Mar 30, 2017

I am working on the compiling issues right now. Ill work on an acceptance test later. Is there a way I can run only the acceptance test for this data source and not the whole collection.

@apparentlymart
Copy link
Contributor

There's some long details on this here:
https://github.com/hashicorp/terraform/blob/master/.github/CONTRIBUTING.md#running-an-acceptance-test

...but in summary: you can use the TESTARGS variable to set the -run argument on the test runner, which allows you to run only the tests whose names match the given regular expression. Usually when running acceptance tests we'll run specifically the test we're working on, since acceptance tests are often relatively slow.

@cwood cwood force-pushed the cwood/new-resource-filter-subnets branch from 42262b0 to 4215b25 Compare March 30, 2017 17:21
@cwood cwood changed the title Add support for aws_subnets as a data source provider/aws Add support for aws_subnet_ids as a data source Mar 30, 2017
@cwood cwood force-pushed the cwood/new-resource-filter-subnets branch 2 times, most recently from cc78f0b to e41e262 Compare March 30, 2017 21:14
@cwood cwood force-pushed the cwood/new-resource-filter-subnets branch from e41e262 to d7a319f Compare March 31, 2017 17:57
@apparentlymart apparentlymart self-requested a review April 3, 2017 18:45
@apparentlymart apparentlymart merged commit 6380384 into hashicorp:master Apr 3, 2017
@ghost
Copy link

ghost commented Apr 14, 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 14, 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.

provider/aws: Subnets Data Source
3 participants