r/cognito_user_pool: Add ConflictsWith to a few fields #2425
Conversation
radeksimko
added
the
documentation
|
According to AWS support,
|
| @@ -31,7 +31,11 @@ The following arguments are supported: | |||
| * `email_configuration` (Optional) - The [Email Configuration](#email-configuration). | |||
| * `name` - (Required) The name of the user pool. | |||
| * `email_verification_subject` - (Optional) A string representing the email verification subject. | |||
|
|
|||
| ~> **Note** `verification_message_template.email_subject` replaces this string when you specify these two. | |||
There was a problem hiding this comment.
I'm wondering whether we should set email_verification_subject as ConflictsWith: []string{"verification_message_template.0. email_subject"},, since we cannot have both being different. The same would applies to the email_verification_message
Thus, we could avoid such notice... what do you think @atsushi-ishibashi
Also pinging @radeksimko to get it's opinion :)
There was a problem hiding this comment.
I'm worrying that it becomes breaking-change.
There are users who are forced update .tf files.
There was a problem hiding this comment.
Yes, it would be, but since it's one or the other according to the AWS support... this is a required thing IMO.
There was a problem hiding this comment.
@atsushi-ishibashi Correct me if I'm wrong but the API misbehaves and causes spurious diffs if you provide both?
The only case when I see ConflictsWith causing problems/BC is when folks use the exact same values.
We can defer the ConflictsWith addition to 2.0.0 and document this as suggested in the meantime.
@Ninir what do you think?
There was a problem hiding this comment.
@atsushi-ishibashi Correct me if I'm wrong but the API misbehaves and causes spurious diffs if you provide both?
Exactly!
We can defer the ConflictsWith addition to 2.0.0 and document this as suggested in the meantime.
Do agree on that :) 👍
There was a problem hiding this comment.
@radeksimko That's right!
We can defer the ConflictsWith addition to 2.0.0 and document this as suggested in the meantime.
I also agree!
atsushi-ishibashi
force-pushed
the
issue2423
branch
from
ef8794e to
f5dc889
Compare
radeksimko
added
breaking-change
radeksimko
changed the title
There was a problem hiding this comment.
LGTM, thanks @atsushi-ishibashi 🚀 This will be added to the Version 2 Upgrade Guide on merge.
--- PASS: TestAccAWSCognitoUserPool_basic (11.04s)
--- PASS: TestAccAWSCognitoUserPool_withPasswordPolicy (17.22s)
--- PASS: TestAccAWSCognitoUserPool_withEmailVerificationMessage (17.71s)
--- PASS: TestAccAWSCognitoUserPool_withTags (19.28s)
--- PASS: TestAccAWSCognitoUserPool_withAliasAttributes (19.78s)
--- PASS: TestAccAWSCognitoUserPool_withAdminCreateUserConfiguration (20.77s)
--- PASS: TestAccAWSCognitoUserPool_withEmailConfiguration (21.09s)
--- PASS: TestAccAWSCognitoUserPool_importBasic (21.60s)
--- PASS: TestAccAWSCognitoUserPool_withDeviceConfiguration (21.67s)
--- PASS: TestAccAWSCognitoUserPool_withVerificationMessageTemplate (22.30s)
--- PASS: TestAccAWSCognitoUserPool_withSchemaAttributes (22.67s)
--- PASS: TestAccAWSCognitoUserPool_withSmsVerificationMessage (24.39s)
--- PASS: TestAccAWSCognitoUserPool_withSmsConfiguration (28.07s)
--- PASS: TestAccAWSCognitoUserPool_withSmsConfigurationUpdated (32.73s)
--- PASS: TestAccAWSCognitoUserPool_withAdvancedSecurityMode (33.28s)
--- PASS: TestAccAWSCognitoUserPool_withLambdaConfig (39.78s)
--- PASS: TestAccAWSCognitoUserPool_update (44.01s)
…ersion 2 Upgrade Guide, and CHANGELOG for #2425
|
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
locked and limited conversation to collaborators
Related: #2423