Issue #3984 New Datasource: aws_vpcs #4736

saravanan30erd · 2018-06-04T07:23:22Z

New datasource aws_vpc_ids to discover all VPC IDs(Issue #3984 ).

saravanan30erd · 2018-06-04T18:06:54Z

$ make testacc TEST=./aws/ TESTARGS='-run=TestAccDataSourceAwsVpcIDs_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws/ -v -run=TestAccDataSourceAwsVpcIDs_ -timeout 120m
=== RUN TestAccDataSourceAwsVpcIDs_basic
--- PASS: TestAccDataSourceAwsVpcIDs_basic (81.57s)
=== RUN TestAccDataSourceAwsVpcIDs_tags
--- PASS: TestAccDataSourceAwsVpcIDs_tags (81.60s)
PASS
ok github.com/terraform-providers/terraform-provider-aws/aws 163.211s

bflad

Hi @saravanan30erd 👋 Thanks for submitting this! I left you some initial feedback below. Please take a look and let us know if you have any questions or do not have time to implement the feedback.

bflad · 2018-06-06T13:36:15Z

aws/data_source_aws_vpc_ids_test.go

+			{
+				Config: testAccDataSourceAwsVpcIDsConfig(),
+				Check: resource.ComposeTestCheckFunc(
+					//datasource aws_vpc_ids will list all the vpcs in a region, so it also includes the Default vpc.


It does not look like anything is reading the filter schema attribute into req.Filter, which might explain the behavior you're seeing here when you have an invalid filter in the test configuration. It looks like the valid value should be vpc-id according to the SDK documentation: https://docs.aws.amazon.com/sdk-for-go/api/service/ec2/#DescribeVpcsInput

For compatibility in all AWS accounts (which may or may not have other existing VPCs since you can delete the default VPC), I would recommend setting up the _basic test to not have any ids/filter/tags arguments, create 1 VPC resource, add depends_on in the data source to that VPC resource, then ensure that the value of the attribute is greater than 0. We cannot count on ids being "3".

@bflad I added the filter to req.Filter now.
Regarding acceptance tests, I am getting the plan empty error now.
testing.go:518: Step 0 error: After applying this step and refreshing, the plan was not empty:
what I am missing here?

If you turn on the debug logging, you should see the empty plan output. I generally don't show it during PR submission but I find it very helpful to run my acceptance testing like this:

rm -f aws/log.txt; TF_LOG_PATH=log.txt TF_LOG=debug make testacc TEST=./aws TESTARGS='-run=TestAccAWSXXX'

Which will turn on debug logging and write that output to a separate log file (aws/log.txt) that I can inspect.

@bflad In acceptance test _basic, getting the below error when I using depends_on in data source to VPC resource.
`--- FAIL: TestAccDataSourceAwsVpcs_basic (79.22s)
testing.go:518: Step 0 error: After applying this step and refreshing, the plan was not empty:

DIFF: CREATE: data.aws_vpcs.all ids.#: "" => "<computed>" tags.%: "" => "<computed>" STATE: aws_vpc.test-vpc: ID = vpc-954c44ec provider = provider.aws assign_generated_ipv6_cidr_block = false cidr_block = 10.0.0.0/24 default_network_acl_id = acl-61b72c19 default_route_table_id = rtb-5ff0e927 default_security_group_id = sg-29daa658 dhcp_options_id = dopt-f0038698 enable_classiclink = false enable_classiclink_dns_support = false enable_dns_hostnames = false enable_dns_support = true instance_tenancy = default main_route_table_id = rtb-5ff0e927 tags.% = 0

FAIL
FAIL github.com/terraform-providers/terraform-provider-aws/aws 79.264s`

After destroyed the vpc resource and data source and running the plan shows the drift due to depends_on (to vpc resource) in data source. In aws_vpc resource test data_source_aws_vpc_test.go, TestAccDataSourceAwsVpc_basic also fails with same error after adding depends_on in data source. So I am just checking data source id in _basic acceptance test here.

bflad · 2018-06-06T13:38:56Z

aws/data_source_aws_vpc_ids_test.go

+
+func testAccDataSourceAwsVpcIDsConfig() string {
+	return fmt.Sprintf(`
+	provider "aws" {


Declaring the "default" provider region is extraneous in test configurations and discouraged unless its absolutely necessary (e.g. EC2 Classic testing) -- its handled by the AWS_DEFAULT_REGION environment variable (or defaults to us-west-2) here:

https://github.com/terraform-providers/terraform-provider-aws/blob/59ea1c3e859c45a3a19d60b49b7c42fee88e5d7b/aws/provider_test.go#L69

bflad · 2018-06-06T13:40:25Z

aws/data_source_aws_vpc_ids_test.go

+	}
+
+	data "aws_vpc_ids" "selected" {
+		filter {


We should remove this in preference of only testing tags in this configuration 👍

bflad · 2018-06-06T13:42:08Z

aws/data_source_aws_vpc_ids.go

+	}
+
+	d.SetId(time.Now().UTC().String())
+	d.Set("ids", vpcs)


I would recommend adding error checking for setting a non-scalar attribute:

if err := d.Set("ids", vpcs); err != nil { return fmt.Errorf("error setting ids: %s", err) }

Done, can you please explain about non-scalar attribute?

Technically speaking, we should probably perform the error checking on all attributes during d.Set(), but its quite verbose code-wise and we have found nested and list/set attributes to be the most troublesome when it comes to ensuring that the type from the API matches the type defined in the schema (e.g. incorrectly trying to put []*string into a TypeList with TypeString elements). Generally its worse when there is more attribute nesting happening when you need to be very concerned about lining up []map[string]interface{} correctly.

When this type mismatch occurs, the behavior of Terraform core is to pass the configuration through into the state as-is from the configuration and not provide any feedback about this behavior unless the error check is in place. In the situation where we don't error check and provide incompatible type(s) into d.Set(), its not possible to detect drift from the API. It will also show as a plan difference after resource import since the state could not set the real value during the import read.

We have a flag in Terraform core to instead panic() when it runs into this situation (you can manually enable it today with TF_SCHEMA_PANIC_ON_ERROR=1 environment variable), but I'm not sure we'll ever flip it on given the current state of many of the providers and generally poorer user experience that provides even though it would certainly make these bugs way more obvious.

bflad · 2018-06-06T13:43:02Z

aws/data_source_aws_vpc_ids.go

+	vpcs := make([]string, 0)
+
+	for _, vpc := range resp.Vpcs {
+		vpcs = append(vpcs, *vpc.VpcId)


To prevent potential panics we might want to use the SDK helper function here: aws.StringValue(vpc.VpcId)

bflad · 2018-06-06T13:44:02Z

website/docs/d/vpc_ids.html.markdown

+    Provides a list of VPC Ids in a region
+---
+
+# Data Source: aws_vpc_ids


This documentation page is missing a sidebar link in website/aws.erb

bflad · 2018-06-06T13:46:34Z

website/docs/d/vpc_ids.html.markdown

+}
+```
+
+The primary use case would be interpolate the vpc_ids output into `count` of an aws_flow_log resource.


Nitpick: We generally try not to prescribe what might be a "primary" use case -- people use Terraform in all sorts of scenarios. I'd consider saying An example use case.

bflad · 2018-06-06T13:50:45Z

aws/data_source_aws_vpc_ids.go

+	return &schema.Resource{
+		Read: dataSourceAwsVpcIDsRead,
+		Schema: map[string]*schema.Schema{
+			"filter": dataSourceFiltersSchema(),


A few items with this attribute:

It does seem to be read into req.Filters so it appears to not currently be doing anything

Its missing in the data source documentation

I would recommend testing this attribute separately from the _basic test (preferably not being passed any arguments) and the _tags test (preferably only testing tags filtering).

Its my mistake, I added the filter in last minute when I was working on test cases, then I forgot to follow on.

bflad · 2018-06-06T13:52:16Z

website/docs/d/vpc_ids.html.markdown

+
+# Data Source: aws_vpc_ids
+
+`aws_vpc_ids` provides a list of vpc ids


Nitpick: This sentence seems redundant with the one below. Also, the casing of VPC IDs is different between various sentences in the documentation page.

bflad · 2018-06-06T13:55:33Z

aws/provider.go

@@ -242,6 +242,7 @@ func Provider() terraform.ResourceProvider {
 			"aws_ssm_parameter":                    dataSourceAwsSsmParameter(),
 			"aws_subnet":                           dataSourceAwsSubnet(),
 			"aws_subnet_ids":                       dataSourceAwsSubnetIDs(),
+			"aws_vpc_ids":                          dataSourceAwsVpcIDs(),


I'd recommend calling this data source aws_vpcs -- we may return more than IDs in the future. Unfortunately we have some more explicitly _ids named ones (like aws_subnet_ids above) that eventually may need to be deprecated should we need to return information other than IDs.

bflad

Thanks for the updates @saravanan30erd -- looks good! Let's get this in 🚀

3 tests passed (all tests)
=== RUN   TestAccDataSourceAwsVpcs_basic
--- PASS: TestAccDataSourceAwsVpcs_basic (9.17s)
=== RUN   TestAccDataSourceAwsVpcs_tags
--- PASS: TestAccDataSourceAwsVpcs_tags (9.47s)
=== RUN   TestAccDataSourceAwsVpcs_filters
--- PASS: TestAccDataSourceAwsVpcs_filters (9.51s)

bflad · 2018-06-13T14:54:56Z

aws/data_source_aws_vpcs_test.go

+	}
+}
+
+func testAccCheckAwsVpcsDataSourceID(n string) resource.TestCheckFunc {


Minor nitpick: We will have the documentation up very shortly (hashicorp/terraform-website#317 😉 ), but even when we're not checking for resource/datasource API object, we prefer to name these functions testAccCheckXXXExists for consistency. I'll adjust this on merge.

bflad · 2018-06-14T14:14:57Z

This has been released in version 1.23.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

ghost · 2020-04-05T17:34:54Z

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!

Add Data source to discover all VPC IDs

a584395

ghost added the size/L Managed by automation to categorize the size of a PR. label Jun 4, 2018

saravanan30erd mentioned this pull request Jun 4, 2018

Feature Request: Data source to discover all VPC IDs #3984

Closed

bflad added new-data-source Introduces a new data source. service/ec2 Issues and PRs that pertain to the ec2 service. labels Jun 4, 2018

ghost added the size/L Managed by automation to categorize the size of a PR. label Jun 4, 2018

bflad reviewed Jun 6, 2018

View reviewed changes

bflad added the waiting-response Maintainers are waiting on response from community or contributor. label Jun 6, 2018

ghost added size/L Managed by automation to categorize the size of a PR. labels Jun 6, 2018

saravanan30erd added 5 commits June 9, 2018 20:44

issue 3984 fix the state id

d932896

issue 3984 Add document for datasource aws_vpc_ids

f840a1f

issue 3984 add acceptance test

2c96494

PR hashicorp#4736 corrections based on feedback

9fa0f59

change aws_vpc_ids to aws_vpcs

20b3347

saravanan30erd force-pushed the issue-3984 branch from 8a5ae9b to 20b3347 Compare June 9, 2018 16:45

ghost added the size/L Managed by automation to categorize the size of a PR. label Jun 9, 2018

error handling in filters

35e0cd4

ghost added the size/L Managed by automation to categorize the size of a PR. label Jun 10, 2018

fix for basic acceptance test

5ef85f8

bflad removed the waiting-response Maintainers are waiting on response from community or contributor. label Jun 13, 2018

bflad changed the title ~~Issue #3984 New Datasource: aws_vpc_ids~~ Issue #3984 New Datasource: aws_vpcs Jun 13, 2018

bflad approved these changes Jun 13, 2018

View reviewed changes

bflad added this to the v1.23.0 milestone Jun 13, 2018

bflad merged commit 7f7effc into hashicorp:master Jun 13, 2018

bflad added a commit that referenced this pull request Jun 13, 2018

Update CHANGELOG for #4736

c0558a9

ghost locked and limited conversation to collaborators Apr 5, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #3984 New Datasource: aws_vpcs #4736

Issue #3984 New Datasource: aws_vpcs #4736

saravanan30erd commented Jun 4, 2018 •

edited

Loading

saravanan30erd commented Jun 4, 2018

bflad left a comment

bflad Jun 6, 2018

saravanan30erd Jun 6, 2018

bflad Jun 6, 2018

saravanan30erd Jun 10, 2018 •

edited

Loading

bflad Jun 6, 2018

bflad Jun 6, 2018

saravanan30erd Jun 6, 2018

bflad Jun 6, 2018

saravanan30erd Jun 6, 2018

bflad Jun 6, 2018

saravanan30erd Jun 6, 2018

bflad Jun 6, 2018

bflad Jun 6, 2018

bflad Jun 6, 2018

bflad Jun 6, 2018

saravanan30erd Jun 6, 2018

bflad Jun 6, 2018

saravanan30erd Jun 6, 2018

bflad Jun 6, 2018

bflad left a comment

bflad Jun 13, 2018

saravanan30erd Jun 13, 2018

bflad commented Jun 14, 2018

ghost commented Apr 5, 2020


		# Data Source: aws_vpc_ids

		`aws_vpc_ids` provides a list of vpc ids

Issue #3984 New Datasource: aws_vpcs #4736

Issue #3984 New Datasource: aws_vpcs #4736

Conversation

saravanan30erd commented Jun 4, 2018 • edited Loading

saravanan30erd commented Jun 4, 2018

bflad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saravanan30erd Jun 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bflad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bflad commented Jun 14, 2018

ghost commented Apr 5, 2020

saravanan30erd commented Jun 4, 2018 •

edited

Loading

saravanan30erd Jun 10, 2018 •

edited

Loading