provider/aws: Handle missing EFS mount target in aws_efs_mount_target. #8529

kwilczynski · 2016-08-29T14:20:50Z

This commit resolves issue where the EFS mount target would be already
deleted (e.g. it was deleted outside of Terraform, etc.). Also, correct
how values are begin set in the ReadFunc to avoid nil pointer dereference.

Signed-off-by: Krzysztof Wilczynski [email protected]

This commit resolves issue where the EFS mount target would be already deleted (e.g. it was deleted outside of Terraform, etc.). Also, correct how values are begin set in the ReadFunc to avoid nil pointer dereference. Signed-off-by: Krzysztof Wilczynski <[email protected]>

kwilczynski · 2016-08-29T14:21:08Z

Resolves #8523.

stack72 · 2016-08-29T14:31:42Z

builtin/providers/aws/resource_aws_efs_mount_target.go

-	d.Set("ip_address", *mt.IpAddress)
-	d.Set("subnet_id", *mt.SubnetId)
-	d.Set("network_interface_id", *mt.NetworkInterfaceId)
+	d.Set("file_system_id", mt.FileSystemId)


Nice changes!

kwilczynski · 2016-08-29T14:32:00Z

Tests are passing:

$ make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSEFSMountTarget'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2016/08/29 15:18:55 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSEFSMountTarget -timeout 120m
=== RUN   TestAccAWSEFSMountTarget_importBasic
--- PASS: TestAccAWSEFSMountTarget_importBasic (256.33s)
=== RUN   TestAccAWSEFSMountTarget_basic
--- PASS: TestAccAWSEFSMountTarget_basic (448.11s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/aws    704.461s

kwilczynski · 2016-08-29T14:32:10Z

@stack72 over to you 🚀

stack72 · 2016-08-29T14:36:18Z

builtin/providers/aws/resource_aws_efs_mount_target.go

@@ -114,8 +114,8 @@ func resourceAwsEfsMountTargetCreate(d *schema.ResourceData, meta interface{}) e
 				return nil, "error", err
 			}

-			if len(resp.MountTargets) < 1 {
-				return nil, "error", fmt.Errorf("EFS mount target %q not found", d.Id())
+			if resp.MountTargets != nil && len(resp.MountTargets) < 1 {


Could this be refactored into a helper method? We tend to repeat this process a lot

You mean a lot in here, or a lot all over the place?

i believe it's used 3 or 4 times in this file right?

stack72 · 2016-08-29T14:38:52Z

Few nit picks inline but this looks pretty good :)

kwilczynski · 2016-08-29T14:58:48Z

@stack72 thank you! I will update as per the suggestions :)

radeksimko · 2016-08-29T17:38:46Z

builtin/providers/aws/resource_aws_efs_mount_target_test.go

+	region := "non-existant-1"
+	id := "fs-123456ab"
+
+	expected := fmt.Sprintf("%s.%s.efs.%s.amazonaws.com", az, id, region)


I think that a black-box approach here would be more useful. i.e. The function should not try to cover all possible cases, because that would mean we'd have to reimplement the function again inside a test - which is what you did here.

e.g. typically you'd be testing that

testedSum(1, 1) == 2 testedSum(3, 4) == 7

rather than

mySumInTest(a, b) == testedSum(a, b)

Having static and simpler tests also makes it easier to spot a bug.

Signed-off-by: Krzysztof Wilczynski <[email protected]>

This commit adds a helper which can be used to check whether the response contains a valid and non-empty list of EFS file system mount targets. Signed-off-by: Krzysztof Wilczynski <[email protected]>

This commit adds a test to verify the condition where the underlying EFS mount target would be deleted and/or disappear resulting in a new resource to be created to replace it. Signed-off-by: Krzysztof Wilczynski <[email protected]>

kwilczynski · 2016-08-29T22:23:33Z

@stack72 @radeksimko over to you 🚀

I've added the helper methods with uni tests, plus an acceptance test to check for non-empty plan (the disappear case), the tests are passing:

(unit)

 make test TEST=./builtin/providers/aws TESTARGS='-run=TestResourceAWSEFSMountTarget_ -v'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2016/08/29 23:10:45 Generated command/internal_plugin_list.go
