resource/networkmanager_vpn_attachment and update attachment_accepter #27387

juhala-aws · 2022-10-21T18:42:16Z

Description

Adds new Network Manager VPN Attachment resource and updates logic in Attachment Accepter. Previously Attachment Accepter only worked with VPC attachments. These changes are tightly related to each other as attachment accepter depends on functions in VPN attachment.

Relations

Relates #26420
Relates #25835
Depends #27693
Closes #27544.

References

CreateSiteToSiteVpnAttachment

Output from Acceptance Testing

-> % make testacc PKG=networkmanager TESTS=TestAccNetworkManagerVpnAttachment_basic
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./internal/service/networkmanager/... -v -count 1 -parallel 20 -run='TestAccNetworkManagerVpnAttachment_basic'  -timeout 180m
=== RUN   TestAccNetworkManagerVpnAttachment_basic
=== PAUSE TestAccNetworkManagerVpnAttachment_basic
=== CONT  TestAccNetworkManagerVpnAttachment_basic
--- PASS: TestAccNetworkManagerVpnAttachment_basic (1632.24s)
PASS
ok      github.com/hashicorp/terraform-provider-aws/internal/service/networkmanager     1636.750s


-> % make testacc PKG=networkmanager TESTS=TestAccNetworkManagerVpnAttachment_disappears
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./internal/service/networkmanager/... -v -count 1 -parallel 20 -run='TestAccNetworkManagerVpnAttachment_disappears'  -timeout 180m
=== RUN   TestAccNetworkManagerVpnAttachment_disappears
=== PAUSE TestAccNetworkManagerVpnAttachment_disappears
=== CONT  TestAccNetworkManagerVpnAttachment_disappears
--- PASS: TestAccNetworkManagerVpnAttachment_disappears (1625.69s)
PASS
ok      github.com/hashicorp/terraform-provider-aws/internal/service/networkmanager     1630.283s


-> % make testacc PKG=networkmanager TESTS=TestAccNetworkManagerVpnAttachment_tags
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./internal/service/networkmanager/... -v -count 1 -parallel 20 -run='TestAccNetworkManagerVpnAttachment_tags'  -timeout 180m
=== RUN   TestAccNetworkManagerVpnAttachment_tags
=== PAUSE TestAccNetworkManagerVpnAttachment_tags
=== CONT  TestAccNetworkManagerVpnAttachment_tags
--- PASS: TestAccNetworkManagerVpnAttachment_tags (1721.17s)
PASS
ok      github.com/hashicorp/terraform-provider-aws/internal/service/networkmanager     1725.774s

github-actions · 2022-10-21T18:43:35Z

Community Note

Voting for Prioritization

Please vote on this pull request by adding a 👍 reaction to the original post to help the community and maintainers prioritize this pull request.
Please see our prioritization guide for information on how we prioritize.
Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for issue followers and do not help prioritize the request.

For Submitters

Review the contribution guide relating to the type of change you are making to ensure all of the necessary steps have been taken.
For new resources and data sources, use skaff to generate scaffolding with comments detailing common expectations.
Whether or not the branch has been rebased will not impact prioritization, but doing so is always a welcome surprise.

github-actions

Welcome @juhala-aws 👋

It looks like this is your first Pull Request submission to the Terraform AWS Provider! If you haven’t already done so please make sure you have checked out our CONTRIBUTOR guide and FAQ to make sure your contribution is adhering to best practice and has all the necessary elements in place for a successful approval.

Also take a look at our FAQ which details how we prioritize Pull Requests for inclusion.

Thanks again, and welcome to the community! 😃

schuylr

LGTM with some minor changes!

internal/service/networkmanager/attachment_accepter.go

internal/service/networkmanager/vpn_attachment.go

schuylr · 2022-10-21T20:31:23Z

internal/service/networkmanager/vpn_attachment.go

+			"arn": {
+				Type:     schema.TypeString,
+				Computed: true,
+			},


