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

r/aws_cognito_user_pool_domain - add update functionality for certificate_arn #25275

Closed
wants to merge 62 commits into from

Conversation

austinvalle
Copy link
Member

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 #14733

Output from acceptance testing:

 $ make testacc TESTS=TestAccCognitoIDPUserPoolDomain PKG=cognitoidp
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./internal/service/cognitoidp/... -v -count 1 -parallel 20 -run='TestAccCognitoIDPUserPoolDomain'  -timeout 180m
go: downloading github.com/aws/aws-sdk-go-v2/service/route53domains v1.12.6
go: downloading github.com/aws/aws-sdk-go-v2/feature/ec2/imds v1.12.6
go: downloading github.com/aws/aws-sdk-go-v2/internal/configsources v1.1.12
go: downloading github.com/aws/aws-sdk-go-v2/internal/endpoints/v2 v2.4.6
=== RUN   TestAccCognitoIDPUserPoolDomain_basic
=== PAUSE TestAccCognitoIDPUserPoolDomain_basic
=== RUN   TestAccCognitoIDPUserPoolDomain_custom
=== PAUSE TestAccCognitoIDPUserPoolDomain_custom
=== RUN   TestAccCognitoIDPUserPoolDomain_customCertUpdate
=== PAUSE TestAccCognitoIDPUserPoolDomain_customCertUpdate
=== RUN   TestAccCognitoIDPUserPoolDomain_disappears
=== PAUSE TestAccCognitoIDPUserPoolDomain_disappears
=== CONT  TestAccCognitoIDPUserPoolDomain_basic
=== CONT  TestAccCognitoIDPUserPoolDomain_customCertUpdate
=== CONT  TestAccCognitoIDPUserPoolDomain_custom
=== CONT  TestAccCognitoIDPUserPoolDomain_disappears
--- PASS: TestAccCognitoIDPUserPoolDomain_basic (40.26s)
--- PASS: TestAccCognitoIDPUserPoolDomain_disappears (43.91s)
--- PASS: TestAccCognitoIDPUserPoolDomain_custom (1268.51s)
--- PASS: TestAccCognitoIDPUserPoolDomain_customCertUpdate (1277.69s)
PASS
ok  	github.com/hashicorp/terraform-provider-aws/internal/service/cognitoidp	1279.780s
...

@github-actions
Copy link

Hey @austinvalle 👋 Thank you very much for your contribution! At times, our maintainers need to make direct edits to pull requests in order to help get it ready to be merged. Your current settings do not allow maintainers to make such edits. To help facilitate this, update your pull request to allow such edits as described in GitHub's Allowing changes to a pull request branch created from a fork documentation. (If you're using a fork owned by an organization, your organization may not allow you to change this setting. If that is the case, let us know.)

@github-actions github-actions bot added size/L Managed by automation to categorize the size of a PR. service/cognitoidp Issues and PRs that pertain to the cognitoidp service. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. needs-triage Waiting for first response or review from a maintainer. labels Jun 10, 2022
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Welcome @austinvalle 👋

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! 😃

@justinretzolk justinretzolk 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 Jun 10, 2022
@@ -60,6 +62,10 @@ func ResourceUserPoolDomain() *schema.Resource {
Computed: true,
},
},
CustomizeDiff: customdiff.ForceNewIfChange("certificate_arn", func(_ context.Context, old, new, meta interface{}) bool {
Copy link
Collaborator

Choose a reason for hiding this comment

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

not needed if you removed ForceNew

Copy link
Member Author

Choose a reason for hiding this comment

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

So I added this ForceNewIfChange to prevent the update function being ran in these two specific edge cases, which result in an error from the AWS API:

  1. A custom domain was initially set with a certificate arn, then was removed (this triggers an update to set the certificate ARN to blank, which will cause an error)
  2. A custom domain was not initially set with a certificate arn, then one is added (this indicates that there wasn't a custom domain, which means it must be fully destroyed and re-created to add the cert arn)

If there is an easier way to do this outside of the ForceNewIfChange, or if we don't think it's necessary, I can remove it.

Copy link
Member Author

Choose a reason for hiding this comment

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

@DrFaust92 Friendly ping 😄 , do you think the above use-cases are valid? Or should I remove the CustomizeDiff function?

@DrFaust92
Copy link
Collaborator

Looks good austinvalle, see minor comment

@austinvalle austinvalle requested a review from DrFaust92 July 29, 2022 20:58
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

@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement Requests to existing resources that expand the functionality or scope. service/cognitoidp Issues and PRs that pertain to the cognitoidp service. size/L 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
Development

Successfully merging this pull request may close these issues.

Allow updating Cognito User Pool Domain Certificate ARN
5 participants