TF_ACC= go test ./builtin/providers/aws -run=TestResourceAWSEFSMountTarget_ -v -timeout=30s -parallel=4
=== RUN   TestResourceAWSEFSMountTarget_mountTargetDnsName
--- PASS: TestResourceAWSEFSMountTarget_mountTargetDnsName (0.00s)
=== RUN   TestResourceAWSEFSMountTarget_hasEmptyMountTargets
--- PASS: TestResourceAWSEFSMountTarget_hasEmptyMountTargets (0.00s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/aws    0.016s

(acceptance)

$ make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSEFSMountTarget_'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2016/08/29 23:06:27 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSEFSMountTarget_ -timeout 120m
=== RUN   TestAccAWSEFSMountTarget_importBasic
--- PASS: TestAccAWSEFSMountTarget_importBasic (226.74s)
=== RUN   TestAccAWSEFSMountTarget_basic
--- PASS: TestAccAWSEFSMountTarget_basic (420.03s)
=== RUN   TestAccAWSEFSMountTarget_disappears
--- PASS: TestAccAWSEFSMountTarget_disappears (219.64s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/aws    866.430s

stack72 · 2016-08-30T09:42:51Z

Hi @kwilczynski

This looks good :) Thanks for adding the extra test:

% make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSEFSMountTarget_'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2016/08/30 10:26:01 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSEFSMountTarget_ -timeout 120m
=== RUN   TestAccAWSEFSMountTarget_importBasic
--- PASS: TestAccAWSEFSMountTarget_importBasic (227.40s)
=== RUN   TestAccAWSEFSMountTarget_basic
--- PASS: TestAccAWSEFSMountTarget_basic (425.92s)
=== RUN   TestAccAWSEFSMountTarget_disappears
--- PASS: TestAccAWSEFSMountTarget_disappears (219.35s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/aws    872.688s

Paul

ghost · 2020-04-22T02:30:48Z

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.

stack72 reviewed Aug 29, 2016
View reviewed changes

stack72 added bug provider/aws labels Aug 29, 2016

kwilczynski changed the title ~~Handle missing EFS mount target in aws_efs_mount_target.~~ [WIP] provider/aws: Handle missing EFS mount target in aws_efs_mount_target. Aug 29, 2016

kwilczynski force-pushed the fix/handle-missing-mount-aws_efs_mount_target branch 2 times, most recently from 320e026 to a7e5953 Compare August 29, 2016 17:36

radeksimko reviewed Aug 29, 2016
View reviewed changes

kwilczynski force-pushed the fix/handle-missing-mount-aws_efs_mount_target branch from a7e5953 to 09c37b0 Compare August 29, 2016 17:48

Add EFS mount target DNS helper function.

4df4bb6

Signed-off-by: Krzysztof Wilczynski <[email protected]>

kwilczynski force-pushed the fix/handle-missing-mount-aws_efs_mount_target branch from 09c37b0 to 4df4bb6 Compare August 29, 2016 20:16

kwilczynski added 2 commits August 29, 2016 21:17

Add EFS mount target response helper.

918555c

This commit adds a helper which can be used to check whether the response contains a valid and non-empty list of EFS file system mount targets. Signed-off-by: Krzysztof Wilczynski <[email protected]>

Add acceptance test to check for non-empty plan.

88de0c8

This commit adds a test to verify the condition where the underlying EFS mount target would be deleted and/or disappear resulting in a new resource to be created to replace it. Signed-off-by: Krzysztof Wilczynski <[email protected]>

kwilczynski changed the title ~~[WIP] provider/aws: Handle missing EFS mount target in aws_efs_mount_target.~~ provider/aws: Handle missing EFS mount target in aws_efs_mount_target. Aug 29, 2016

stack72 merged commit 6a94f92 into hashicorp:master Aug 30, 2016

stack72 mentioned this pull request Aug 30, 2016

MountTargetNotFound when trying to destroy EFS #8523

Closed

ghost locked and limited conversation to collaborators Apr 22, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

provider/aws: Handle missing EFS mount target in aws_efs_mount_target. #8529

provider/aws: Handle missing EFS mount target in aws_efs_mount_target. #8529

kwilczynski commented Aug 29, 2016

kwilczynski commented Aug 29, 2016

stack72 Aug 29, 2016

kwilczynski commented Aug 29, 2016

kwilczynski commented Aug 29, 2016

stack72 Aug 29, 2016

kwilczynski Aug 29, 2016

stack72 Aug 29, 2016

stack72 commented Aug 29, 2016

kwilczynski commented Aug 29, 2016

radeksimko Aug 29, 2016 •

edited

Loading

kwilczynski commented Aug 29, 2016

stack72 commented Aug 30, 2016

ghost commented Apr 22, 2020

provider/aws: Handle missing EFS mount target in aws_efs_mount_target. #8529

provider/aws: Handle missing EFS mount target in aws_efs_mount_target. #8529

Conversation

kwilczynski commented Aug 29, 2016

kwilczynski commented Aug 29, 2016

stack72 Aug 29, 2016

Choose a reason for hiding this comment

kwilczynski commented Aug 29, 2016

kwilczynski commented Aug 29, 2016

stack72 Aug 29, 2016

Choose a reason for hiding this comment

kwilczynski Aug 29, 2016

Choose a reason for hiding this comment

stack72 Aug 29, 2016

Choose a reason for hiding this comment

stack72 commented Aug 29, 2016

kwilczynski commented Aug 29, 2016

radeksimko Aug 29, 2016 • edited Loading

Choose a reason for hiding this comment

kwilczynski commented Aug 29, 2016

stack72 commented Aug 30, 2016

ghost commented Apr 22, 2020

radeksimko Aug 29, 2016 •

edited

Loading