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

Email Notifications #786

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Email Notifications #786

wants to merge 8 commits into from

Conversation

coilysiren
Copy link
Contributor

@coilysiren coilysiren commented Nov 19, 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:

Testing

See navapbc/platform-test#141, specifically navapbc/platform-test#141 (comment)

@coilysiren coilysiren changed the title git apply Email Notifications Nov 19, 2024
@coilysiren coilysiren marked this pull request as ready for review November 22, 2024 20:56
Copy link
Contributor

@lorenyu lorenyu left a comment

Choose a reason for hiding this comment

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

LGTM once tests passes

@coilysiren
Copy link
Contributor Author

Ahah! This CI failure is relevant

2024-11-22T22:24:15.8946157Z     command.go:33: 
2024-11-22T22:24:15.8946513Z         	Error Trace:	command.go:33
2024-11-22T22:24:15.8947109Z         	            				template_infra_test.go:123
2024-11-22T22:24:15.8947589Z         	Error:      	Received unexpected error:
2024-11-22T22:24:15.8948398Z         	            	error while running command: exit status 2; �[31m╷�[0m�[0m
2024-11-22T22:24:15.8949466Z         	            	�[31m│�[0m �[0m�[1m�[31mError: �[0m�[0m�[1mAttempt to get attribute from null value�[0m
2024-11-22T22:24:15.8950110Z         	            	�[31m│�[0m �[0m
2024-11-22T22:24:15.8950864Z         	            	�[31m│�[0m �[0m�[0m  on notifications.tf line 12, in locals:
2024-11-22T22:24:15.8952114Z         	            	�[31m│�[0m �[0m  12:   notifications_app_name = "${local.prefix}${local.notifications_config�[4m.name�[0m}"�[0m
2024-11-22T22:24:15.8953182Z         	            	�[31m│�[0m �[0m    �[90m├────────────────�[0m
2024-11-22T22:24:15.8954115Z         	            	�[31m│�[0m �[0m�[0m    �[90m│�[0m �[1mlocal.notifications_config�[0m is null
2024-11-22T22:24:15.8954750Z         	            	�[31m│�[0m �[0m�[0m
2024-11-22T22:24:15.8955587Z         	            	�[31m│�[0m �[0mThis value is null, so it does not have any attributes.
2024-11-22T22:24:15.8956191Z         	            	�[31m╵�[0m�[0m
2024-11-22T22:24:15.8956948Z         	            	make[1]: *** [Makefile:186: infra-update-app-service] Error 1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants