r/aws_workspaces_directory: Fix panic if previously registered directory is no longer registered #11837

ewbankkit · 2020-01-31T16:33:18Z

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_workspaces_directory: Fix panic if previously registered directory is no longer registered

Output from acceptance testing:

$ make testacc TEST=./aws/ TESTARGS='-run=TestAccAwsWorkspacesDirectory'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws/ -v -count 1 -parallel 20 -run=TestAccAwsWorkspacesDirectory -timeout 120m
=== RUN   TestAccAwsWorkspacesDirectory
=== RUN   TestAccAwsWorkspacesDirectory/basic
=== RUN   TestAccAwsWorkspacesDirectory/disappears
=== RUN   TestAccAwsWorkspacesDirectory/subnetIds
--- PASS: TestAccAwsWorkspacesDirectory (2056.41s)
    --- PASS: TestAccAwsWorkspacesDirectory/basic (662.88s)
    --- PASS: TestAccAwsWorkspacesDirectory/disappears (784.63s)
    --- PASS: TestAccAwsWorkspacesDirectory/subnetIds (608.90s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	2056.456s

…ory is no longer registered.

ewbankkit · 2020-02-01T21:57:07Z

Verified that aws_workspaces_ip_group resource handles the similar case correctly
https://github.com/terraform-providers/terraform-provider-aws/blob/31e3d47fbfd65d39d61a59b228626e34e394d330/aws/resource_aws_workspaces_ip_group.go#L79-L90

Tensho

Great stuff 👍 TBH, I even didn't think about the case, when a TF managed resource could be deleted manually and that should be handled with the resource drop from the state on Read. I will add a similar check to other workspace resources I've contributed to. Thank you 🙇

Tensho · 2020-02-03T13:48:25Z

aws/resource_aws_workspaces_directory.go

 	d.Set("directory_id", dir.DirectoryId)
 	d.Set("subnet_ids", dir.SubnetIds)
-	d.Set("self_service_permissions", flattenSelfServicePermissions(dir.SelfservicePermissions))
+	if err := d.Set("self_service_permissions", flattenSelfServicePermissions(dir.SelfservicePermissions)); err != nil {


Please, could you explain what are the circumstances when Set() may return an error?

If the type of the data passed to Set() isn't compatible with the schema type declared for the attribute it's possible to get an error (e.g. passing a map[string]interface{} when a *schema.Set is expected.
To be safe, anytime a "complex" type is being set we should really check errors (although there are many places in the code that isn't done).
Silent failure can lead to attributes not being set correctly in state.

Got it, thank you 🙇

Tensho · 2020-02-03T13:49:02Z

aws/resource_aws_workspaces_directory.go

@@ -205,7 +210,7 @@ func resourceAwsWorkspacesDirectoryDelete(d *schema.ResourceData, meta interface

 	err := workspacesDirectoryDelete(d.Id(), conn)
 	if err != nil {
-		return err
+		return fmt.Errorf("error deleting workspaces directory (%s): %s", d.Id(), err)


Tensho · 2020-02-03T13:55:23Z

aws/resource_aws_workspaces_directory_test.go

@@ -201,6 +232,8 @@ func testAccCheckAwsWorkspacesDirectoryExists(n string) resource.TestCheckFunc {
 		}



Maybe it's worth to check the length here too, isn't it?

Tensho · 2020-02-03T14:09:59Z

aws/resource_aws_workspaces_directory_test.go

-func testAccCheckAwsWorkspacesDirectoryExists(n string) resource.TestCheckFunc {
+func testAccCheckAwsWorkspacesDirectoryDisappears(v *workspaces.WorkspaceDirectory) resource.TestCheckFunc {
+	return func(s *terraform.State) error {
+		return workspacesDirectoryDelete(aws.StringValue(v.DirectoryId), testAccProvider.Meta().(*AWSClient).workspacesconn)


It's not clear from the test helper name that it just deletes the resource under the test and doesn't check anything actually. But the framework expects TestCheckFunc type, so nothing better to do here 🤷‍♂

Tensho · 2020-02-03T14:13:32Z

aws/resource_aws_workspaces_directory_test.go

@@ -66,6 +67,7 @@ func TestAccAwsWorkspacesDirectory(t *testing.T) {
 }

 func testAccAwsWorkspacesDirectory_basic(t *testing.T) {
+	var v workspaces.WorkspaceDirectory


Good point 👍 I guess this should be added in a separate PR with the actual usage of v in checks. I know it's a good pattern to test TF state along with the actual resource one, however, it doesn't make sense without those tests.

bflad

Thanks for this fix, looks good to me, @ewbankkit 🚀

Output from acceptance testing:

--- PASS: TestAccAwsWorkspacesDirectory (1658.93s)
    --- PASS: TestAccAwsWorkspacesDirectory/basic (524.67s)
    --- PASS: TestAccAwsWorkspacesDirectory/disappears (572.09s)
    --- PASS: TestAccAwsWorkspacesDirectory/subnetIds (562.17s)

ghost · 2020-02-28T14:37:15Z

This has been released in version 2.51.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-03-27T17:10:18Z

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!

r/aws_workspaces_directory: Fix panic if previously registered direct…

d45f552

…ory is no longer registered.

ewbankkit requested a review from a team January 31, 2020 16:33

This was referenced Feb 1, 2020

New Resource: aws_workspaces_workspace #11608

Merged

New Data Source: aws_workspaces_image #11428

Merged

Tensho reviewed Feb 3, 2020

View reviewed changes

bflad added bug Addresses a defect in current functionality. crash Results from or addresses a Terraform crash or kernel panic. and removed needs-triage Waiting for first response or review from a maintainer. labels Feb 11, 2020

bflad self-assigned this Feb 11, 2020

bflad added this to the v2.51.0 milestone Feb 23, 2020

bflad approved these changes Feb 23, 2020

View reviewed changes

bflad merged commit 8271366 into hashicorp:master Feb 23, 2020

bflad added a commit that referenced this pull request Feb 23, 2020

Update CHANGELOG for #11837

44f3ed9

ewbankkit deleted the issue-11814 branch February 23, 2020 22:57

ghost locked and limited conversation to collaborators Mar 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

r/aws_workspaces_directory: Fix panic if previously registered directory is no longer registered #11837

r/aws_workspaces_directory: Fix panic if previously registered directory is no longer registered #11837

ewbankkit commented Jan 31, 2020

ewbankkit commented Feb 1, 2020

Tensho left a comment

Tensho Feb 3, 2020

ewbankkit Feb 3, 2020

Tensho Mar 6, 2020

Tensho Feb 3, 2020

Tensho Feb 3, 2020

Tensho Feb 3, 2020

Tensho Feb 3, 2020

bflad left a comment

ghost commented Feb 28, 2020

ghost commented Mar 27, 2020

		@@ -201,6 +232,8 @@ func testAccCheckAwsWorkspacesDirectoryExists(n string) resource.TestCheckFunc {
		}

r/aws_workspaces_directory: Fix panic if previously registered directory is no longer registered #11837

r/aws_workspaces_directory: Fix panic if previously registered directory is no longer registered #11837

Conversation

ewbankkit commented Jan 31, 2020

Community Note

ewbankkit commented Feb 1, 2020

Tensho left a comment

Choose a reason for hiding this comment

Tensho Feb 3, 2020

Choose a reason for hiding this comment

ewbankkit Feb 3, 2020

Choose a reason for hiding this comment

Tensho Mar 6, 2020

Choose a reason for hiding this comment

Tensho Feb 3, 2020

Choose a reason for hiding this comment

Tensho Feb 3, 2020

Choose a reason for hiding this comment

Tensho Feb 3, 2020

Choose a reason for hiding this comment

Tensho Feb 3, 2020

Choose a reason for hiding this comment

bflad left a comment

Choose a reason for hiding this comment

ghost commented Feb 28, 2020

ghost commented Mar 27, 2020