resource/aws_dx_private_virtual_interface: Update resource to support MTU param #6141

slapula · 2018-10-13T04:32:41Z

Changes proposed in this pull request:

Updated aws_dx_private_virtual_interface resource to support new MTU parameter

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccAwsDxPrivateVirtualInterface_basic'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -run=TestAccAwsDxPrivateVirtualInterface_basic -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAwsDxPrivateVirtualInterface_basic
--- SKIP: TestAccAwsDxPrivateVirtualInterface_basic (0.00s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	0.029s

… MTU param

ewbankkit · 2018-10-13T11:07:48Z

resource/aws_dx_hosted_private_virtual_interface: Update resource to support MTU param #6142
resource/aws_dx_connection: Export bool value of new MTU parameter #6143

ewbankkit · 2018-10-13T11:11:28Z

Please add verification of the mtu and jumbo_frame_enabled attributes to the acceptance tests; something like:

resource.TestCheckResourceAttr("aws_dx_private_virtual_interface.foo", "mtu", "9001"),
resource.TestCheckResourceAttr("aws_dx_private_virtual_interface.foo", "jumbo_frame_enabled", "1"),

ewbankkit · 2018-10-13T11:14:01Z

aws/resource_aws_dx_private_virtual_interface.go

+				Computed: true,
+				ForceNew: true,
+			},
+			"jumbo_frame_enabled": {


Nitpick - In the documentation the feature is called Jumbo Frames. For consistency I suggest naming the attribute jumbo_frames_enabled or jumbo_frame_capable (to match the API).

Oops, I meant to have this match the API but I just spaced on it. This will be corrected in my next commit.

ewbankkit · 2018-10-13T20:49:27Z

aws/resource_aws_dx_private_virtual_interface.go

@@ -84,6 +84,16 @@ func resourceAwsDxPrivateVirtualInterface() *schema.Resource {
 				Computed: true,
 				ForceNew: true,
 			},
+			"mtu": {


See ValidateFunc comment: https://github.com/terraform-providers/terraform-provider-aws/pull/6142/files#r224971730.

ewbankkit · 2018-10-13T20:54:34Z

aws/resource_aws_dx_private_virtual_interface.go

+				Type:     schema.TypeInt,
+				Default:  1500,
+				Optional: true,
+				ForceNew: true,


See Update comment: https://github.com/terraform-providers/terraform-provider-aws/pull/6142/files#r224971890.

slapula · 2018-10-15T03:12:47Z

@ewbankkit I believe I have addressed your comments with this latest commit. I ran out of time today to test and write tests so I will likely circle back to that tomorrow or Tuesday. It looks to me that my changes "should work" so if you have spare cycles to test before I get to it please feel free.

slapula · 2018-10-15T03:16:21Z

aws/resource_aws_dx_private_virtual_interface.go

@@ -219,3 +235,22 @@ func dxPrivateVirtualInterfaceWaitUntilAvailable(d *schema.ResourceData, conn *d
 			directconnect.VirtualInterfaceStateDown,
 		})
 }
