Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

aws_workspaces_directory: Workspaces Creation Properties #14577

Merged
merged 6 commits into from
Oct 14, 2020

Conversation

Tensho
Copy link
Contributor

@Tensho Tensho commented Aug 11, 2020

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

Closes #12737
Closes #14276

Release note for CHANGELOG:

aws_workspaces_directory: Add workspaces creation properties (#14577)

Acceptance Tests

resource aws_workspaces_directory

$ make testacc TEST=./aws TESTARGS='-run=TestAccAwsWorkspacesDirectory'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 1 -run=TestAccAwsWorkspacesDirectory -timeout 120m
=== RUN   TestAccAwsWorkspacesDirectory_basic
=== PAUSE TestAccAwsWorkspacesDirectory_basic
=== RUN   TestAccAwsWorkspacesDirectory_disappears
=== PAUSE TestAccAwsWorkspacesDirectory_disappears
=== RUN   TestAccAwsWorkspacesDirectory_subnetIds
=== PAUSE TestAccAwsWorkspacesDirectory_subnetIds
=== RUN   TestAccAwsWorkspacesDirectory_tags
=== PAUSE TestAccAwsWorkspacesDirectory_tags
=== RUN   TestAccAwsWorkspacesDirectory_selfServicePermissions
=== PAUSE TestAccAwsWorkspacesDirectory_selfServicePermissions
=== RUN   TestAccAwsWorkspacesDirectory_workspaceCreationProperties
=== PAUSE TestAccAwsWorkspacesDirectory_workspaceCreationProperties
=== CONT  TestAccAwsWorkspacesDirectory_basic
--- PASS: TestAccAwsWorkspacesDirectory_basic (33.76s)
=== CONT  TestAccAwsWorkspacesDirectory_selfServicePermissions
--- PASS: TestAccAwsWorkspacesDirectory_selfServicePermissions (28.62s)
=== CONT  TestAccAwsWorkspacesDirectory_workspaceCreationProperties
--- PASS: TestAccAwsWorkspacesDirectory_workspaceCreationProperties (35.64s)
=== CONT  TestAccAwsWorkspacesDirectory_subnetIds
--- PASS: TestAccAwsWorkspacesDirectory_subnetIds (31.42s)
=== CONT  TestAccAwsWorkspacesDirectory_tags
--- PASS: TestAccAwsWorkspacesDirectory_tags (67.84s)
=== CONT  TestAccAwsWorkspacesDirectory_disappears
    TestAccAwsWorkspacesDirectory_disappears: resource_aws_workspaces_directory_test.go:117: [INFO] Got non-empty plan, as expected
    TestAccAwsWorkspacesDirectory_disappears: resource_aws_workspaces_directory_test.go:117: [INFO] Got non-empty plan, as expected
--- PASS: TestAccAwsWorkspacesDirectory_disappears (29.26s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	1066.828s

data aws_workspaces_directory

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

References

@Tensho Tensho requested a review from a team August 11, 2020 20:09
@ghost ghost added size/XL Managed by automation to categorize the size of a PR. needs-triage Waiting for first response or review from a maintainer. documentation Introduces or discusses updates to documentation. service/workspaces Issues and PRs that pertain to the workspaces service. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. labels Aug 11, 2020
@Tensho Tensho force-pushed the workspaces-creation-properties branch 4 times, most recently from b1c175f to 3bf186a Compare August 14, 2020 12:43
@Tensho Tensho force-pushed the workspaces-creation-properties branch from 3bf186a to 69a9a22 Compare September 23, 2020 10:23
@ghost ghost added size/L Managed by automation to categorize the size of a PR. and removed size/XL Managed by automation to categorize the size of a PR. labels Sep 23, 2020
@Tensho Tensho force-pushed the workspaces-creation-properties branch from 69a9a22 to b79c6aa Compare September 23, 2020 10:29
@ewbankkit
Copy link
Contributor

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

@Tensho Tensho force-pushed the workspaces-creation-properties branch from b79c6aa to b87c6a3 Compare September 24, 2020 18:48
@ewbankkit
Copy link
Contributor

@DrFaust92 Could you review also? Thanks.

@ewbankkit ewbankkit removed the needs-triage Waiting for first response or review from a maintainer. label Sep 25, 2020
Copy link
Collaborator

@DrFaust92 DrFaust92 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comments

aws/resource_aws_workspaces_directory.go Show resolved Hide resolved
aws/resource_aws_workspaces_directory_test.go Outdated Show resolved Hide resolved
website/docs/r/workspaces_directory.html.markdown Outdated Show resolved Hide resolved
@Tensho Tensho force-pushed the workspaces-creation-properties branch from b87c6a3 to 4631622 Compare September 30, 2020 11:19
@ghost ghost added size/XL Managed by automation to categorize the size of a PR. and removed size/L Managed by automation to categorize the size of a PR. labels Sep 30, 2020
@Tensho Tensho force-pushed the workspaces-creation-properties branch 3 times, most recently from 0f5b88b to dcbf7af Compare September 30, 2020 13:41
Copy link
Collaborator

@DrFaust92 DrFaust92 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

quick comment and ill start running tests to verify.

@@ -120,8 +153,8 @@ func resourceAwsWorkspacesDirectoryCreate(d *schema.ResourceData, meta interface

input := &workspaces.RegisterWorkspaceDirectoryInput{
DirectoryId: aws.String(directoryId),
EnableSelfService: aws.Bool(false), // this is handled separately below
EnableWorkDocs: aws.Bool(false),
EnableSelfService: aws.Bool(true), // this is handled separately below
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not familiar with workspapces, whats the reason these are change to true?

Copy link
Contributor Author

@Tensho Tensho Oct 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EnableSelfService attribute determines self-service capabilities enable/disable status for all workspaces in the particular workspaces directory. For example, with enabled self-service permissions, workspaces administrator can allow workspace user (client) to change the compute type or volume size without administrator's involvement (on-demand).

Here I've changed it from false to true for the same reason as EnableWorkDocs attribute below – I thought it couldn't be changed after directory creation. However, self-service permissions can be changed both with ModifySelfservicePermissions API call and AWS Management Console at any time. I'll return it back to false to preserve backward compatibility. Thank you for your thorough review.

P.S. If EnableSelfService attribute is set to false, it doesn't mean all attributes behind it are set to false too.

@Tensho Tensho force-pushed the workspaces-creation-properties branch from 6bd63f8 to 0e8d1dc Compare October 1, 2020 08:31
@DrFaust92 DrFaust92 self-assigned this Oct 1, 2020
@DrFaust92
Copy link
Collaborator

getting

resource_aws_workspaces_directory_test.go:295: skipping acceptance test: required IAM role "workspaces_DefaultRole" is not present

@DrFaust92 DrFaust92 removed their assignment Oct 1, 2020
@Tensho
Copy link
Contributor Author

Tensho commented Oct 2, 2020

@DrFaust92 Please check out "AWS WorkSpaces Example" page. workspaces_DefaultRole IAM role is managed by AWS (I guess it's called service-linked role). It's created automatically in AWS account on the first workspaces directory registration. For a brand new AWS account, this role is absent.

Do you think we should manage this role in test within Terraform code?

References

@bflad
Copy link
Contributor

bflad commented Oct 2, 2020

@Tensho I believe we opted out of managing that IAM Role in the test configurations since it would force us to serialize all the tests, which already take a very long time.

@DrFaust92
Copy link
Collaborator

getting errors:

        stderr:
        
        Error: error deleting Directory Service Directory (d-9267091723): ClientException: Cannot delete the directory because it still has authorized applications. : RequestId: 50a70a12-35e2-40a6-b486-1d91ce8caf67
        {
          RespMetadata: {
            StatusCode: 400,
            RequestID: "50a70a12-35e2-40a6-b486-1d91ce8caf67"
          },
          Message_: "Cannot delete the directory because it still has authorized applications. : RequestId: 50a70a12-35e2-40a6-b486-1d91ce8caf67",
          RequestId: "50a70a12-35e2-40a6-b486-1d91ce8caf67"
        }
        
        
        
--- FAIL: TestAccAwsWorkspacesDirectory_tags (487.94s)

@Tensho Tensho force-pushed the workspaces-creation-properties branch 2 times, most recently from 31190e5 to c783478 Compare October 8, 2020 20:15
@DrFaust92
Copy link
Collaborator

Related #14332

@DrFaust92
Copy link
Collaborator

LGTM (except lint error)

--- PASS: TestAccAwsWorkspacesDirectory_workspaceCreationProperties (656.05s)
--- PASS: TestAccAwsWorkspacesDirectory_basic (669.32s)
--- PASS: TestAccAwsWorkspacesDirectory_subnetIds (670.19s)
--- PASS: TestAccAwsWorkspacesDirectory_selfServicePermissions (680.34s)
--- PASS: TestAccAwsWorkspacesDirectory_disappears (721.30s)
--- PASS: TestAccAwsWorkspacesDirectory_tags (790.78s)

@Tensho Tensho force-pushed the workspaces-creation-properties branch from c783478 to e5910ae Compare October 12, 2020 12:36
@Tensho Tensho force-pushed the workspaces-creation-properties branch from 12c15e2 to 266f1fb Compare October 12, 2020 13:18
Copy link
Collaborator

@DrFaust92 DrFaust92 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@breathingdust
Copy link
Member

LGTM 🚀 Thanks @Tensho

Verified Acceptance Tests in Commercial (us-west-2)

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_basic
=== PAUSE TestAccAwsWorkspacesDirectory_basic
=== RUN   TestAccAwsWorkspacesDirectory_disappears
=== PAUSE TestAccAwsWorkspacesDirectory_disappears
=== RUN   TestAccAwsWorkspacesDirectory_subnetIds
=== PAUSE TestAccAwsWorkspacesDirectory_subnetIds
=== RUN   TestAccAwsWorkspacesDirectory_tags
=== PAUSE TestAccAwsWorkspacesDirectory_tags
=== RUN   TestAccAwsWorkspacesDirectory_selfServicePermissions
=== PAUSE TestAccAwsWorkspacesDirectory_selfServicePermissions
=== RUN   TestAccAwsWorkspacesDirectory_workspaceCreationProperties
=== PAUSE TestAccAwsWorkspacesDirectory_workspaceCreationProperties
=== CONT  TestAccAwsWorkspacesDirectory_basic
=== CONT  TestAccAwsWorkspacesDirectory_selfServicePermissions
=== CONT  TestAccAwsWorkspacesDirectory_subnetIds
=== CONT  TestAccAwsWorkspacesDirectory_tags
=== CONT  TestAccAwsWorkspacesDirectory_workspaceCreationProperties
=== CONT  TestAccAwsWorkspacesDirectory_disappears
--- PASS: TestAccAwsWorkspacesDirectory_selfServicePermissions (635.80s)
--- PASS: TestAccAwsWorkspacesDirectory_workspaceCreationProperties (639.71s)
--- PASS: TestAccAwsWorkspacesDirectory_basic (644.66s)
--- PASS: TestAccAwsWorkspacesDirectory_disappears (687.79s)
--- PASS: TestAccAwsWorkspacesDirectory_subnetIds (695.49s)
--- PASS: TestAccAwsWorkspacesDirectory_tags (737.19s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	739.056s

Note that this functionality is not available in GovCloud due to Simple AD Creation not being available.

@breathingdust breathingdust added this to the v3.11.0 milestone Oct 14, 2020
@breathingdust breathingdust merged commit a6edb1a into hashicorp:master Oct 14, 2020
breathingdust added a commit that referenced this pull request Oct 14, 2020
@Tensho Tensho deleted the workspaces-creation-properties branch October 14, 2020 09:18
@ghost
Copy link

ghost commented Oct 15, 2020

This has been released in version 3.11.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
Copy link

ghost commented Nov 13, 2020

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!

@ghost ghost locked as resolved and limited conversation to collaborators Nov 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation Introduces or discusses updates to documentation. service/workspaces Issues and PRs that pertain to the workspaces service. size/XL Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure.
Projects
None yet
5 participants