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 'aws_vpc_peering_connection_accepter' resource #11505

Merged
merged 2 commits into from
Feb 9, 2017
Merged

provider/aws: Add 'aws_vpc_peering_connection_accepter' resource #11505

merged 2 commits into from
Feb 9, 2017

Conversation

ewbankkit
Copy link
Contributor

Initial commit from @BSick7:
Implementing vpc_peering_connection_accept.

Additions from @ewbankkit:
Rename 'aws_vpc_peering_connection_accept' to 'aws_vpc_peering_connection_accepter'.
Get it working reusing functionality from 'aws_vpc_peering_connection' resource.

Implementing vpc_peering_connection_accept.

Additions from @ewbankkit:
Rename 'aws_vpc_peering_connection_accept' to 'aws_vpc_peering_connection_accepter'.
Get it working reusing functionality from 'aws_vpc_peering_connection' resource.
@ewbankkit
Copy link
Contributor Author

Extends the work of @BSick7 in #6843.
aws_vpc_peering_connection_accepter seems like a more self-explanatory name for the resource.
By using pretty much the same schema as aws_vpc_peering_connection we can reuse much of that resource's functionality.
I have it working with 2 AWS accounts that I have access to.

Still TODO:

  • Tighten up documentation
  • Cross-account test cases - Is there prior art here or advice on how to do this in Terraform acceptance tests?

@ewbankkit
Copy link
Contributor Author

Currently leaving the DELETE functionality as just removing the resource from management.
VPC Peering Connections can be deleted from either side so we could do a Delete API call when this resource gets deleted but then the requester side would find the resource deleted from under it. Seems cleaner this way, but open to suggestions.

@ewbankkit
Copy link
Contributor Author

  • Tightened up documentation
  • Added same-account acceptance test (expected errors)
  • Modified CREATE so that read is done before update to ensure we don't go into a retry loop when querying tags with an invalid PCX ID

@ewbankkit
Copy link
Contributor Author

Still looking for guidance on how to do cross-account testing using the current acceptance testing functionality.

@ewbankkit
Copy link
Contributor Author

Being nitpicky but this should be tagged with new-resource, not new-data-source.

@catsby
Copy link
Contributor

catsby commented Feb 8, 2017

Hey @ewbankkit , can you help me understand a thing or two here?

  • I see we have gone against the suggestion of making this feature an attribute of aws_vpc_peering_connection; that's fine, but just want to double check that was intentional

  • I applied the infrastructure in the test file; everything worked as I expected! Is this pull request still a WIP then?

The multi-account testing is something we'll have to figure out internally. We have some ideas and I think the right CI in place to do it.

Let me know if this is still WIP, and if it depends on #11648 being addressed too. Thanks!

@ewbankkit
Copy link
Contributor Author

@catsby Yes, it was intentional to have a separate resource type.
I couldn't find any other AWS resources that either created the resource in AWS or adopted it into management depending on the value of an attribute. The accepter-side of a cross-account VPC Peering Connection is placed into the accepter's AWS account automatically and so it seems more like a VPC's default security group or default NACL (which have their own Terraform resource types).
This doesn't depend on #11648 - That's just me thinking through some inconsistencies.

@ewbankkit ewbankkit changed the title [WIP] provider/aws: Add 'aws_vpc_peering_connection_accepter' resource provider/aws: Add 'aws_vpc_peering_connection_accepter' resource Feb 9, 2017
@ewbankkit
Copy link
Contributor Author

Removed WIP.

@catsby
Copy link
Contributor

catsby commented Feb 9, 2017

Thanks for clarifying @ewbankkit – I'll do an final review and hope to merge today!

@catsby
Copy link
Contributor

catsby commented Feb 9, 2017

Pulling this in, thanks!
I'll work on getting a cross-account test setup in our CI. Thanks!

@ghost
Copy link

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

3 participants