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

feat: Add sns notification support to Parser utility #206 #207

Merged
merged 2 commits into from Nov 18, 2020

Conversation

ran-isenberg
Copy link
Contributor

Issue #, if available:

Description of changes:

Checklist

Breaking change checklist

RFC issue #:

  • Migration process documented
  • Implement warnings (if it can live side by side)

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@ran-isenberg ran-isenberg changed the title Feature: Add sns notification support to Parser utility #206 feat: Add sns notification support to Parser utility #206 Nov 15, 2020
@codecov-io
Copy link

codecov-io commented Nov 15, 2020

Codecov Report

Merging #207 (f6ccaa3) into develop (805ba53) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           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           
Impacted Files Coverage Δ
..._powertools/utilities/parser/envelopes/__init__.py 100.00% <100.00%> (ø)
...ambda_powertools/utilities/parser/envelopes/sns.py 100.00% <100.00%> (ø)
...bda_powertools/utilities/parser/models/__init__.py 100.00% <100.00%> (ø)
...s_lambda_powertools/utilities/parser/models/sns.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 805ba53...f6ccaa3. Read the comment docs.

@heitorlessa heitorlessa self-assigned this Nov 17, 2020
from typing_extensions import Literal


class SqsMsgAttributeModel(BaseModel):
Copy link
Contributor

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?

Suggested change
class SqsMsgAttributeModel(BaseModel):
class SnsMsgAttributeModel(BaseModel):

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed!

Copy link
Contributor

@heitorlessa heitorlessa left a 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):
Copy link
Contributor

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

Copy link
Contributor

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.

@heitorlessa heitorlessa added area/utilities feature New feature or functionality and removed feature New feature or functionality labels Nov 17, 2020
Copy link
Contributor

@heitorlessa heitorlessa left a 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

@heitorlessa heitorlessa merged commit 7b8fd7c into aws-powertools:develop Nov 18, 2020
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.

None yet

3 participants