+
+func IntInSlice(valid []int) schema.SchemaValidateFunc {


Again, haven't fully tested this yet but would this be a good candidate to move to the core validation library?

Yes, I think it would but given the churn in the codebase with v0.12 it is probably best to wait until that settles down.

Probably better to move this to validators.go and rename it to something like validateIntInSlice and add a unit test to validators_test.go.

ewbankkit · 2018-10-15T10:55:44Z

OK, I'll look at testing this week.

ewbankkit · 2018-10-21T22:41:56Z

@slapula I made a couple of mainly cosmetic changes and pushed to my slapula-dx-private-virtual-interface-mtu branch:

According to the documentation, after an update is made to the virtual interface its state will be pending until the update is complete so I added an Update value to the schema's Timeouts and added a call to dxPrivateVirtualInterfaceWaitUntilAvailable after calling the update API (this changed the signature to a function shared by all the VIF attributes so there was some cascading call site changes)
Simplified your acceptance tests - No need to do the MTU update in TestAccAwsDxPrivateVirtualInterface_basic, just do it in TestAccAwsDxPrivateVirtualInterface_mtuUpdate
Minor documentation tweaks

I should be able to run the acceptance tests in the next couple of days - I'll make any modifications on that branch and you can then merge back.

ewbankkit · 2018-10-22T20:43:05Z

@slapula I took your latest code (fixed a merge error) and ran acceptance tests for the new test case.
With one minor change

resource.TestCheckResourceAttr("aws_dx_private_virtual_interface.foo", "jumbo_frame_capable", "true")
->
resource.TestCheckResourceAttr("aws_dx_private_virtual_interface.foo", "jumbo_frame_capable", "1")

the test passes:

$ DX_CONNECTION_ID=dxcon-xxxxxxxx AWS_DEFAULT_REGION=us-east-1 make testacc TESTARGS='-run=TestAccAwsDxPrivateVirtualInterface_mtuUpdate'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -parallel 20 -run=TestAccAwsDxPrivateVirtualInterface_mtuUpdate -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAwsDxPrivateVirtualInterface_mtuUpdate
--- PASS: TestAccAwsDxPrivateVirtualInterface_mtuUpdate (1252.51s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	1270.641s

If you merge the latest changes from https://github.com/ewbankkit/terraform-provider-aws/tree/slapula-dx-private-virtual-interface-mtu I can the run all the aws_dx_private_virtual_interface acceptance tests on the complete code.

slapula · 2018-10-22T22:28:30Z

@ewbankkit Thanks for the help on this! I totally spaced on the Update and WaitUntilAvailable calls so good catch. Anyway, I have your changes pulled in and Travis is green. If we're good here, I can start porting these changes over to #6142.

ewbankkit · 2018-10-23T13:00:12Z

aws/resource_aws_dx_private_virtual_interface_test.go

+					testAccCheckAwsDxPrivateVirtualInterfaceExists("aws_dx_private_virtual_interface.foo"),
+					resource.TestCheckResourceAttr("aws_dx_private_virtual_interface.foo", "name", vifName),
+					resource.TestCheckResourceAttr("aws_dx_private_virtual_interface.foo", "mtu", "1500"),
+					resource.TestCheckResourceAttr("aws_dx_private_virtual_interface.foo", "jumbo_frame_capable", "1"),


Need to change this to

resource.TestCheckResourceAttr("aws_dx_private_virtual_interface.foo", "jumbo_frame_capable", "true"),

or else will get errors like

$ DX_CONNECTION_ID=dxcon-xxxxxxxx AWS_DEFAULT_REGION=us-east-1 make testacc TESTARGS='-run=TestAccAwsDxPrivateVirtualInterface_mtuUpdate' ==> Checking that code complies with gofmt requirements... TF_ACC=1 go test ./... -v -parallel 20 -run=TestAccAwsDxPrivateVirtualInterface_mtuUpdate -timeout 120m ? github.com/terraform-providers/terraform-provider-aws [no test files] === RUN TestAccAwsDxPrivateVirtualInterface_mtuUpdate --- FAIL: TestAccAwsDxPrivateVirtualInterface_mtuUpdate (667.21s) testing.go:538: Step 0 error: Check failed: Check 4/4 error: aws_dx_private_virtual_interface.foo: Attribute 'jumbo_frame_capable' expected "1", got "true" FAIL FAIL github.com/terraform-providers/terraform-provider-aws/aws 668.068s make: *** [testacc] Error 1

ewbankkit · 2018-10-23T13:01:55Z

@slapula Left a comment on changing 1 to true in the new acceptance test, once that's committed I can run the full acceptance tests. Yes, makes sense to merge the changes into the other PR.

slapula · 2018-10-23T13:30:44Z

Oh ok, I misread your previous comment. I'll update that in a moment.

ewbankkit · 2018-10-23T16:11:17Z

OK, tested the code (serially as there is a problem with running the acceptance tests in parallel, #6244, which shouldn't block this PR).

$ git rev-parse HEAD
da69517c8d918611b8ab9599b7e82ee1445c8be4
$ DX_CONNECTION_ID=dxcon-xxxxxxxx AWS_DEFAULT_REGION=us-east-1 make testacc TESTARGS='-run=TestAccAwsDxPrivateVirtualInterface_mtuUpdate'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -parallel 20 -run=TestAccAwsDxPrivateVirtualInterface_mtuUpdate -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAwsDxPrivateVirtualInterface_mtuUpdate
--- PASS: TestAccAwsDxPrivateVirtualInterface_mtuUpdate (1252.51s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	(cached)
$ DX_CONNECTION_ID=dxcon-xxxxxxxx AWS_DEFAULT_REGION=us-east-1 make testacc TESTARGS='-run=TestAccAwsDxPrivateVirtualInterface_basic'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -parallel 20 -run=TestAccAwsDxPrivateVirtualInterface_basic -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAwsDxPrivateVirtualInterface_basic
=== PAUSE TestAccAwsDxPrivateVirtualInterface_basic
=== CONT  TestAccAwsDxPrivateVirtualInterface_basic
--- PASS: TestAccAwsDxPrivateVirtualInterface_basic (409.98s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	410.875s
$ DX_CONNECTION_ID=dxcon-xxxxxxxx AWS_DEFAULT_REGION=us-east-1 make testacc TESTARGS='-run=TestAccAwsDxPrivateVirtualInterface_dxGateway'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -parallel 20 -run=TestAccAwsDxPrivateVirtualInterface_dxGateway -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAwsDxPrivateVirtualInterface_dxGateway
=== PAUSE TestAccAwsDxPrivateVirtualInterface_dxGateway
=== CONT  TestAccAwsDxPrivateVirtualInterface_dxGateway
--- PASS: TestAccAwsDxPrivateVirtualInterface_dxGateway (589.50s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	590.384s

ewbankkit · 2018-10-23T16:12:43Z

This PR is now ready for review.

bflad

LGTM, thanks @slapula and @ewbankkit! 🚀

bflad · 2018-10-30T15:50:42Z

aws/dx_vif.go

@@ -97,17 +115,17 @@ func dxVirtualInterfaceStateRefresh(conn *directconnect.DirectConnect, vifId str
 	}
 }

-func dxVirtualInterfaceWaitUntilAvailable(d *schema.ResourceData, conn *directconnect.DirectConnect, pending, target []string) error {
+func dxVirtualInterfaceWaitUntilAvailable(conn *directconnect.DirectConnect, vifId string, timeout time.Duration, pending, target []string) error {


bflad · 2018-11-01T00:21:13Z

This has been released in version 1.42.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-02T17:44:00Z

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!

slapula added 2 commits October 12, 2018 22:47

resource/aws_dx_private_virtual_interface: Update resource to support…

5298e48

… MTU param

fixing mtu int handling

d549990

exporting value of jumbo frames param

b5f9c82

This was referenced Oct 13, 2018

resource/aws_dx_connection: Export bool value of new MTU parameter #6143

Merged

resource/aws_dx_hosted_private_virtual_interface: Update resource to support MTU param #6142

Merged

ewbankkit reviewed Oct 13, 2018

View reviewed changes

adding acc test for mtu

cdecb24

ghost added size/S Managed by automation to categorize the size of a PR. and removed size/XS Managed by automation to categorize the size of a PR. labels Oct 13, 2018

slapula added 2 commits October 13, 2018 08:04

correcting exported value name

4b3d2f0

setting default for mtu

c4593cc

ewbankkit reviewed Oct 13, 2018

View reviewed changes

ewbankkit mentioned this pull request Oct 14, 2018

Feature request: Direct Connect support for changing network MTU #6136

Closed

validating mtu input & adding expanding vif update function

636db33

ghost added size/M Managed by automation to categorize the size of a PR. and removed size/S Managed by automation to categorize the size of a PR. labels Oct 15, 2018

slapula commented Oct 15, 2018

View reviewed changes

slapula and others added 4 commits October 15, 2018 11:55

adding acc test for updating mtu

257a94a

'IntInSlice' -> 'validateIntegerInSlice'.

4cff326

Add Update timeout to r/aws_dx_private_virtual_interface.

0cf60ab

Merge branch 'master' into slapula-dx-private-virtual-interface-mtu

83b25df

slapula added 3 commits October 22, 2018 10:24

pulling in ewbankkit changes

bd5798b

forgot a missing file from previous commit

1d75428

Merge branch 'master' into dx-private-virtual-interface-mtu

6f8738b

fixing merge conflict

1e6b61a

ghost added size/L Managed by automation to categorize the size of a PR. and removed size/M Managed by automation to categorize the size of a PR. labels Oct 22, 2018

removing conflict cruft

77e465a

ewbankkit reviewed Oct 23, 2018

View reviewed changes

changing 1 to true

da69517

ewbankkit mentioned this pull request Oct 23, 2018

r/aws_dx_private_virtual_interface: Error running parallel acceptance tests #6244

Closed

Merge branch 'master' into dx-private-virtual-interface-mtu

1a7b0d0

bflad approved these changes Oct 30, 2018

View reviewed changes

bflad added this to the v1.42.0 milestone Oct 30, 2018

bflad merged commit 27d3c7c into hashicorp:master Oct 30, 2018

bflad added a commit that referenced this pull request Oct 30, 2018

Update CHANGELOG for #6141

cc1f8fb

ghost locked and limited conversation to collaborators Apr 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

resource/aws_dx_private_virtual_interface: Update resource to support MTU param #6141

resource/aws_dx_private_virtual_interface: Update resource to support MTU param #6141

slapula commented Oct 13, 2018 •

edited

Loading

ewbankkit commented Oct 13, 2018

ewbankkit commented Oct 13, 2018

ewbankkit Oct 13, 2018

slapula Oct 13, 2018

ewbankkit Oct 13, 2018

ewbankkit Oct 13, 2018

slapula commented Oct 15, 2018

slapula Oct 15, 2018

ewbankkit Oct 15, 2018

ewbankkit Oct 19, 2018 •

edited

Loading

ewbankkit commented Oct 15, 2018

ewbankkit commented Oct 21, 2018 •

edited

Loading

ewbankkit commented Oct 22, 2018

slapula commented Oct 22, 2018

ewbankkit Oct 23, 2018

ewbankkit commented Oct 23, 2018

slapula commented Oct 23, 2018

ewbankkit commented Oct 23, 2018

ewbankkit commented Oct 23, 2018

bflad left a comment

bflad Oct 30, 2018

bflad commented Nov 1, 2018

ghost commented Apr 2, 2020

resource/aws_dx_private_virtual_interface: Update resource to support MTU param #6141

resource/aws_dx_private_virtual_interface: Update resource to support MTU param #6141

Conversation

slapula commented Oct 13, 2018 • edited Loading

ewbankkit commented Oct 13, 2018

ewbankkit commented Oct 13, 2018

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

slapula commented Oct 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ewbankkit Oct 19, 2018 • edited Loading

Choose a reason for hiding this comment

ewbankkit commented Oct 15, 2018

ewbankkit commented Oct 21, 2018 • edited Loading

ewbankkit commented Oct 22, 2018

slapula commented Oct 22, 2018

Choose a reason for hiding this comment

ewbankkit commented Oct 23, 2018

slapula commented Oct 23, 2018

ewbankkit commented Oct 23, 2018

ewbankkit commented Oct 23, 2018

bflad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bflad commented Nov 1, 2018

ghost commented Apr 2, 2020

slapula commented Oct 13, 2018 •

edited

Loading

ewbankkit Oct 19, 2018 •

edited

Loading

ewbankkit commented Oct 21, 2018 •

edited

Loading