Single SNS topic, instead of topic per result type#12
Conversation
No longer needed, since we only need to create one SNS topic.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #12 +/- ##
==========================================
+ Coverage 98.36% 98.39% +0.03%
==========================================
Files 8 8
Lines 306 312 +6
==========================================
+ Hits 301 307 +6
Misses 5 5 ☔ 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 refactors the cloud-cron architecture to use a single SNS topic with message attribute filtering instead of multiple topics per result type. This simplifies infrastructure management while maintaining flexible routing capabilities through SNS subscription filter policies.
Changes:
- Replaced per-result-type SNS topics with a single shared topic and message attribute-based filtering
- Updated Python runtime to publish messages with a
result_typemessage attribute - Added subscription filter policy support in notification modules to enable selective result type subscription
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/cloud_cron/lambda_task.py | Refactored to use single SNS topic ARN and add result_type as message attribute; removed validate_sns_result function |
| src/cloud_cron/notifications/base.py | Added include_result_type parameter and _extract_result_type method to inject result type into payloads from message attributes |
| tests/test_lambda_task.py | Updated tests to reflect single topic architecture and message attribute usage |
| tests/notifications/test_base.py | Added comprehensive tests for result_type injection with parametrized test cases |
| modules/sns-topics/* | Removed entire module as it's no longer needed with single-topic architecture |
| modules/scheduled-lambda/* | Changed from map of topic ARNs to single topic ARN parameter |
| modules/notification-plumbing/* | Added result_types parameter and SNS filter policy support for message attribute filtering |
| modules/print-notification/* | Added result_types parameter pass-through to notification-plumbing module |
| examples/basic/main.tf | Replaced sns-topics module with direct aws_sns_topic resource |
| PLAN.md | Documented Phase 2.5 architectural decision and implementation checklist |
| IDEA.md | Updated usage examples to reflect single-topic pattern |
Comments suppressed due to low confidence (1)
examples/basic/main.tf:88
- The print_notification module is missing the
result_typesparameter. According to the module's README and PLAN.md (Phase 2.5), the example should demonstrate filtering by result type. Since the scheduled lambda publishes messages with result_type "example", the print_notification should includeresult_types = ["example"]to demonstrate the subscription filtering feature. While omitting this parameter will subscribe to all result types (which works), it doesn't showcase the new filtering capability that is a key part of this architectural change.
module "print_notification" {
source = "../../modules/print-notification"
sns_topic_arn = aws_sns_topic.results.arn
fifo_queue_name = "example-print.fifo"
lambda_image_uri = local.active_print_image_uri
template_file = "${path.module}/templates/print.txt"
tags = local.common_tags
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@ethanholz : Looks like a version of this gave me the "Hello world" in practice deployment. Should be ready for review! |
ethanholz
left a comment
There was a problem hiding this comment.
Looks good, lets merge!
This switches us to an architecture where there is a single SNS topic, with subscribers who filter based on result type, instead of passing a mapping of result type to SNS topic ARN. This does a little more to decouple the code and infrastructure, while still enabling the use cases we want.
This also removes the
sns-topicsmodule, since it mainly existed to provide a JSON mapping of result type to topic. Now a user just needs to provide a topic.