Add deployment type to aws_fsx_lustre_file_system by mtpdt · Pull Request #13639 · hashicorp/terraform-provider-aws

mtpdt · 2020-06-05T17:49:11Z

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:

resource/aws_fsx_lustre_file_system: Add `deployment_type` and `per_unit_storage_throughput`

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccAWSFsxLustreFileSystem_DeploymentType'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSFsxLustreFileSystem_DeploymentType -timeout 120m
make testacc TESTARGS='-run=TestAccAWSFsxLustreFileSystem_basic'
=== RUN   TestAccAWSFsxLustreFileSystem_DeploymentTypePersistent1
=== PAUSE TestAccAWSFsxLustreFileSystem_DeploymentTypePersistent1
=== RUN   TestAccAWSFsxLustreFileSystem_DeploymentTypeScratch2
=== PAUSE TestAccAWSFsxLustreFileSystem_DeploymentTypeScratch2
=== CONT  TestAccAWSFsxLustreFileSystem_DeploymentTypePersistent1
=== CONT  TestAccAWSFsxLustreFileSystem_DeploymentTypeScratch2
--- PASS: TestAccAWSFsxLustreFileSystem_DeploymentTypeScratch2 (434.35s)
--- PASS: TestAccAWSFsxLustreFileSystem_DeploymentTypePersistent1 (518.06s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	518.129s

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

mtpdt · 2020-06-05T17:52:03Z

Hello, this is my first pull request into terraform-provider-aws, so I appreciate your understanding. In your contribution guide, you mention an option to accept a "best effort" implementation of acceptance tests. Could you please try to run the acceptance tests I wrote for this feature?

DrFaust92

Looks Good, a few comments

DrFaust92 · 2020-07-18T11:46:18Z

please add these fields to the read function.
something along the lines of:

d.Set("deployment_type", filesystem.LustreConfiguration.DeploymentType)

you would probably need to add default value of fsx.LustreDeploymentTypeScratch1 to deployment_type as well.

done, added a default and added the line you requested to the read function.

DrFaust92 · 2020-07-18T11:47:21Z

lets simplify this to enumerate the values in the docs here.

Done, I followed the format I found from another AWS resource's doumentation.

mtpdt · 2020-07-21T15:47:18Z

@DrFaust92 requested changes implemented and unit tests pass.

I have a few more minor improvements to aws_lustre_file_system that will come after this PR. Would it help speed things along if I was able to run acceptance tests myself? I'm in a bit of a constrained environment, but if it's worth it, I could put work into being able to run the acceptance tests.

DrFaust92

some more changes to tests

DrFaust92 · 2020-07-21T15:42:31Z

no need for the second test as its a property that cannot be updated

DrFaust92 · 2020-07-21T15:54:22Z

probably need to change it to the default value here

DrFaust92 · 2020-07-21T15:58:02Z

i think that it would be best to split this to 2 separate tests for PERSISTENT_1
and just have a plan only test for SCRATCH_1
see examples here:
https://github.com/terraform-providers/terraform-provider-aws/pull/12676/files#diff-17cef5d816ce52cd994aef2105a31a2dR117
https://github.com/terraform-providers/terraform-provider-aws/pull/12676/files#diff-17cef5d816ce52cd994aef2105a31a2dR124

Done, I've created 2 separate tests, one for PERSISTENT_1, and another for SCRATCH_2.

And I've added a plan only test for SCRATCH_1.

DrFaust92 · 2020-07-21T15:59:05Z

@mtpdt, some more changes to tests and it would be best if you could run these to save us sometime and cycles

mtpdt · 2020-07-21T22:57:52Z

@DrFaust92 thank you for the reviews. All current comments addressed.

I'll work on my end to run the tests myself for the subsequent PRs I send.

DrFaust92 · 2020-07-22T15:12:14Z

getting failures for a few tests:

TestAccAWSFsxLustreFileSystem_PerUnitStorageThroughput
2020/07/22 13:29:52 [ERROR] <root>: eval: *terraform.EvalApplyPost, err: Error creating FSx filesystem: IncompatibleParameterError: The storage throughput can not be specified for a lustre file system with DeploymentType=SCRATCH_2

    TestAccAWSFsxLustreFileSystem_basic: testing.go:684: Step 0 error: Check failed: Check 16/16 error: aws_fsx_lustre_file_system.test: Attribute 'per_unit_storage_throughput' not found

TestAccAWSFsxLustreFileSystem_Scratch2DeploymentType: testing.go:684: Step 0 error: errors during apply:
        
        Error: Error creating FSx filesystem: IncompatibleParameterError: The storage throughput can not be specified for a lustre file system with DeploymentType=SCRATCH_2

it seems that per_unit_storage_throughput can only be set with a persistent deployment type

DrFaust92 · 2020-07-22T15:15:45Z

actually it seems you are missing something along the lines of the following in the read func:

	if filesystem.LustreConfiguration.PerUnitStorageThroughput != nil {
		d.Set("per_unit_storage_throughput", filesystem.LustreConfiguration.PerUnitStorageThroughput)
	}

mtpdt · 2020-07-22T22:34:37Z

@DrFaust92 thank you for that code snippet.

I've fixed both errors, and I've managed to get the acceptance tests running in my environment. I've updated the PR comment to include the passing acceptance test output for the new tests I've added, as well as the basic integration test for fsx lustre.

mtpdt · 2020-07-22T22:36:03Z

I took out the separate per unit storage throughput test, and combined it into this one, since per unit storage throughput is only valid for persistent1 type lustre.

DrFaust92

minor test change for import tests

DrFaust92 · 2020-07-23T07:26:52Z

can you add:

{ ImportStateVerify: true, ImportStateVerifyIgnore: []string{"security_group_ids"}, },

to the added tests?

DrFaust92 · 2020-07-23T15:31:27Z

thanks @mtpdt, i'll try to run these later today. fsx take so long to create and test 😢

DrFaust92 · 2020-07-24T10:04:08Z

sorry @mtpdt, I messed up the import test addition due to copy paste

can you change it to

			{
				ResourceName:            resourceName,
				ImportState:             true,
				ImportStateVerify:       true,
				ImportStateVerifyIgnore: []string{"security_group_ids"},
			},

otherwise looks good and well run tests one more time and we are good to go

mtpdt · 2020-07-24T14:07:30Z

@DrFaust92 no problem, thank you for running the tests for me. I've updated this PR as per your requested changes.

Note that after this one, I have two much smaller related PRs: 14314 and 14313, I have already run the acceptance tests for both of them.

DrFaust92

--- PASS: TestAccAWSFsxLustreFileSystem_DeploymentTypePersistent1 (599.31s)
--- PASS: TestAccAWSFsxLustreFileSystem_DeploymentTypeScratch2 (586.80s)
--- PASS: TestAccAWSFsxLustreFileSystem_basic (963.80s)

LGTM, ill take a look at the other PRs as well

breathingdust · 2020-07-28T23:30:33Z

LGTM 🚀 Thanks @DrFaust92!

Verified Acceptance Tests

make testacc TEST=./aws/ TESTARGS='-run=TestAccAWSFsxLustreFileSystem_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSFsxLustreFileSystem_ -timeout 120m
=== RUN   TestAccAWSFsxLustreFileSystem_basic
=== PAUSE TestAccAWSFsxLustreFileSystem_basic
=== RUN   TestAccAWSFsxLustreFileSystem_disappears
=== PAUSE TestAccAWSFsxLustreFileSystem_disappears
=== RUN   TestAccAWSFsxLustreFileSystem_ExportPath
=== PAUSE TestAccAWSFsxLustreFileSystem_ExportPath
=== RUN   TestAccAWSFsxLustreFileSystem_ImportPath
=== PAUSE TestAccAWSFsxLustreFileSystem_ImportPath
=== RUN   TestAccAWSFsxLustreFileSystem_ImportedFileChunkSize
=== PAUSE TestAccAWSFsxLustreFileSystem_ImportedFileChunkSize
=== RUN   TestAccAWSFsxLustreFileSystem_SecurityGroupIds
=== PAUSE TestAccAWSFsxLustreFileSystem_SecurityGroupIds
=== RUN   TestAccAWSFsxLustreFileSystem_StorageCapacity
=== PAUSE TestAccAWSFsxLustreFileSystem_StorageCapacity
=== RUN   TestAccAWSFsxLustreFileSystem_Tags
=== PAUSE TestAccAWSFsxLustreFileSystem_Tags
=== RUN   TestAccAWSFsxLustreFileSystem_WeeklyMaintenanceStartTime
=== PAUSE TestAccAWSFsxLustreFileSystem_WeeklyMaintenanceStartTime
=== RUN   TestAccAWSFsxLustreFileSystem_DeploymentTypePersistent1
=== PAUSE TestAccAWSFsxLustreFileSystem_DeploymentTypePersistent1
=== RUN   TestAccAWSFsxLustreFileSystem_DeploymentTypeScratch2
=== PAUSE TestAccAWSFsxLustreFileSystem_DeploymentTypeScratch2
=== CONT  TestAccAWSFsxLustreFileSystem_basic
=== CONT  TestAccAWSFsxLustreFileSystem_StorageCapacity
=== CONT  TestAccAWSFsxLustreFileSystem_DeploymentTypePersistent1
=== CONT  TestAccAWSFsxLustreFileSystem_WeeklyMaintenanceStartTime
=== CONT  TestAccAWSFsxLustreFileSystem_Tags
=== CONT  TestAccAWSFsxLustreFileSystem_ExportPath
=== CONT  TestAccAWSFsxLustreFileSystem_DeploymentTypeScratch2
=== CONT  TestAccAWSFsxLustreFileSystem_ImportPath
=== CONT  TestAccAWSFsxLustreFileSystem_SecurityGroupIds
=== CONT  TestAccAWSFsxLustreFileSystem_ImportedFileChunkSize
=== CONT  TestAccAWSFsxLustreFileSystem_disappears
--- PASS: TestAccAWSFsxLustreFileSystem_DeploymentTypePersistent1 (426.79s)
--- PASS: TestAccAWSFsxLustreFileSystem_WeeklyMaintenanceStartTime (440.55s)
--- PASS: TestAccAWSFsxLustreFileSystem_basic (486.00s)
--- PASS: TestAccAWSFsxLustreFileSystem_Tags (541.28s)
--- PASS: TestAccAWSFsxLustreFileSystem_DeploymentTypeScratch2 (548.79s)
--- PASS: TestAccAWSFsxLustreFileSystem_disappears (559.20s)
--- PASS: TestAccAWSFsxLustreFileSystem_StorageCapacity (996.81s)
--- PASS: TestAccAWSFsxLustreFileSystem_SecurityGroupIds (1010.93s)
--- PASS: TestAccAWSFsxLustreFileSystem_ImportedFileChunkSize (1214.90s)
--- PASS: TestAccAWSFsxLustreFileSystem_ExportPath (1457.39s)
--- PASS: TestAccAWSFsxLustreFileSystem_ImportPath (1476.03s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	1478.540s

mtpdt · 2020-07-29T14:18:59Z

nice, thanks!

ghost · 2020-07-31T02:23:33Z

This has been released in version 3.0.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 · 2020-08-28T17:10:31Z

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!

mtpdt requested a review from a team June 5, 2020 17:49

mtpdt force-pushed the f-aws_fsx_lustre_file_system-deployment_type branch 5 times, most recently from 6926691 to 6f9f406 Compare June 5, 2020 19:56

DrFaust92 removed the needs-triage Waiting for first response or review from a maintainer. label Jul 18, 2020

DrFaust92 self-assigned this Jul 18, 2020

DrFaust92 suggested changes Jul 18, 2020

View reviewed changes

mtpdt force-pushed the f-aws_fsx_lustre_file_system-deployment_type branch 5 times, most recently from 8f50661 to 1c1a62b Compare July 21, 2020 14:32

mtpdt requested review from DrFaust92 and removed request for a team July 21, 2020 15:47

DrFaust92 suggested changes Jul 21, 2020

View reviewed changes

anGie44 force-pushed the master branch from 54db70d to 48ba2f7 Compare July 21, 2020 18:26

mtpdt force-pushed the f-aws_fsx_lustre_file_system-deployment_type branch 3 times, most recently from 3bbb029 to 260518a Compare July 21, 2020 21:21

mtpdt force-pushed the f-aws_fsx_lustre_file_system-deployment_type branch from 260518a to ca2d1d2 Compare July 22, 2020 22:15

mtpdt commented Jul 22, 2020

View reviewed changes

mtpdt mentioned this pull request Jul 23, 2020

Add mount_name to aws_fsx_lustre_file_system #14313

Merged

DrFaust92 suggested changes Jul 23, 2020

View reviewed changes

mtpdt force-pushed the f-aws_fsx_lustre_file_system-deployment_type branch from ca2d1d2 to fc0e3db Compare July 23, 2020 14:36

Add deployment type to aws_fsx_lustre_file_system

4ba8382

mtpdt force-pushed the f-aws_fsx_lustre_file_system-deployment_type branch from fc0e3db to 4ba8382 Compare July 24, 2020 14:04

DrFaust92 approved these changes Jul 24, 2020

View reviewed changes

DrFaust92 assigned breathingdust and unassigned DrFaust92 Jul 28, 2020

breathingdust added this to the v3.0.0 milestone Jul 28, 2020

breathingdust merged commit 47d4803 into hashicorp:master Jul 28, 2020

mtpdt deleted the f-aws_fsx_lustre_file_system-deployment_type branch July 29, 2020 14:19

ghost locked and limited conversation to collaborators Aug 28, 2020

Conversation

mtpdt commented Jun 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Community Note

Uh oh!

mtpdt commented Jun 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrFaust92 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtpdt commented Jul 21, 2020

Uh oh!

DrFaust92 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DrFaust92 commented Jul 21, 2020

Uh oh!

mtpdt commented Jul 21, 2020

Uh oh!

DrFaust92 commented Jul 22, 2020

Uh oh!

DrFaust92 commented Jul 22, 2020

Uh oh!

mtpdt commented Jul 22, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DrFaust92 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DrFaust92 commented Jul 23, 2020

Uh oh!

DrFaust92 commented Jul 24, 2020

Uh oh!

mtpdt commented Jul 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrFaust92 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

breathingdust commented Jul 28, 2020

Uh oh!

mtpdt commented Jul 29, 2020

Uh oh!

ghost commented Jul 31, 2020

Uh oh!

ghost commented Aug 28, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mtpdt commented Jun 5, 2020 •

edited

Loading

mtpdt commented Jun 5, 2020 •

edited

Loading

mtpdt commented Jul 24, 2020 •

edited

Loading

DrFaust92 left a comment •

edited

Loading