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

Added CIDR block set to datasource VPC peering #13420

Conversation

AlbertoSH
Copy link
Contributor

@AlbertoSH AlbertoSH commented May 20, 2020

Current data source aws_vpc_peering_connection only provides cidr_block and peer_cidr_block. This works fine for most scenarios in which you have two VPCs with just their primary CIDR, but when you attach secondary CIDRs, that info is missing

This PR adds both a cidr_block_set and a cidr_block_set attributes to the data source exposing those values

Community Note

  • Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

Release note for CHANGELOG:

- Added `cidr_block_set` & `peer_cidr_block_set` to `aws_vpc_peering_connection` data source.

Output from acceptance testing:

$ make testacc TEST=./aws TESTARGS='-run=TestAccDataSourceAwsVpcPeeringConnection'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccDataSourceAwsVpcPeeringConnection -timeout 120m
=== RUN   TestAccDataSourceAwsVpcPeeringConnection_basic
=== PAUSE TestAccDataSourceAwsVpcPeeringConnection_basic
=== RUN   TestAccDataSourceAwsVpcPeeringConnection_cidBlockSets
=== PAUSE TestAccDataSourceAwsVpcPeeringConnection_cidBlockSets
=== CONT  TestAccDataSourceAwsVpcPeeringConnection_basic
=== CONT  TestAccDataSourceAwsVpcPeeringConnection_cidBlockSets
--- PASS: TestAccDataSourceAwsVpcPeeringConnection_basic (69.24s)
--- PASS: TestAccDataSourceAwsVpcPeeringConnection_cidBlockSets (95.30s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	97.233s

@AlbertoSH AlbertoSH requested a review from a team May 20, 2020 07:41
@ghost ghost added size/M Managed by automation to categorize the size of a PR. needs-triage Waiting for first response or review from a maintainer. service/ec2 Issues and PRs that pertain to the ec2 service. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. labels May 20, 2020
@AlbertoSH
Copy link
Contributor Author

If you consider so, I can move the included checks into a new test so the existing one remains as simple as it was before

My Golang capabilities are quite limited, I did my best 🙃

@ewbankkit
Copy link
Contributor

@AlbertoSH Thanks for submitting this. The Go looks perfect 😄.
Yes, we prefer additional test(s) for new attributes to ensure that we introduce no regressions.
Also, please update the relevant documentation: website/docs/d/vpc.html.markdown.

@ghost ghost added the documentation Introduces or discusses updates to documentation. label May 20, 2020
@AlbertoSH
Copy link
Contributor Author

Hi @ewbankkit

I moved the checks to a new test & updated the docs. I left the existing test as it was and both tests are green (I updated the PR description with this new output)

$ make testacc TEST=./aws TESTARGS='-run=TestAccDataSourceAwsVpcPeeringConnection'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccDataSourceAwsVpcPeeringConnection -timeout 120m
=== RUN   TestAccDataSourceAwsVpcPeeringConnection_basic
=== PAUSE TestAccDataSourceAwsVpcPeeringConnection_basic
=== RUN   TestAccDataSourceAwsVpcPeeringConnection_cidBlockSets
=== PAUSE TestAccDataSourceAwsVpcPeeringConnection_cidBlockSets
=== CONT  TestAccDataSourceAwsVpcPeeringConnection_basic
=== CONT  TestAccDataSourceAwsVpcPeeringConnection_cidBlockSets
--- PASS: TestAccDataSourceAwsVpcPeeringConnection_basic (69.24s)
--- PASS: TestAccDataSourceAwsVpcPeeringConnection_cidBlockSets (95.30s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	97.233s

@ewbankkit
Copy link
Contributor

Verified acceptance tests:

$ make testacc TEST=./aws/ TESTARGS='-run=TestAccDataSourceAwsVpcPeeringConnection_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws/ -v -count 1 -parallel 20 -run=TestAccDataSourceAwsVpcPeeringConnection_ -timeout 120m
=== RUN   TestAccDataSourceAwsVpcPeeringConnection_basic
=== PAUSE TestAccDataSourceAwsVpcPeeringConnection_basic
=== RUN   TestAccDataSourceAwsVpcPeeringConnection_cidBlockSets
=== PAUSE TestAccDataSourceAwsVpcPeeringConnection_cidBlockSets
=== CONT  TestAccDataSourceAwsVpcPeeringConnection_basic
=== CONT  TestAccDataSourceAwsVpcPeeringConnection_cidBlockSets
--- PASS: TestAccDataSourceAwsVpcPeeringConnection_basic (41.27s)
--- PASS: TestAccDataSourceAwsVpcPeeringConnection_cidBlockSets (64.32s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	64.353s

@@ -41,6 +41,25 @@ func TestAccDataSourceAwsVpcPeeringConnection_basic(t *testing.T) {
})
}

func TestAccDataSourceAwsVpcPeeringConnection_cidBlockSets(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Rename to TestAccDataSourceAwsVpcPeeringConnection_cidrBlockSets().

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!
Renamed

@AlbertoSH
Copy link
Contributor Author

Hi @ewbankkit
Is there anything else needed to merge the PR? We (my team) would like to have this feature available in a regular provider release 🙂

@Amzani
Copy link

Amzani commented Jun 3, 2020

Hey guys,

Any updates on this.
We really need it.

@artem-timchenko
Copy link

Hi guys,
Any plans to merge this? This feature will be really helpful

@Amzani
Copy link

Amzani commented Aug 10, 2020

@ewbankkit when do you guys plan to merge this ?

@ewbankkit ewbankkit requested review from DrFaust92 and removed request for a team August 10, 2020 16:15
@ewbankkit
Copy link
Contributor

@AlbertoSH This is looking good. Can you please convert the new acceptance tests config to Terraform 0.12 syntax?

For example

"${aws_vpc.foo.id}"

to

aws_vpc.foo.id

@AlbertoSH
Copy link
Contributor Author

@ewbankkit There you go!
:)

@DrFaust92 DrFaust92 added enhancement Requests to existing resources that expand the functionality or scope. and removed needs-triage Waiting for first response or review from a maintainer. labels Oct 23, 2020
Copy link
Collaborator

@DrFaust92 DrFaust92 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

--- PASS: TestAccDataSourceAwsVpcPeeringConnection_basic (71.73s)
--- PASS: TestAccDataSourceAwsVpcPeeringConnection_cidrBlockSets (98.37s)

@bflad bflad self-assigned this Jan 22, 2021
Base automatically changed from master to main January 23, 2021 00:57
@breathingdust breathingdust requested a review from a team as a code owner January 23, 2021 00:57
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.

Hi @AlbertoSH 👋 Thank you for submitting this, overall this was looking good, just some minor changes we will complete on merge. 👍

@@ -40,6 +40,19 @@ func dataSourceAwsVpcPeeringConnection() *schema.Resource {
Optional: true,
Computed: true,
},
"cidr_block_set": {
Type: schema.TypeList,
Optional: true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the attribute is not used for configuring the data source, Optional should be removed. 👍

Suggested change
Optional: true,

@@ -60,6 +73,19 @@ func dataSourceAwsVpcPeeringConnection() *schema.Resource {
Optional: true,
Computed: true,
},
"peer_cidr_block_set": {
Type: schema.TypeList,
Optional: true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly:

Suggested change
Optional: true,

@@ -82,6 +82,10 @@ All of the argument attributes except `filter` are also exported as result attri
* `requester` - A configuration block that describes [VPC Peering Connection]
(https://docs.aws.amazon.com/vpc/latest/peering/what-is-vpc-peering.html) options set for the requester VPC.

* `cidr_block_set` - (Optional) The list of all CIDR blocks of the requester VPC of the specific VPC Peering Connection to retrieve.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* `cidr_block_set` - (Optional) The list of all CIDR blocks of the requester VPC of the specific VPC Peering Connection to retrieve.
* `cidr_block_set` - List of all CIDR blocks of the requester VPC.

@@ -82,6 +82,10 @@ All of the argument attributes except `filter` are also exported as result attri
* `requester` - A configuration block that describes [VPC Peering Connection]
(https://docs.aws.amazon.com/vpc/latest/peering/what-is-vpc-peering.html) options set for the requester VPC.

* `cidr_block_set` - (Optional) The list of all CIDR blocks of the requester VPC of the specific VPC Peering Connection to retrieve.

* `peer_cidr_block_set` - (Optional) The list of all CIDR blocks of the accepter VPC of the specific VPC Peering Connection to retrieve.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* `peer_cidr_block_set` - (Optional) The list of all CIDR blocks of the accepter VPC of the specific VPC Peering Connection to retrieve.
* `peer_cidr_block_set` - List of all CIDR blocks of the accepter VPC.

@bflad bflad merged commit bfe78e0 into hashicorp:main Jan 25, 2021
@github-actions github-actions bot added this to the v3.26.0 milestone Jan 25, 2021
@ghost
Copy link

ghost commented Jan 28, 2021

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

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template for triage. Thanks!

@ghost
Copy link

ghost commented Feb 24, 2021

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 as resolved and limited conversation to collaborators Feb 24, 2021
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.

7 participants