It looks like id should also be used, example here: https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/networkmanager_vpc_attachment#id

yfauser

That's a lot of great work in there @juhala-aws ! I left a couple of comments. Also, you still need to add the changelog file in .changelog

internal/service/networkmanager/vpn_attachment.go

yfauser · 2022-10-21T20:49:21Z

internal/service/networkmanager/attachment_accepter.go

@@ -2,6 +2,7 @@ package networkmanager

 import (
 	"context"
+	"log"


You might have to review the way you do logging (https://developer.hashicorp.com/terraform/plugin/log/writing). That's dependent on the SDK version

yfauser · 2022-10-21T20:59:41Z

internal/service/networkmanager/vpn_attachment.go

+		DeleteWithoutTimeout: resourceVpnAttachmentDelete,
+
+		Importer: &schema.ResourceImporter{
+			State: schema.ImportStatePassthrough,


I think you should use ImportStatePassthroughContext
https://pkg.go.dev/github.com/hashicorp/terraform-plugin-sdk/v2@v2.24.0/helper/schema#ImportStatePassthroughContext

The ImportStatePassthrough seems to be deprecated
https://pkg.go.dev/github.com/hashicorp/terraform-plugin-sdk/v2@v2.24.0/helper/schema#ImportStatePassthrough

internal/service/networkmanager/vpn_attachment.go

website/docs/r/networkmanager_attachment_accepter.html.markdown

website/docs/r/networkmanager_vpn_attachment.html.markdown

gdavison

I've added some comments to the PR. The resource type will also need to be registered with the provider in the file internal/provider/provider.go

internal/service/networkmanager/attachment_accepter.go

gdavison · 2022-10-24T22:47:17Z

internal/service/networkmanager/attachment_accepter.go

+			return diag.Errorf("reading Network Manager VPC Attachment (%s): %s", d.Id(), err)
+		}
+
+		a := vpcAttachment.Attachment


In both cases, a is being set to the *networkmanager.Attachment returned and then the same values are being set on the resource. Instead of duplicating the code for each case, a could be retrieved using the Find... function, and a could be handled once.

How would you do that change as the a is of different type in both cases?

In both cases, a is a *networkmanager.Attachment, so you could declare a before he switch statement, set the value in the switch statement, and then read the values after the switch.

Something like:

var a *networkmanager.Attachment switch aType := d.Get("attachment_type"); aType { case "VPC": ... a = vpcAttachment.Attachment case "SITE_TO_SITE_VPN": ... a = vpnAttachment.Attachment } d.Set("a_b_c", a.ABC) ... d.Set("x_y_z", a.XYZ)

internal/service/networkmanager/vpn_attachment.go

internal/service/networkmanager/vpn_attachment_test.go

…pn).

…onnection_arn'.

ewbankkit

LGTM 🚀.

% make testacc TESTARGS='-run=TestAccNetworkManagerSiteToSiteVPNAttachment_' PKG=networkmanager ACCTEST_PARALLELISM=2
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./internal/service/networkmanager/... -v -count 1 -parallel 2  -run=TestAccNetworkManagerSiteToSiteVPNAttachment_ -timeout 180m
=== RUN   TestAccNetworkManagerSiteToSiteVPNAttachment_basic
=== PAUSE TestAccNetworkManagerSiteToSiteVPNAttachment_basic
=== RUN   TestAccNetworkManagerSiteToSiteVPNAttachment_disappears
=== PAUSE TestAccNetworkManagerSiteToSiteVPNAttachment_disappears
=== RUN   TestAccNetworkManagerSiteToSiteVPNAttachment_tags
=== PAUSE TestAccNetworkManagerSiteToSiteVPNAttachment_tags
=== CONT  TestAccNetworkManagerSiteToSiteVPNAttachment_basic
=== CONT  TestAccNetworkManagerSiteToSiteVPNAttachment_tags
--- PASS: TestAccNetworkManagerSiteToSiteVPNAttachment_tags (1181.63s)
=== CONT  TestAccNetworkManagerSiteToSiteVPNAttachment_disappears
--- PASS: TestAccNetworkManagerSiteToSiteVPNAttachment_basic (1289.84s)
--- PASS: TestAccNetworkManagerSiteToSiteVPNAttachment_disappears (1508.83s)
PASS
ok  	github.com/hashicorp/terraform-provider-aws/internal/service/networkmanager	2694.597s
% make testacc TESTARGS='-run=TestAccNetworkManagerVPCAttachment_' PKG=networkmanager ACCTEST_PARALLELISM=2
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./internal/service/networkmanager/... -v -count 1 -parallel 2  -run=TestAccNetworkManagerVPCAttachment_ -timeout 180m
=== RUN   TestAccNetworkManagerVPCAttachment_basic
=== PAUSE TestAccNetworkManagerVPCAttachment_basic
=== RUN   TestAccNetworkManagerVPCAttachment_disappears
=== PAUSE TestAccNetworkManagerVPCAttachment_disappears
=== RUN   TestAccNetworkManagerVPCAttachment_tags
=== PAUSE TestAccNetworkManagerVPCAttachment_tags
=== RUN   TestAccNetworkManagerVPCAttachment_update
=== PAUSE TestAccNetworkManagerVPCAttachment_update
=== CONT  TestAccNetworkManagerVPCAttachment_basic
=== CONT  TestAccNetworkManagerVPCAttachment_tags
--- PASS: TestAccNetworkManagerVPCAttachment_basic (887.30s)
=== CONT  TestAccNetworkManagerVPCAttachment_update
--- PASS: TestAccNetworkManagerVPCAttachment_tags (1008.65s)
=== CONT  TestAccNetworkManagerVPCAttachment_disappears
--- PASS: TestAccNetworkManagerVPCAttachment_update (1110.56s)
--- PASS: TestAccNetworkManagerVPCAttachment_disappears (960.53s)
PASS
ok  	github.com/hashicorp/terraform-provider-aws/internal/service/networkmanager	1973.655s

ewbankkit · 2022-11-10T18:55:33Z

@juhala-aws Thanks for the contribution 🎉 👏.
In general everything was great.
I went ahead and changed the resource name to aws_networkmanager_site_to_site_vpn_attachment to match the underlying AWS API and future-proof against potentially other types of VPN attachment.

github-actions · 2022-11-10T23:54:11Z

This functionality has been released in v4.39.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. Thank you!

github-actions · 2022-12-11T02:19:18Z

I'm going to lock this pull request 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 related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

juhala-aws added 2 commits October 21, 2022 11:14

Add networkmanager VPN attachment and update attachment_accepter

9cc884c

Add documentation for vpn_attachment and attachment_accepter

f28cf86

github-actions bot reviewed Oct 21, 2022

View reviewed changes

Format Terraform

0e671c1

schuylr suggested changes Oct 21, 2022

View reviewed changes

yfauser reviewed Oct 21, 2022

View reviewed changes

gdavison requested changes Oct 24, 2022

View reviewed changes

justinretzolk removed the needs-triage Waiting for first response or review from a maintainer. label Nov 1, 2022

juhala-aws added 6 commits November 7, 2022 10:50

Change cases to use consts

cabcc24

Change function call to individual values

69aabdc

Validate ARN with regex

2f740c1

Fix logging messages

56075a2

Change function to private

c1fcf44

Add VPN attachment resource to provider

3660bf1

juhala-aws mentioned this pull request Nov 8, 2022

Change parameter to optional and add test #27693

Merged

github-actions bot added the provider Pertains to the provider itself, rather than any interaction with AWS. label Nov 8, 2022

juhala-aws added 2 commits November 8, 2022 20:29

Add changelog

e7ea76a

Fix linting

6b59762

juhala-aws marked this pull request as ready for review November 10, 2022 08:01

juhala-aws and others added 5 commits November 10, 2022 10:47

Fix semgrep

7a82ec5

Merge branch 'main' into f-networkmanager_vpn_attachment

a6ab736

Merge branch 'main' into f-networkmanager_vpn_attachment

036631a

Fix semgrep

83555b5

Tweak documentation.

ebbc05c

ewbankkit added 6 commits November 10, 2022 12:05

r/aws_networkmanager_vpn_attachment: Rename files (site_to_site_vpn).

a57415d

r/aws_networkmanager_vpn_attachment: Rename functions (site_to_site_v…

c226839

…pn).

r/aws_networkmanager_vpn_attachment: Rename resource (site_to_site_vpn).

b4e79f9

r/aws_networkmanager_site_to_site_vpn_attachment: 'vpn_arn' -> 'vpn_c…

676b40e

…onnection_arn'.

r/aws_networkmanager_site_to_site_vpn_attachment: Update error messages.

cd99a88

r/aws_networkmanager_attachment_accepter: Update error messages.

8e0f25a

ewbankkit approved these changes Nov 10, 2022

View reviewed changes

ewbankkit added 3 commits November 10, 2022 14:35

Correct 'TestAccNetworkManagerVPCAttachment_basic'.

ff4efc6

Fix typo.

f891366

Documentation corrections.

5a3289c

ewbankkit merged commit 5a83916 into hashicorp:main Nov 10, 2022

github-actions bot added this to the v4.39.0 milestone Nov 10, 2022

ewbankkit mentioned this pull request Nov 15, 2022

Added a full example for aws_networkmanager_site_to_site_vpn_attachment documentation #27807

Merged

github-actions bot locked as resolved and limited conversation to collaborators Dec 11, 2022

justinretzolk added the partner Contribution from a partner. label May 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

resource/networkmanager_vpn_attachment and update attachment_accepter #27387

resource/networkmanager_vpn_attachment and update attachment_accepter #27387

juhala-aws commented Oct 21, 2022 •

edited

Loading

github-actions bot commented Oct 21, 2022

github-actions bot left a comment

schuylr left a comment

schuylr Oct 21, 2022

yfauser left a comment

yfauser Oct 21, 2022

yfauser Oct 21, 2022

gdavison left a comment

gdavison Oct 24, 2022

juhala-aws Nov 10, 2022

gdavison Nov 11, 2022

ewbankkit left a comment •

edited

Loading

ewbankkit commented Nov 10, 2022 •

edited

Loading

github-actions bot commented Nov 10, 2022

github-actions bot commented Dec 11, 2022

resource/networkmanager_vpn_attachment and update attachment_accepter #27387

resource/networkmanager_vpn_attachment and update attachment_accepter #27387

Conversation

juhala-aws commented Oct 21, 2022 • edited Loading

Description

Relations

References

Output from Acceptance Testing

github-actions bot commented Oct 21, 2022

Community Note

github-actions bot left a comment

Choose a reason for hiding this comment

schuylr left a comment

Choose a reason for hiding this comment

schuylr Oct 21, 2022

Choose a reason for hiding this comment

yfauser left a comment

Choose a reason for hiding this comment

yfauser Oct 21, 2022

Choose a reason for hiding this comment

yfauser Oct 21, 2022

Choose a reason for hiding this comment

gdavison left a comment

Choose a reason for hiding this comment

gdavison Oct 24, 2022

Choose a reason for hiding this comment

juhala-aws Nov 10, 2022

Choose a reason for hiding this comment

gdavison Nov 11, 2022

Choose a reason for hiding this comment

ewbankkit left a comment • edited Loading

Choose a reason for hiding this comment

ewbankkit commented Nov 10, 2022 • edited Loading

github-actions bot commented Nov 10, 2022

github-actions bot commented Dec 11, 2022

juhala-aws commented Oct 21, 2022 •

edited

Loading

ewbankkit left a comment •

edited

Loading

ewbankkit commented Nov 10, 2022 •

edited

Loading