Workspaces Directory Access Properties by Tensho · Pull Request #16688 · hashicorp/terraform-provider-aws

Tensho · 2020-12-10T13:10:50Z

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:

aws_workspaces_directory: Add access properties
data aws_workspaces_directory: Add access properties

Unit Tests

$ make test TEST=./aws TESTARGS='-run=TestExpandWorkspaceAccessProperties'
==> Checking that code complies with gofmt requirements...
go test ./aws -run=TestExpandWorkspaceAccessProperties -timeout=5m -parallel=4
ok  	github.com/terraform-providers/terraform-provider-aws/aws	5.380s

$ make test TEST=./aws TESTARGS='-run=TestFlattenWorkspaceAccessProperties'
==> Checking that code complies with gofmt requirements...
go test ./aws -run=TestFlattenWorkspaceAccessProperties -timeout=5m -parallel=4
ok  	github.com/terraform-providers/terraform-provider-aws/aws	5.393s

Acceptance Tests

Resource

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

Data Source

$ 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 20 -run=TestAccDataSourceAwsWorkspacesDirectory_basic -timeout 120m
=== RUN   TestAccDataSourceAwsWorkspacesDirectory_basic
=== PAUSE TestAccDataSourceAwsWorkspacesDirectory_basic
=== CONT  TestAccDataSourceAwsWorkspacesDirectory_basic
--- PASS: TestAccDataSourceAwsWorkspacesDirectory_basic (42.77s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	45.813s

Fixes

Fix resource name interpolation into tags in test configs

References

ModifyWorkspaceAccessProperties API

DrFaust92 · 2020-12-10T14:49:28Z

lets keep the API structure here and use string and validate values.

I don't mind, but in this case self_service_permissions block attributes should be changed too as far as they have a similar semantic. If it makes sense to you, please could you suggest the best way to handle self_service_permissions backward compatibility?

It may require a breaking change in the future but lets keep to the scope of this PR as this is a new block added. from my experience maintainers usually prefer(and i agree) to keep the original type used in the SDK. we dont know the reason why this was not bool and in the future this opens a door for more values (as unlikely as it looks now) and it means breaking this interface or adding extra arguments.

Make total sense to me. Thank you, Ilia 🙇 Revamping...

Indeed API doesn't specify the defaults, here is my best effort to guess the sensible values. Is it OK to rely on the current non-specified default values from API response? I mean they may be changed in the future by AWS.

Yeah, they are defacto defaults. they still may change but more unlikely. just adding the test to basic would be fine we can use optional with default. i would avoid computed to catch config drift. so the way it was before is good just keep the check for basic test as well.

sorry for the multiple cycles if i wasnt clear.

No worries, I appreciate your thoughtful review 🙇

Removed Default schema options in favor of API default values

Added default values assertions to the basic test

Could you elaborate on Computed schema option removal? If I understand correctly, user may skip workspace_access_properties attribute configuration and API returns default values, which are populated to state on Read. This kind of behavior should be marked as Computed. Also, basic acc test should contain assertions exactly for Computed + Optional attributes.

Doesnt the current setting cause config drift?
there should be a computed: true or default value.

workspace_access_properties doesn't have a default value right now. The plan is empty at the end of each test step.

DrFaust92 · 2020-12-10T17:55:58Z

Whats the default based on?
https://docs.aws.amazon.com/workspaces/latest/api/API_WorkspaceAccessProperties.html doesnt specify a value.
if this is indeed the defaults returned lets also add a check to the base test.

DrFaust92 · 2020-12-11T16:22:54Z

data source no need for optional

Good point, fixed. I've removed MaxItems too.

DrFaust92 · 2020-12-11T16:27:09Z

Doesnt the current setting cause config drift?
there should be a computed: true or default value.

DrFaust92

Looks Good to me.

breathingdust · 2021-01-13T22:50:22Z

LGTM 🚀 Thanks @Tensho

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

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

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 20 -run=TestAccDataSourceAwsWorkspacesDirectory_basic -timeout 120m
=== RUN   TestAccDataSourceAwsWorkspacesDirectory_basic
=== PAUSE TestAccDataSourceAwsWorkspacesDirectory_basic
=== CONT  TestAccDataSourceAwsWorkspacesDirectory_basic
--- PASS: TestAccDataSourceAwsWorkspacesDirectory_basic (613.22s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	615.085s

Acceptance Tests fail in GovCloud due to SimpleAD not being available, there is code to handle this occurrence in the testAccPreCheckAWSDirectoryServiceSimpleDirectory precheck but it appears not to be working, I will create an issue to track this and link back to this issue.

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 20 -run=TestAccDataSourceAwsWorkspacesDirectory_basic -timeout 120m
=== RUN   TestAccDataSourceAwsWorkspacesDirectory_basic
=== PAUSE TestAccDataSourceAwsWorkspacesDirectory_basic
=== CONT  TestAccDataSourceAwsWorkspacesDirectory_basic
    data_source_aws_workspaces_directory_test.go:17: Step 1/1 error: Error running apply: 2021/01/13 14:34:41 [DEBUG] Using modified User-Agent: Terraform/0.12.29 HashiCorp-terraform-exec/0.12.0

        Error: ClientException: Simple AD directory creation is currently not supported in this region. : RequestId: e70ba230-04f3-4f62-acad-6e2614a0c687 : RequestId: e70ba230-04f3-4f62-acad-6e2614a0c687
        {
          RespMetadata: {
            StatusCode: 400,
            RequestID: "e70ba230-04f3-4f62-acad-6e2614a0c687"
          },
          Message_: "Simple AD directory creation is currently not supported in this region. : RequestId: e70ba230-04f3-4f62-acad-6e2614a0c687 : RequestId: e70ba230-04f3-4f62-acad-6e2614a0c687",
          RequestId: "e70ba230-04f3-4f62-acad-6e2614a0c687"
        }


--- FAIL: TestAccDataSourceAwsWorkspacesDirectory_basic (8.80s)
FAIL
FAIL	github.com/terraform-providers/terraform-provider-aws/aws	10.774s
FAIL
make: *** [testacc] Error 1

ghost · 2021-01-15T00:17:38Z

This has been released in version 3.24.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 · 2021-02-13T17:11:10Z

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!

Tensho requested a review from a team as a code owner December 10, 2020 13:10

ghost added size/L Managed by automation to categorize the size of a PR. service/workspaces Issues and PRs that pertain to the workspaces service. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. labels Dec 10, 2020

github-actions Bot added the needs-triage Waiting for first response or review from a maintainer. label Dec 10, 2020

Tensho force-pushed the workspaces-directory-access-properties branch 2 times, most recently from 0f5a8bd to ddbda6b Compare December 10, 2020 13:35

ghost added size/XL Managed by automation to categorize the size of a PR. documentation Introduces or discusses updates to documentation. and removed size/L Managed by automation to categorize the size of a PR. labels Dec 10, 2020

Tensho changed the title ~~[aws_workspaces_directory] Add Access Properties~~ [aws_workspaces_directory] Workspace Access Properties Dec 10, 2020

Tensho changed the title ~~[aws_workspaces_directory] Workspace Access Properties~~ Workspace Directory Access Properties Dec 10, 2020

Tensho changed the title ~~Workspace Directory Access Properties~~ Workspaces Directory Access Properties Dec 10, 2020

DrFaust92 suggested changes Dec 10, 2020

View reviewed changes

DrFaust92 added enhancement Requests to existing resources that expand the functionality or scope. and removed needs-triage Waiting for first response or review from a maintainer. labels Dec 10, 2020

DrFaust92 suggested changes Dec 11, 2020

View reviewed changes

Tensho force-pushed the workspaces-directory-access-properties branch 2 times, most recently from 5af9c55 to 5daa021 Compare December 17, 2020 12:30

DrFaust92 approved these changes Dec 17, 2020

View reviewed changes

Tensho force-pushed the workspaces-directory-access-properties branch from 5daa021 to 6f2cda0 Compare December 28, 2020 10:48

Tensho added 7 commits January 11, 2021 13:18

aws_workspaces_directory: Add access properties

1ea2aff

data aws_workspaces_directory: Add access properties

dda6087

data aws_workspaces_directory: Add missed workspace_creation_properties

2656870

Fix resource name interpolation into tags in test configs

0b2efc3

Shift workspace_access_attributes from bool to string type

6a78fdc

Test workspace_access_properties default values

ea785d2

Add acctest Name tag to all test workspaces

412f7de

Remove Otional and MaxItems from data source schema

408b892

Tensho force-pushed the workspaces-directory-access-properties branch from 6f2cda0 to 408b892 Compare January 11, 2021 11:18

Tensho self-assigned this Jan 12, 2021

breathingdust merged commit 3de6fc4 into hashicorp:master Jan 13, 2021

github-actions Bot added this to the v3.24.0 milestone Jan 13, 2021

breathingdust added a commit that referenced this pull request Jan 13, 2021

Update CHANGELOG.md for #16688

44eb67e

Tensho deleted the workspaces-directory-access-properties branch January 13, 2021 23:11

breathingdust mentioned this pull request Jan 14, 2021

tests/workspaces - Add GovCloud related prechecks to workspaces directory datasource #17115

Merged

ghost locked as resolved and limited conversation to collaborators Feb 13, 2021

Conversation

Tensho commented Dec 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Community Note

Unit Tests

Acceptance Tests

Resource

Data Source

Fixes

References

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tensho Dec 10, 2020 • 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!

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!

Tensho Dec 10, 2020 • 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!

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!

Tensho Dec 14, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

DrFaust92 left a comment

Choose a reason for hiding this comment

Uh oh!

breathingdust commented Jan 13, 2021

Uh oh!

ghost commented Jan 15, 2021

Uh oh!

ghost commented Feb 13, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Tensho commented Dec 10, 2020 •

edited

Loading

Tensho Dec 10, 2020 •

edited

Loading

Tensho Dec 10, 2020 •

edited

Loading

Tensho Dec 14, 2020 •

edited

Loading