Skip to content

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

Merged
heitorlessa merged 2 commits into
aws-powertools:developfrom
ran-isenberg:sns
Nov 18, 2020
Merged

feat: Add sns notification support to Parser utility #206#207
heitorlessa merged 2 commits into
aws-powertools:developfrom
ran-isenberg:sns

Conversation

@ran-isenberg

Copy link
Copy Markdown
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

codecov-io commented Nov 15, 2020

Copy link
Copy Markdown

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
Comment thread aws_lambda_powertools/utilities/parser/envelopes/sns.py
from typing_extensions import Literal


class SqsMsgAttributeModel(BaseModel):

Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fixed!

@heitorlessa heitorlessa left a comment

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
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

@heitorlessa heitorlessa left a comment

Copy link
Copy Markdown
Contributor

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
@ran-isenberg ran-isenberg deleted the sns branch November 18, 2020 07:24
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.

3 participants