-
Notifications
You must be signed in to change notification settings - Fork 9.2k
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
Feature: add sns_region to sms_configuration in aws_cognito_user_pool #26684
Feature: add sns_region to sms_configuration in aws_cognito_user_pool #26684
Conversation
Hi there - this is just a draft for now as I have two issues/questions:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Welcome @ReedSoftware 👋
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 CONTRIBUTOR 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! 😃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the API's behaviour of validating the sns_region parameter for us, I'd be wary of adding tests for explicit failure cases, e.g. passing in an sns_region
that doesn't match the currently set TF provider region. Whilst plan-time validation of the sns_region
attribute might provide a quality-of-life improvement, I'm not sure that hard-coding the table at https://docs.aws.amazon.com/cognito/latest/developerguide/user-pool-sms-settings.html is something we want to do. For now, I'd recommend we just let the API catch configuration issues for us, but am happy for more experienced reviewers to suggest otherwise!
Co-authored-by: Matthew Burgess <549318+mattburgess@users.noreply.github.com>
Co-authored-by: Matthew Burgess <549318+mattburgess@users.noreply.github.com>
Co-authored-by: Matthew Burgess <549318+mattburgess@users.noreply.github.com>
Co-authored-by: Matthew Burgess <549318+mattburgess@users.noreply.github.com>
Hey guys, any reason why this is not being merged? Can we help or do something about it? |
Any updates? |
…nk lines [Expected: 1; Actual: 2]'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🚀.
% make testacc TESTS=TestAccCognitoIDPUserPool_ PKG=cognitoidp ACCTEST_PARALLELISM=2
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./internal/service/cognitoidp/... -v -count 1 -parallel 2 -run='TestAccCognitoIDPUserPool_' -timeout 180m
=== RUN TestAccCognitoIDPUserPool_basic
=== PAUSE TestAccCognitoIDPUserPool_basic
=== RUN TestAccCognitoIDPUserPool_deletionProtection
=== PAUSE TestAccCognitoIDPUserPool_deletionProtection
=== RUN TestAccCognitoIDPUserPool_recovery
=== PAUSE TestAccCognitoIDPUserPool_recovery
=== RUN TestAccCognitoIDPUserPool_withAdminCreateUser
=== PAUSE TestAccCognitoIDPUserPool_withAdminCreateUser
=== RUN TestAccCognitoIDPUserPool_withAdminCreateUserAndPasswordPolicy
=== PAUSE TestAccCognitoIDPUserPool_withAdminCreateUserAndPasswordPolicy
=== RUN TestAccCognitoIDPUserPool_withAdvancedSecurityMode
=== PAUSE TestAccCognitoIDPUserPool_withAdvancedSecurityMode
=== RUN TestAccCognitoIDPUserPool_withDevice
=== PAUSE TestAccCognitoIDPUserPool_withDevice
=== RUN TestAccCognitoIDPUserPool_withEmailVerificationMessage
=== PAUSE TestAccCognitoIDPUserPool_withEmailVerificationMessage
=== RUN TestAccCognitoIDPUserPool_MFA_sms
=== PAUSE TestAccCognitoIDPUserPool_MFA_sms
=== RUN TestAccCognitoIDPUserPool_MFA_smsAndSoftwareTokenMFA
=== PAUSE TestAccCognitoIDPUserPool_MFA_smsAndSoftwareTokenMFA
=== RUN TestAccCognitoIDPUserPool_MFA_smsToSoftwareTokenMFA
=== PAUSE TestAccCognitoIDPUserPool_MFA_smsToSoftwareTokenMFA
=== RUN TestAccCognitoIDPUserPool_MFA_softwareTokenMFA
=== PAUSE TestAccCognitoIDPUserPool_MFA_softwareTokenMFA
=== RUN TestAccCognitoIDPUserPool_MFA_softwareTokenMFAToSMS
=== PAUSE TestAccCognitoIDPUserPool_MFA_softwareTokenMFAToSMS
=== RUN TestAccCognitoIDPUserPool_smsAuthenticationMessage
=== PAUSE TestAccCognitoIDPUserPool_smsAuthenticationMessage
=== RUN TestAccCognitoIDPUserPool_sms
=== PAUSE TestAccCognitoIDPUserPool_sms
=== RUN TestAccCognitoIDPUserPool_SMS_snsRegion
=== PAUSE TestAccCognitoIDPUserPool_SMS_snsRegion
=== RUN TestAccCognitoIDPUserPool_SMS_externalID
=== PAUSE TestAccCognitoIDPUserPool_SMS_externalID
=== RUN TestAccCognitoIDPUserPool_SMS_snsCallerARN
=== PAUSE TestAccCognitoIDPUserPool_SMS_snsCallerARN
=== RUN TestAccCognitoIDPUserPool_smsVerificationMessage
=== PAUSE TestAccCognitoIDPUserPool_smsVerificationMessage
=== RUN TestAccCognitoIDPUserPool_withEmail
=== PAUSE TestAccCognitoIDPUserPool_withEmail
=== RUN TestAccCognitoIDPUserPool_withEmailSource
user_pool_test.go:815: 'TEST_AWS_SES_VERIFIED_EMAIL_ARN' not set, skipping test.
--- SKIP: TestAccCognitoIDPUserPool_withEmailSource (0.00s)
=== RUN TestAccCognitoIDPUserPool_withTags
=== PAUSE TestAccCognitoIDPUserPool_withTags
=== RUN TestAccCognitoIDPUserPool_withAliasAttributes
=== PAUSE TestAccCognitoIDPUserPool_withAliasAttributes
=== RUN TestAccCognitoIDPUserPool_withUsernameAttributes
=== PAUSE TestAccCognitoIDPUserPool_withUsernameAttributes
=== RUN TestAccCognitoIDPUserPool_withPasswordPolicy
=== PAUSE TestAccCognitoIDPUserPool_withPasswordPolicy
=== RUN TestAccCognitoIDPUserPool_withUsername
=== PAUSE TestAccCognitoIDPUserPool_withUsername
=== RUN TestAccCognitoIDPUserPool_withLambda
=== PAUSE TestAccCognitoIDPUserPool_withLambda
=== RUN TestAccCognitoIDPUserPool_WithLambda_email
=== PAUSE TestAccCognitoIDPUserPool_WithLambda_email
=== RUN TestAccCognitoIDPUserPool_WithLambda_sms
=== PAUSE TestAccCognitoIDPUserPool_WithLambda_sms
=== RUN TestAccCognitoIDPUserPool_schemaAttributes
=== PAUSE TestAccCognitoIDPUserPool_schemaAttributes
=== RUN TestAccCognitoIDPUserPool_schemaAttributesRemoved
=== PAUSE TestAccCognitoIDPUserPool_schemaAttributesRemoved
=== RUN TestAccCognitoIDPUserPool_schemaAttributesModified
=== PAUSE TestAccCognitoIDPUserPool_schemaAttributesModified
=== RUN TestAccCognitoIDPUserPool_withVerificationMessageTemplate
=== PAUSE TestAccCognitoIDPUserPool_withVerificationMessageTemplate
=== RUN TestAccCognitoIDPUserPool_update
=== PAUSE TestAccCognitoIDPUserPool_update
=== RUN TestAccCognitoIDPUserPool_disappears
=== PAUSE TestAccCognitoIDPUserPool_disappears
=== RUN TestAccCognitoIDPUserPool_withUserAttributeUpdateSettings
=== PAUSE TestAccCognitoIDPUserPool_withUserAttributeUpdateSettings
=== CONT TestAccCognitoIDPUserPool_basic
=== CONT TestAccCognitoIDPUserPool_smsVerificationMessage
--- PASS: TestAccCognitoIDPUserPool_basic (22.50s)
=== CONT TestAccCognitoIDPUserPool_MFA_smsAndSoftwareTokenMFA
--- PASS: TestAccCognitoIDPUserPool_smsVerificationMessage (35.87s)
=== CONT TestAccCognitoIDPUserPool_SMS_snsCallerARN
--- PASS: TestAccCognitoIDPUserPool_MFA_smsAndSoftwareTokenMFA (62.17s)
=== CONT TestAccCognitoIDPUserPool_SMS_externalID
--- PASS: TestAccCognitoIDPUserPool_SMS_snsCallerARN (57.69s)
=== CONT TestAccCognitoIDPUserPool_SMS_snsRegion
--- PASS: TestAccCognitoIDPUserPool_SMS_snsRegion (30.78s)
=== CONT TestAccCognitoIDPUserPool_sms
--- PASS: TestAccCognitoIDPUserPool_SMS_externalID (62.96s)
=== CONT TestAccCognitoIDPUserPool_smsAuthenticationMessage
--- PASS: TestAccCognitoIDPUserPool_smsAuthenticationMessage (33.25s)
=== CONT TestAccCognitoIDPUserPool_MFA_softwareTokenMFAToSMS
--- PASS: TestAccCognitoIDPUserPool_sms (58.27s)
=== CONT TestAccCognitoIDPUserPool_MFA_softwareTokenMFA
--- PASS: TestAccCognitoIDPUserPool_MFA_softwareTokenMFA (46.82s)
=== CONT TestAccCognitoIDPUserPool_MFA_smsToSoftwareTokenMFA
--- PASS: TestAccCognitoIDPUserPool_MFA_softwareTokenMFAToSMS (51.36s)
=== CONT TestAccCognitoIDPUserPool_WithLambda_sms
--- PASS: TestAccCognitoIDPUserPool_MFA_smsToSoftwareTokenMFA (43.85s)
=== CONT TestAccCognitoIDPUserPool_withUserAttributeUpdateSettings
--- PASS: TestAccCognitoIDPUserPool_WithLambda_sms (57.68s)
=== CONT TestAccCognitoIDPUserPool_disappears
--- PASS: TestAccCognitoIDPUserPool_withUserAttributeUpdateSettings (29.95s)
=== CONT TestAccCognitoIDPUserPool_update
--- PASS: TestAccCognitoIDPUserPool_disappears (14.15s)
=== CONT TestAccCognitoIDPUserPool_withVerificationMessageTemplate
--- PASS: TestAccCognitoIDPUserPool_withVerificationMessageTemplate (32.03s)
=== CONT TestAccCognitoIDPUserPool_schemaAttributesModified
--- PASS: TestAccCognitoIDPUserPool_schemaAttributesModified (22.98s)
=== CONT TestAccCognitoIDPUserPool_schemaAttributesRemoved
--- PASS: TestAccCognitoIDPUserPool_update (64.85s)
=== CONT TestAccCognitoIDPUserPool_schemaAttributes
--- PASS: TestAccCognitoIDPUserPool_schemaAttributesRemoved (22.72s)
=== CONT TestAccCognitoIDPUserPool_withPasswordPolicy
--- PASS: TestAccCognitoIDPUserPool_schemaAttributes (34.24s)
=== CONT TestAccCognitoIDPUserPool_WithLambda_email
--- PASS: TestAccCognitoIDPUserPool_withPasswordPolicy (33.15s)
=== CONT TestAccCognitoIDPUserPool_withLambda
--- PASS: TestAccCognitoIDPUserPool_WithLambda_email (65.87s)
=== CONT TestAccCognitoIDPUserPool_withUsername
--- PASS: TestAccCognitoIDPUserPool_withLambda (73.56s)
=== CONT TestAccCognitoIDPUserPool_withAdvancedSecurityMode
--- PASS: TestAccCognitoIDPUserPool_withUsername (34.57s)
=== CONT TestAccCognitoIDPUserPool_MFA_sms
--- PASS: TestAccCognitoIDPUserPool_withAdvancedSecurityMode (46.95s)
=== CONT TestAccCognitoIDPUserPool_withEmailVerificationMessage
--- PASS: TestAccCognitoIDPUserPool_withEmailVerificationMessage (32.29s)
=== CONT TestAccCognitoIDPUserPool_withDevice
--- PASS: TestAccCognitoIDPUserPool_MFA_sms (75.85s)
=== CONT TestAccCognitoIDPUserPool_withTags
--- PASS: TestAccCognitoIDPUserPool_withDevice (33.73s)
=== CONT TestAccCognitoIDPUserPool_withEmail
=== CONT TestAccCognitoIDPUserPool_withUsernameAttributes
--- PASS: TestAccCognitoIDPUserPool_withEmail (24.61s)
--- PASS: TestAccCognitoIDPUserPool_withTags (53.09s)
=== CONT TestAccCognitoIDPUserPool_withAdminCreateUser
--- PASS: TestAccCognitoIDPUserPool_withUsernameAttributes (38.64s)
=== CONT TestAccCognitoIDPUserPool_withAdminCreateUserAndPasswordPolicy
--- PASS: TestAccCognitoIDPUserPool_withAdminCreateUser (38.33s)
=== CONT TestAccCognitoIDPUserPool_recovery
--- PASS: TestAccCognitoIDPUserPool_withAdminCreateUserAndPasswordPolicy (23.41s)
=== CONT TestAccCognitoIDPUserPool_deletionProtection
--- PASS: TestAccCognitoIDPUserPool_recovery (62.49s)
=== CONT TestAccCognitoIDPUserPool_withAliasAttributes
--- PASS: TestAccCognitoIDPUserPool_deletionProtection (46.54s)
--- PASS: TestAccCognitoIDPUserPool_withAliasAttributes (60.64s)
PASS
ok github.com/hashicorp/terraform-provider-aws/internal/service/cognitoidp 797.888s
@ReedSoftware Thanks for the contribution 🎉 👏. |
Wooohooo! Thanks so much for rerunning those gh actions and fixing up those bits @ewbankkit as well as reviewing and approving. Feels good to give back :) |
This functionality has been released in v4.40.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! |
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. |
Community Note
Closes #26144
Output from acceptance testing: