-
Notifications
You must be signed in to change notification settings - Fork 421
feat: Add sns notification support to Parser utility #206 #207
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
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #207 +/- ##
========================================
Coverage 99.87% 99.88%
========================================
Files 67 69 +2
Lines 2499 2543 +44
Branches 108 109 +1
========================================
+ Hits 2496 2540 +44
Misses 3 3
Continue to review full report at Codecov.
|
from typing_extensions import Literal | ||
|
||
|
||
class SqsMsgAttributeModel(BaseModel): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Meant SNS instead of SQS?
class SqsMsgAttributeModel(BaseModel): | |
class SnsMsgAttributeModel(BaseModel): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good to me except FIFO if we want to be future proof
Value: str | ||
|
||
|
||
class SnsNotificationModel(BaseModel): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need groupId
and dedupId
optional fields that might be added when FIFO is enabled - Haven't tried yet myself.
Publisher reference: https://docs.aws.amazon.com/sns/latest/dg/fifo-topic-code-examples.html#fifo-topic-java-publish
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolving it as @risenberg-cyberark confirmed Lambda cannot directly subscribe to SNS FIFO topics, it'd have to go through SQS FIFO, in which case our model already covers it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding support to SNS @risenberg-cyberark :) much appreciated. Merging it now
Issue #, if available:
Description of changes:
Checklist
Breaking change checklist
RFC issue #:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.