Simplify setup: Root TF, public ECR for notifications#16
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #16 +/- ##
=======================================
Coverage 98.28% 98.28%
=======================================
Files 10 10
Lines 408 408
=======================================
Hits 401 401
Misses 7 7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR simplifies the Cloud Cron setup by introducing a root Terraform module that centralizes SNS topic and scheduled Lambda creation, a public ECR repository for notification handlers, and a module for building/publishing to public ECR. This reduces boilerplate in the example and makes it easier for end users to adopt the system.
Changes:
- Added a root Terraform module that creates an SNS topic and scheduled Lambda with minimal configuration
- Created a notification-container module that builds and publishes a shared Lambda image with email and print handlers to public ECR
- Added a lambda-image-public module for building and pushing to public ECR repositories
- Updated lambda-image-republish module to support pulling from public ECR instead of private ECR
- Updated print and email notification modules to use explicit image_config commands for the shared notification container
- Simplified examples/basic by removing local notification handler builds and using the public ECR image instead
Reviewed changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| main.tf | Root module that creates SNS topic and scheduled Lambda |
| variables.tf | Variables for the root module including SNS and Lambda configuration |
| outputs.tf | Outputs for the root module including SNS topic ARN and Lambda details |
| versions.tf | (Pre-existing) Terraform and provider version requirements with AWS provider configuration |
| notification-container/main.tf | Module to build and publish notification handler image to public ECR |
| notification-container/lambda.py | Python Lambda handlers for email and print notifications |
| notification-container/Dockerfile | Dockerfile for the shared notification handler container |
| notification-container/variables.tf | Variables for the notification container module |
| notification-container/outputs.tf | Outputs for the notification container module |
| notification-container/versions.tf | Version requirements for the notification container module |
| notification-container/requirements.txt | Python dependencies for the notification handlers |
| modules/lambda-image-public/* | New module for building and publishing to public ECR |
| modules/lambda-image-republish/main.tf | Updated to support pulling from public ECR repositories |
| modules/lambda-image-republish/variables.tf | Updated variable descriptions for public ECR support |
| modules/print-notification/main.tf | Added image_config to specify print handler command |
| modules/email-notification/main.tf | Added image_config to specify email handler command |
| examples/basic/main.tf | Updated to use root module and public ECR image for notifications |
| examples/basic/variables.tf | Added notification image variables, removed separate print/email repo variables |
| examples/basic/print-notifier/* | Removed local print notification handler (now using public image) |
| examples/basic/email-notifier/* | Removed local email notification handler (now using public image) |
| PLAN.md | Updated checkboxes to mark completed Phase 5 tasks |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Also deny namespacing the destination.
That was silly.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
(uncovered when trying to implement with eshgham)
…into packaging
|
Hold off on reviewing this until after #17 is merged; there are some additional changes that will come in after that. |
|
@ethanholz : Now this should be ready for review! |
ethanholz
left a comment
There was a problem hiding this comment.
Just a few comments here.
| tags = local.common_tags | ||
| } | ||
| module "notification_image_republish" { | ||
| source = "../../modules/lambda-image-republish" |
There was a problem hiding this comment.
Should we make these portable using git sources instead of relative filepaths? This would allow for people to copy this code directly out of the repo. This opinion isn't strongly held but may be worth thinking about.
There was a problem hiding this comment.
I lean toward keeping it as-is, with the intent that the examples can also be used in testing (so we want them to represent the checked-out state).
| default = "cloudcron-basic" | ||
| } | ||
|
|
||
| variable "create_test_url" { |
There was a problem hiding this comment.
Have we done a test of this where you enable a testing URL at deploy and then do a separate deploy disabling it? I just want to make sure there isn't some weird AWS thing that happens for active deployments.
There was a problem hiding this comment.
Just gave it a try. It looks like everything behaves as desired (tofu plan made sense; curling the URL didn't work; and remaining infra remained in place, including an enabled CloudWatch events). Cron events continued to function.
| ## Notes | ||
|
|
||
| - Requires Docker with `buildx` and `aws` CLI credentials capable of creating public ECR repositories. | ||
| - Public ECR repositories can only be created in `us-east-1`; pass a provider configured for `us-east-1` via `providers`. |
There was a problem hiding this comment.
This is a good thing to note. Thanks for adding it.
| interpreter = ["/bin/sh", "-c"] | ||
| command = <<-EOC | ||
| set -euo pipefail | ||
| aws ecr-public get-login-password --region us-east-1 | docker login --username AWS --password-stdin public.ecr.aws |
There was a problem hiding this comment.
Is this just to facilitate deploying publicly?
There was a problem hiding this comment.
Other way around: this is pulling from a public ECR, then pushing to a private (which might not be in the same region). The login for the public endpoints is locked to us-east-1. The next line is the login to the ECR in our account (tied to our region), where we'll push it back up.
If I understand correctly, we could do the public pull without login, but logging in is better for quotas, etc. (Not that this is likely to matter for our use cases, but still.)
The main goal of this PR is to centralize some logic that was previously in
example/basic, thereby making the example simpler (and making implementation by users simpler).This involved fixing up the republish module so that it came from a public repo. It also allows us to remove some of the extra code for notifications from the example.