Email Notifications [mimics PR branch] #141
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…require first running a -target run
|
It took me an undue amount of time to get the |
lorenyu
reviewed
Nov 14, 2024
There was a problem hiding this comment.
code LGTM but couple of comments:
- can you test the identity provider flows to make sure the account creation emails are still sending properly since we made changes to identity provider
- i think i spotted a bug in the sesv2_email_identity resource where we are passing in the dash_domain for the email_identity
potentially out of scope for this PR, but the identity provider and notifications features are sufficiently complex that i think we should add some e2e testing (in template-only-ci-infra.yml) for those to prevent regressions, maybe we can do that in a later PR and just be ok with manual testing for now
lorenyu
approved these changes
Nov 15, 2024
infra/modules/notifications-email-domain/resources/variables.tf
Outdated
Show resolved
Hide resolved
|
test: 
|
|
🚀 |
coilysiren
added a commit
to navapbc/template-infra
that referenced
this pull request
Dec 3, 2024
## Ticket Resolves navapbc/platform-test#140 ## Changes - Apply defaults to _"sender email"_ and _"reply to email"_ - Propagate `enable_notifications` throughout the configuration as it is now being used - Create the following modules: - `notifications_email_domain`, meant to be deployed from `main`, which deploys an DNS records in service of validating an email identity - `existing_notifications_email_domain`, meant be deployed from temporary environments, which uses data resources to pull from `notifications_email_domain` - `notifications`, used in both of the above cases, which deploys an AWS Pinpoint application capable of sending emails - Creates notification resources! ## Usage Context Grants.gov plans to use this for "transactional" emails, eg. password resets, status updates, etc. ## Context for reviewers This will be followed up by: - #789 - #778 - #777 ## Testing See navapbc/platform-test#141, specifically navapbc/platform-test#141 (comment)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Ticket
See also: #140
Resolves navapbc/template-infra#690
Changes
Creates
infra/app/service/notifications.tfwith the email notifications configurationCreates
infra/app/service/identity_provider.tfto mirror the above, entirely becauseinfra/app/service/main.tfwas getting too longAdds a pair of
notificationsandexisting_notifications, which work on the main branch and PR branches respectively. Thenotificationsmodule creates the shared resources for an environment's SES configuration (ex. DNS records), whereasexisting_notificationsrequests those shared resources. Then both environments usenotifications_appwhich creates the email notifications client application (AWS Pinpoint) that applications will use to send emails.Context for reviewers
You can test sending an email here: https://us-east-1.console.aws.amazon.com/pinpoint/home?region=us-east-1#/apps/e547b32682c34c5da1e7e18994794ca9/direct ...just... don't send emails to random 3rd parties as me 😆
Testing
Preview environment