Add email sending account #8626
Conversation
ghost
added
size/XS
SantoDE
force-pushed
the
add_email_sending_account
branch
from
54e977e to
5d2e8dd
Compare
There was a problem hiding this comment.
@SantoDE thanks for submitting this. The updates look go so far. I left a few suggestions to help push this through and would like to ask that you update the existing test to account for the new account_sending_attributes. Please see our CONTRIBUTING guide for information on updating acceptance tests.
| "email_sending_account": { | ||
| Type: schema.TypeString, | ||
| Optional: true, | ||
| ValidateFunc: validateCognitoEmailSendingAccount, |
There was a problem hiding this comment.
We can drop the function for an inline validation func
| ValidateFunc: validateCognitoEmailSendingAccount, | |
| ValidateFunc: validation.StringInSlice([]string{ | |
| cognitoidentityprovider.EmailSendingAccountTypeCognitoDefault, | |
| cognitoidentityprovider.EmailSendingAccountTypeDeveloper, | |
| },false), |
aws/validators.go
Outdated
| @@ -1762,6 +1762,16 @@ func validateCognitoResourceServerScopeName(v interface{}, k string) (ws []strin | |||
| return | |||
| } | |||
|
|
|||
| func validateCognitoEmailSendingAccount(v interface{}, k string) (ws []string, errors []error) { | |||
There was a problem hiding this comment.
We can drop this function for and use the builtin validation.StringInSlice ValidateFunc to check against the provided constants.
| @@ -65,6 +65,7 @@ The following arguments are supported: | |||
|
|
|||
| * `reply_to_email_address` (Optional) - The REPLY-TO email address. | |||
| * `source_arn` (Optional) - The ARN of the email source. | |||
| * `email_sending_account` (Optional) - Instruct Cognito to either use Cognito or SES to send out emails | |||
There was a problem hiding this comment.
| * `email_sending_account` (Optional) - Instruct Cognito to either use Cognito or SES to send out emails | |
| * `email_sending_account` (Optional) - Instruct Cognito to either use its built-in functional or Amazon SES to send out emails. |
nywilken
added
the
waiting-response
|
Hey @nywilken , thanks for your feedback! the changes are applied. However, the acceptance test is not yet passing. Sadly, it's neither without my modifications so I'm a little lost on how to move forward. Will of course try to figure things out, but a helping hand would be awesome! :) Log from the test: |
ghost
removed
the
waiting-response
|
@SantoDE thanks for the update. I haven't had a chance to revisit since you made updates, but I will take a look tomorrow, in the am, to see how I can best help with the tests. It looks like some state is not being persisted properly. The tests appear to be failing on master. |
There was a problem hiding this comment.
@SantoDE there seems to be an quirk when using COGNITO_DEFAULT for the email_sending_account where the API does not return its value in its response so it never gets persisted to the state. Which is why the existing test results in the "non empty plan error". I need to think about this a little more. A sample of the configuration is below.
resource "aws_cognito_user_pool" "pool" {
name = "terraform-test-pool"
email_configuration {
reply_to_email_address = "[email protected]"
email_sending_account = "COGNITO_DEFAULT"
}
}
In the meantime, I went ahead add made a few comments around the testing of the new attribute which should help you push this PR further. Below is the output after running all of the tests.
--- PASS: TestAccAWSCognitoUserPoolDomain_basic (22.54s)
--- PASS: TestAccAWSCognitoUserPool_withVerificationMessageTemplate (27.53s)
--- PASS: TestAccAWSCognitoUserPool_withPasswordPolicy (27.56s)
--- PASS: TestAccAWSCognitoUserPool_withDeviceConfiguration (27.58s)
--- PASS: TestAccAWSCognitoUserPool_withEmailVerificationMessage (27.62s)
--- PASS: TestAccAWSCognitoUserPool_withSmsVerificationMessage (27.80s)
--- PASS: TestAccAWSCognitoUserPoolClient_allFieldsUpdatingOneField (29.15s)
--- PASS: TestAccAWSCognitoUserPoolClient_RefreshTokenValidity (31.00s)
--- PASS: TestAccAWSCognitoUserPool_withSmsConfiguration (31.32s)
--- PASS: TestAccAWSCognitoUserPool_withSchemaAttributes (33.06s)
--- PASS: TestAccAWSCognitoUserPool_withTags (33.77s)
--- PASS: TestAccAWSCognitoUserPool_EmailConfiguration (34.48s)
--- PASS: TestAccAWSCognitoUserPool_basic (18.45s)
--- PASS: TestAccAWSCognitoUserPool_withAdvancedSecurityMode (38.52s)
--- PASS: TestAccAWSCognitoUserPoolClient_importBasic (21.93s)
--- PASS: TestAccAWSCognitoUserPool_withSmsConfigurationUpdated (41.47s)
--- PASS: TestAccAWSCognitoUserPool_withAdminCreateUserConfiguration (26.55s)
--- PASS: TestAccAWSCognitoUserPool_withAliasAttributes (50.54s)
--- PASS: TestAccAWSCognitoUserPool_withLambdaConfig (51.31s)
--- PASS: TestAccAWSCognitoUserPool_update (61.56s)
Maybe this info can help you figure out how to best handle the COGNITO_DEFAULT case.
| @@ -838,6 +839,8 @@ resource "aws_cognito_user_pool" "pool" { | |||
|
|
|||
| email_configuration { | |||
| reply_to_email_address = "foo.bar@baz" | |||
| source_arn = "arn:aws:ses:eu-west-1:123456789101:identity/[email protected]" | |||
There was a problem hiding this comment.
Lets update testAccAWSCognitoUserPoolConfig_withEmailConfiguration to look like
func testAccAWSCognitoUserPoolConfig_withEmailConfiguration(name, email, arn, account string) string {
return fmt.Sprintf(`
resource "aws_cognito_user_pool" "pool" {
name = "terraform-test-pool-%[1]s"
email_configuration {
reply_to_email_address = %[2]q
source_arn = %[3]q
email_sending_account = %[4]q
}
}`, name, email, arn, account)
}| @@ -287,6 +287,7 @@ func TestAccAWSCognitoUserPool_withEmailConfiguration(t *testing.T) { | |||
| Check: resource.ComposeAggregateTestCheckFunc( | |||
| resource.TestCheckResourceAttr("aws_cognito_user_pool.pool", "email_configuration.#", "1"), | |||
| resource.TestCheckResourceAttr("aws_cognito_user_pool.pool", "email_configuration.0.reply_to_email_address", "foo.bar@baz"), | |||
| resource.TestCheckResourceAttr("aws_cognito_user_pool.pool", "email_configuration.0.email_sending_account", "DEVELOPER"), | |||
There was a problem hiding this comment.
Looks we need a slight more involved tests to handle this check as the tests require a verified SES email ARN in order to work as expected. Trying replacing the existing test TestAccAWSCognitoUserPool_withEmailConfiguration with the following updated test case.
func TestAccAWSCognitoUserPool_EmailConfiguration(t *testing.T) {
name := acctest.RandString(5)
replyTo := fmt.Sprintf("tf-acc-reply-%[email protected]", name)
sourceARN, ok := os.LookupEnv("TEST_AWS_SES_VERIFIED_EMAIL_ARN")
if !ok {
t.Skip("'TEST_AWS_SES_VERIFIED_EMAIL_ARN' not set, skipping test.")
}
resource.ParallelTest(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckAWSCognitoUserPoolDestroy,
Steps: []resource.TestStep{
{
Config: testAccAWSCognitoUserPoolConfig_basic(name),
Check: resource.ComposeAggregateTestCheckFunc(
testAccCheckAWSCognitoUserPoolExists("aws_cognito_user_pool.pool"),
),
},
{
Config: testAccAWSCognitoUserPoolConfig_withEmailConfiguration(name, replyTo, sourceARN, "DEVELOPER"),
Check: resource.ComposeAggregateTestCheckFunc(
resource.TestCheckResourceAttr("aws_cognito_user_pool.pool", "email_configuration.#", "1"),
resource.TestCheckResourceAttr("aws_cognito_user_pool.pool", "email_configuration.0.reply_to_email_address", replyTo),
resource.TestCheckResourceAttr("aws_cognito_user_pool.pool", "email_configuration.0.email_sending_account", "DEVELOPER"),
),
},
},
})
}
nywilken
added
the
waiting-response
|
Hey @nywilken , thanks for the heads up! I was out for a couple days, but will have a look at this again tonight :) |
ghost
added
size/S
|
hey @nywilken, I did some more fiddling around (setting a default for example) but I cant get it to work. Got to admin, I'm a little lost with the whole all of that works together so I guess Im not of a real help anymore :( |
|
hey again @nywilken, I finally fixed it! see Acc Tests Please tell me whats left to get that baby shipped. :) |
ghost
added
size/M
SantoDE
force-pushed
the
add_email_sending_account
branch
from
d6bf69c to
10d0215
Compare
|
@SantoDE thank you for sticking in there with me. I didn’t have a moment to follow up these past two weeks but I’m glad you were able to get the tests passing. I’ll do another review tomorrow to get this merged and lined up for the next release; I’m excited to see the fix. Thanks again for the contribution. |
SantoDE
force-pushed
the
add_email_sending_account
branch
from
ee2ae52 to
8a81511
Compare
* Update validation for reply_to_address to allow for empty strings to fix issues around drift detection. * Replace email validation with an inline validation func within the resource. * Update email_configuration tests to test both Cognito Email constant types. Acceptance tests after change ``` --- PASS: TestAccAWSCognitoUserPool_basic (17.29s) --- PASS: TestAccAWSCognitoUserPool_importBasic (19.75s) --- PASS: TestAccAWSCognitoUserPool_withVerificationMessageTemplate (28.54s) --- PASS: TestAccAWSCognitoUserPool_withEmailConfiguration (29.14s) --- PASS: TestAccAWSCognitoUserPool_withAdminCreateUserConfiguration (29.14s) --- PASS: TestAccAWSCognitoUserPool_withTags (29.93s) --- PASS: TestAccAWSCognitoUserPool_withPasswordPolicy (29.96s) --- PASS: TestAccAWSCognitoUserPool_withDeviceConfiguration (30.86s) --- PASS: TestAccAWSCognitoUserPool_withEmailVerificationMessage (30.90s) --- PASS: TestAccAWSCognitoUserPool_withAliasAttributes (30.98s) --- PASS: TestAccAWSCognitoUserPool_withSchemaAttributes (32.96s) --- PASS: TestAccAWSCognitoUserPool_withSmsConfiguration (35.26s) --- PASS: TestAccAWSCognitoUserPool_withSmsVerificationMessage (35.92s) --- PASS: TestAccAWSCognitoUserPool_withAdvancedSecurityMode (41.35s) --- PASS: TestAccAWSCognitoUserPool_withSmsConfigurationUpdated (46.93s) --- PASS: TestAccAWSCognitoUserPool_withLambdaConfig (53.77s) --- PASS: TestAccAWSCognitoUserPool_update (58.74s) ```
There was a problem hiding this comment.
@SantoDE thanks for the updates here. I'm going to approve this PR as is and check-in a few changes on top of your PR before merging. I left a brief description around my changes so that you can understand why they were made.
| "email_sending_account": { | ||
| Type: schema.TypeString, | ||
| Optional: true, | ||
| Computed: true, |
There was a problem hiding this comment.
Since we know the expected default value here, it is preferred that it not be computed and that a default be set
| Computed: true, | |
| Default: cognitoidentityprovider.EmailSendingAccountTypeCognitoDefault, |
| @@ -131,6 +131,7 @@ func resourceAwsCognitoUserPool() *schema.Resource { | |||
| Type: schema.TypeList, | |||
| Optional: true, | |||
| MaxItems: 1, | |||
| Computed: true, | |||
There was a problem hiding this comment.
Setting Computed for an attribute means that Terraform can't detect drift for unconfigured attributes. In this case the prefer schema change is to add a DiffSuppressFunc to ignore the case where Terraform thinks our schema default was a change when it's not, given the default API response.
| Computed: true, | |
| DiffSuppressFunc: suppressMissingOptionalConfigurationBlock, |
| @@ -131,6 +131,7 @@ func resourceAwsCognitoUserPool() *schema.Resource { | |||
| Type: schema.TypeList, | |||
| Optional: true, | |||
| MaxItems: 1, | |||
| Computed: true, | |||
| Elem: &schema.Resource{ | |||
| Schema: map[string]*schema.Schema{ | |||
| "reply_to_email_address": { | |||
There was a problem hiding this comment.
In order to get our testing working I added "" as a valid value in the validation function.
|
This was merged manually. Oddly enough GitHub is not showing the correct status and it somehow has the latest changes I tried pushing to your branch which resulted in in a permission denied error locally 🤷♂️ Thanks again for your patience on this PR and for your contribution. |
|
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
Community Note
Fixes #8434
Release note for CHANGELOG:
Output from acceptance testing:
Looks like the tests can not clean up. Probably an issue with my account? I also don't know how to add that case to the AcceptanceTests (as sourceArn is also not included yet), but I did a manual test. See screenshot attached.
If someone has a guiding to get that into the Acceptance Tests, I'm happy to help :)