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

HCPE-830 - Add TGW attachment resource #58

Merged
merged 17 commits into from
Feb 11, 2021
Merged

HCPE-830 - Add TGW attachment resource #58

merged 17 commits into from
Feb 11, 2021

Conversation

roaks3
Copy link
Contributor

@roaks3 roaks3 commented Feb 4, 2021

Ticket: https://hashicorp.atlassian.net/browse/HCPE-830

Adds the hcp_aws_transit_gateway_attachment resource and data source. The implementation is largely pulled from the initial implementation: https://github.com/hashicorp/cloud-terraform-provider-hcp-internal/pull/8

The inclusion of aws in the resource name came from some discussion captured here, which might change moving forward.

A simple example of what the resource looks like:

resource "hcp_aws_transit_gateway_attachment" "example" {
  hvn_id                        = hcp_hvn.main.hvn_id
  transit_gateway_attachment_id = "example-tgw-attachment"
  transit_gateway_id            = aws_ec2_transit_gateway.example.id
  resource_share_arn            = aws_ram_resource_share.example.arn
  destination_cidrs             = [aws_vpc.example.cidr_block]
}

To Test

The example included here can be used to test the functionality of this resource. Note that the AWS config cannot be the same account as the HVN.

@roaks3 roaks3 marked this pull request as draft February 4, 2021 00:59
@roaks3 roaks3 force-pushed the HCPE-830-tgw-resource branch 2 times, most recently from 7dd4fa5 to ab742b7 Compare February 8, 2021 22:03
@roaks3 roaks3 changed the title (WIP) HCPE-830 - Add TGW resource HCPE-830 - Add TGW attachment resource Feb 9, 2021
@@ -0,0 +1,51 @@
---
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The docs/* files included here were generated with go generate, which pulls in the examples and resource/field descriptions.

@roaks3 roaks3 marked this pull request as ready for review February 9, 2021 17:07
@xargs-P xargs-P self-requested a review February 9, 2021 17:22
@zackiles zackiles requested a review from nombiezinja February 9, 2021 18:22
Copy link
Contributor

@bcmdarroch bcmdarroch left a comment

Choose a reason for hiding this comment

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

Had some questions and a suggestion for the new provider_account_id HVN output, but otherwise nice work! ✨

docs/data-sources/aws_transit_gateway_attachment.md Outdated Show resolved Hide resolved
@@ -89,6 +89,11 @@ func resourceHvn() *schema.Resource {
Type: schema.TypeString,
Computed: true,
},
"provider_account_id": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's save this for a follow-up, but I think this is a good point to introduce the pattern for returning cloud-specific outputs. I'm thinking we'll have an output with the top-level name aws, which contains account_id, similar to this vpc_config object in the the AWS provider's eks_cluster resource. It's a bit of a hack, but it sounds like it's the best we've got while the provider SDK team works out a better solution for input/output objects.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea, I think this would warrant some more discussion, so my preference would be to include that in the general topic around cloud-specific values in our network resources. Definitely agree that this field falls into that category.

internal/clients/tgw.go Outdated Show resolved Hide resolved
internal/clients/tgw.go Outdated Show resolved Hide resolved
Copy link
Contributor

@smaant smaant left a comment

Choose a reason for hiding this comment

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

A few comments

docs/data-sources/aws_transit_gateway_attachment.md Outdated Show resolved Hide resolved
docs/resources/aws_transit_gateway_attachment.md Outdated Show resolved Hide resolved
docs/resources/aws_transit_gateway_attachment.md Outdated Show resolved Hide resolved
docs/resources/aws_transit_gateway_attachment.md Outdated Show resolved Hide resolved
docs/data-sources/aws_transit_gateway_attachment.md Outdated Show resolved Hide resolved
docs/data-sources/aws_transit_gateway_attachment.md Outdated Show resolved Hide resolved
@roaks3 roaks3 force-pushed the HCPE-830-tgw-resource branch from 67a5712 to b4cb33e Compare February 10, 2021 16:20
@smaant
Copy link
Contributor

smaant commented Feb 11, 2021

Great job @roaks3, thank you for handling this!

Copy link
Contributor

@nombiezinja nombiezinja left a comment

Choose a reason for hiding this comment

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

Awesome work, appreciate you taking this on!

@roaks3 roaks3 merged commit bf1cd0b into main Feb 11, 2021
@roaks3 roaks3 deleted the HCPE-830-tgw-resource branch February 11, 2021 22:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants