r/aws_connect_instance - add new resource

[abebars]

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

Relates #16392

Release note for CHANGELOG:

resource_aws_connect_instance - add new resource

Output from acceptance testing:

$  make testacc TEST=./aws/ TESTARGS='-run=TestAccAwsConnectInstance_' 

TF_ACC=1 go test ./aws -v -count 1 -parallel 1 -run=TestAccAwsConnectInstance_ -timeout 120m
=== RUN   TestAccAwsConnectInstance_basic
=== PAUSE TestAccAwsConnectInstance_basic
=== RUN   TestAccAwsConnectInstance_custom
=== PAUSE TestAccAwsConnectInstance_custom
=== RUN   TestAccAwsConnectInstance_directory
=== PAUSE TestAccAwsConnectInstance_directory
=== RUN   TestAccAwsConnectInstance_saml
=== PAUSE TestAccAwsConnectInstance_saml
=== CONT  TestAccAwsConnectInstance_basic
--- PASS: TestAccAwsConnectInstance_basic (108.62s)
=== CONT  TestAccAwsConnectInstance_saml
--- PASS: TestAccAwsConnectInstance_saml (107.18s)
=== CONT  TestAccAwsConnectInstance_directory
--- PASS: TestAccAwsConnectInstance_directory (1011.30s)
=== CONT  TestAccAwsConnectInstance_custom
--- PASS: TestAccAwsConnectInstance_custom (93.667s)
PASS
ok    github.com/terraform-providers/terraform-provider-aws/aws       1318.638s

$  make testacc TEST=./aws/ TESTARGS='-run=TestAccAwsConnectInstanceDataSource_'

=== RUN   TestAccAwsConnectInstanceDataSource_basic
=== PAUSE TestAccAwsConnectInstanceDataSource_basic
=== RUN   TestAccAwsConnectInstanceDataSource_alias
=== PAUSE TestAccAwsConnectInstanceDataSource_alias
=== CONT  TestAccAwsConnectInstanceDataSource_basic
--- PASS: TestAccAwsConnectInstanceDataSource_basic (119.30s)
=== CONT  TestAccAwsConnectInstanceDataSource_alias
--- PASS: TestAccAwsConnectInstanceDataSource_alias (107.08s)
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws       229.736s

$   SWEEPARGS=-sweep-run=aws_connect_instance make sweep

go test ./aws -v -sweep=us-east-1,us-west-2 -sweep-run=aws_connect_instance -timeout 60m
2020/12/16 01:16:44 [DEBUG] Running Sweepers for region (us-east-1):
2020/12/16 01:16:44 [DEBUG] Running Sweeper (aws_connect_instance) in region (us-east-1)
2020/12/16 01:16:44 [INFO] AWS Auth provider used: "EnvProvider"
2020/12/16 01:16:44 [DEBUG] Trying to get account information via sts:GetCallerIdentity
2020/12/16 01:16:44 [DEBUG] Trying to get account information via sts:GetCallerIdentity
2020/12/16 01:16:45 Sweeper Tests ran successfully:
        - aws_connect_instance
2020/12/16 01:16:45 [DEBUG] Running Sweepers for region (us-west-2):
2020/12/16 01:16:45 [DEBUG] Running Sweeper (aws_connect_instance) in region (us-west-2)
2020/12/16 01:16:45 [INFO] AWS Auth provider used: "EnvProvider"
2020/12/16 01:16:45 [DEBUG] Trying to get account information via sts:GetCallerIdentity
2020/12/16 01:16:45 [DEBUG] Trying to get account information via sts:GetCallerIdentity
2020/12/16 01:16:46 Sweeper Tests ran successfully:
        - aws_connect_instance
ok      github.com/terraform-providers/terraform-provider-aws/aws       5.580s

[github-actions]

Welcome @abebars 👋

It looks like this is your first Pull Request submission to the Terraform AWS Provider! If you haven’t already done so please make sure you have checked out our CONTRIBUTING guide and FAQ to make sure your contribution is adhering to best practice and has all the necessary elements in place for a successful approval.

Also take a look at our FAQ which details how we prioritize Pull Requests for inclusion.

Thanks again, and welcome to the community! 😃

[thornleyk]

Great work! I like the way you dealt with the attribute vs instance value dichotomy the dataResourceConnectInstanceAttributesMapping is also a nice way of dealing with NVP api calls. I think there is a bunch of flags that need to be collapsed into nested struct so that simple instance creations can use the minimal, I think identity probably needs to be treated the same way as its a very binary capability

[abebars]

Great work! I like the way you dealt with the attribute vs instance value dichotomy the dataResourceConnectInstanceAttributesMapping is also a nice way of dealing with NVP api calls. I think there is a bunch of flags that need to be collapsed into nested struct so that simple instance creations can use the minimal, I think identity probably needs to be treated the same way as its a very binary capability

I had similar thoughts about the creation process. I tried to keep the same contract for the API and especially because these fields can cause a recreation for the resource. One issue with having the identity implicit that changing it later will force a new resource because you can't update this config after the instance creation.

[abebars]

@bflad if you get a chance to look into this PR .. that will be great!

[thornleyk]

Great work Ahmed, excited to add my connect flow next!!

[nbmustafa]

Hello guys, any reason why this is still not been merged, is there a way to drag in someone from Terraform to review this great work done by @abebars and the other PR that @thornleyk raised?

[abebars]

@ewbankkit or @bflad if you have a moment to take a look at this PR, Thank you

[DrFaust92]

First round of comments:

while not a blocker it would be nice to get tagging support here as well. see https://github.com/hashicorp/terraform-provider-aws/blob/main/docs/contributing/running-and-writing-acceptance-tests.md#randomized-naming

[Matausi29]

Approve

[anGie44]

Hi @abebars @AdamTylerLynch , thank you for contributing to this new resource! Overall, changes looked great! Just a couple updates made to the data source (c313e27) and resource (08b6773) including updates to paginated methods available in the AWS Go SDK and documentation describing resource import and attributes e.g. Required where needed. I've rebased as well to ensure the CI error related to tfplugindocs was addressed as merging in main was previously causing this.

Output of acceptance tests:

--- PASS: TestAccAwsConnectInstance_serial (0.00s)
    --- PASS: TestAccAwsConnectInstance_serial/basic (100.52s)
    --- PASS: TestAccAwsConnectInstance_serial/saml (104.80s)
    --- PASS: TestAccAwsConnectInstance_serial/directory (827.04s)

--- PASS: TestAccAwsConnectInstanceDataSource_basic (172.59s)

[anGie44]

Small data source test error addressed in 823d1a7

Output of test:

--- PASS: TestAccAwsConnectInstanceDataSource_basic (124.77s)

[github-actions]

This functionality has been released in v3.60.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. Thank you!

[github-actions]

I'm going to lock this pull request 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 related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.