feat: Add an existing SQS queue option to SNS event #1231

53ningen · 2019-10-31T19:37:05Z

readable diff:
https://github.com/awslabs/serverless-application-model/pull/1231/files?w=1

Issue #, if available:
#854

Description of changes:
Add an existing SQS queue option to SNS event

Description of how you validated changes:

make pr
verify transformed template deploys and application functions as expected

Checklist:

Write/update tests
make pr passes
Update documentation
Verify transformed template deploys and application functions as expected
Add/update example to examples/2016-10-31

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

codecov-io · 2019-10-31T19:40:45Z

Codecov Report

Merging #1231 into develop will decrease coverage by 0.02%.
The diff coverage is 93.1%.

@@             Coverage Diff             @@
##           develop    #1231      +/-   ##
===========================================
- Coverage    94.61%   94.59%   -0.03%     
===========================================
  Files           78       78              
  Lines         4252     4271      +19     
  Branches       844      846       +2     
===========================================
+ Hits          4023     4040      +17     
- Misses         110      111       +1     
- Partials       119      120       +1

Impacted Files	Coverage Δ
samtranslator/model/eventsources/push.py	`90.82% <93.1%> (-0.06%)`	⬇️

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 9a66c83...4456c25. Read the comment docs.

to minimize the diff, refine SNS.to_cloudformation in advance

…d QueuePolicy

53ningen · 2019-11-08T14:34:31Z

this PR is now ready for review

53ningen · 2019-11-08T14:54:45Z

samtranslator/model/eventsources/push.py

@@ -392,17 +392,42 @@ def to_cloudformation(self, **kwargs):
            )
            return [self._construct_permission(function, source_arn=self.Topic), subscription]

-        # SNS -> SQS -> Lambda
+        # SNS -> SQS(Create New) -> Lambda
+        if isinstance(self.SqsSubscription, bool):


if isinstance(self.SqsSubscription, bool) is True, then self.SqsSubscription is always True, because we checked if self.SqsSubscription is False on L388

ShreyaGangishetty

@53ningen Thank you for contributing this feature!! This looks great. I have minor comments about adding a unit test and also on throwing errors when QueueArn or QueueUrl are not given.
Could you please add/update any of the existing unit tests for a condition where SqsSubscritption: False.

samtranslator/model/eventsources/push.py

timoschilling · 2019-11-12T09:20:20Z

The informations of QueueArn and QueueUrl seams to be redundant. Isn't it possible to build the QueueUrl from the QueueArn, so that we only need to configure one of them?

chrisoverzero · 2019-11-12T11:44:56Z

@timoschilling Gominingen described why this was sometimes not possible (and why a separate URL can be a feature) back in the original issue. In short, QueueArn is rarely going to be a string. It will almost always be an object with a key of Fn::GetAtt.

I can imagine that and similar being features of the next round of the, uh, feature:

If no QueueUrl specified:
- If QueueArn is string, construct URL as string.
- If it is Fn::GetAtt object, get logical ID, construct Ref object.
- Bail on other intrinsics.

…which sound like really nice checks, assuming I haven’t forgotten why they’re impossible.

53ningen · 2019-12-05T17:10:04Z

tests/model/eventsources/test_sns_event_source.py

+        self.sns_event_source.SqsSubscription = sqsSubscription
+        self.assertRaises(TypeError, self.sns_event_source.to_cloudformation)
+
+    def test_to_cloudformation_when_sqs_subscription_disable(self):


Could you please add/update any of the existing unit tests for a condition where SqsSubscritption: False.

I've added on this line.

53ningen · 2019-12-05T17:13:11Z

@ShreyaGangishetty
Thank very much you for reviewing this PR and I'm sorry for my late response.
I have addressed your pull request comments.

Ready for review again.

ShreyaGangishetty

@53ningen Thank you for addressing my comments. This PR looks good to me!!

praneetap

Thanks a ton for submitting this feature!

ljacobsson · 2020-11-06T09:41:39Z

What's the status on this?

It looks like it was included in the v1.20.0 release, but I can't get it working and failing on this line. I've even tried to define both QueueArn and QueueUrl

Also, there's no sign of this feature in the documentation.

chrisoverzero · 2020-11-06T13:11:35Z

Also, there's no sign of this feature in the documentation.

It is in the documentation and specification.

53ningen force-pushed the sns-sqs-lambda branch from c9f78f0 to 9f7f2c7 Compare October 31, 2019 19:41

53ningen added 4 commits November 8, 2019 03:12

refinement: SNS.to_cloudformation

c33f019

to minimize the diff, refine SNS.to_cloudformation in advance

refactor: make some parameters injectable to SQSEventSourceMapping an…

6da31a7

…d QueuePolicy

feat: Add an existing SQS queue option to SNS event

f0f2be5

Add tests

90192b6

53ningen force-pushed the sns-sqs-lambda branch 3 times, most recently from 20a09d8 to e50de06 Compare November 8, 2019 14:17

update a document

fd99293

53ningen force-pushed the sns-sqs-lambda branch from e50de06 to fd99293 Compare November 8, 2019 14:24

53ningen marked this pull request as ready for review November 8, 2019 14:25

53ningen mentioned this pull request Nov 8, 2019

Feature Request: Add SQS option to SNS event #854

Closed

53ningen commented Nov 8, 2019

View reviewed changes

praneetap assigned ShreyaGangishetty Nov 8, 2019

ShreyaGangishetty suggested changes Nov 11, 2019

View reviewed changes

samtranslator/model/eventsources/push.py Show resolved Hide resolved

53ningen added 3 commits December 6, 2019 01:45

throws exception when a QueueARN or QueueURL not given

40cba76

add a unit test for SqsSubscription: false

9189a2e

Merge branch 'develop' into sns-sqs-lambda

4456c25

53ningen requested a review from ShreyaGangishetty December 5, 2019 17:01

53ningen commented Dec 5, 2019

View reviewed changes

ShreyaGangishetty approved these changes Dec 5, 2019

View reviewed changes

ShreyaGangishetty assigned praneetap and unassigned ShreyaGangishetty Dec 5, 2019

praneetap approved these changes Dec 13, 2019

View reviewed changes

praneetap merged commit 30e33b4 into aws:develop Dec 13, 2019

praneetap added stage/waiting-for-release labels Dec 13, 2019

53ningen deleted the sns-sqs-lambda branch December 13, 2019 08:42

ShreyaGangishetty mentioned this pull request Dec 17, 2019

V1.20.0 Release Tracker #1212

Closed

jamesiri unassigned praneetap Aug 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add an existing SQS queue option to SNS event #1231

feat: Add an existing SQS queue option to SNS event #1231

53ningen commented Oct 31, 2019 •

edited

Loading

codecov-io commented Oct 31, 2019 •

edited

Loading

53ningen commented Nov 8, 2019 •

edited

Loading

53ningen Nov 8, 2019 •

edited

Loading

ShreyaGangishetty left a comment

timoschilling commented Nov 12, 2019 •

edited

Loading

chrisoverzero commented Nov 12, 2019

53ningen Dec 5, 2019

53ningen commented Dec 5, 2019

ShreyaGangishetty left a comment

praneetap left a comment

ljacobsson commented Nov 6, 2020

chrisoverzero commented Nov 6, 2020

feat: Add an existing SQS queue option to SNS event #1231

feat: Add an existing SQS queue option to SNS event #1231

Conversation

53ningen commented Oct 31, 2019 • edited Loading

codecov-io commented Oct 31, 2019 • edited Loading

Codecov Report

53ningen commented Nov 8, 2019 • edited Loading

53ningen Nov 8, 2019 • edited Loading

Choose a reason for hiding this comment

ShreyaGangishetty left a comment

Choose a reason for hiding this comment

timoschilling commented Nov 12, 2019 • edited Loading

chrisoverzero commented Nov 12, 2019

53ningen Dec 5, 2019

Choose a reason for hiding this comment

53ningen commented Dec 5, 2019

ShreyaGangishetty left a comment

Choose a reason for hiding this comment

praneetap left a comment

Choose a reason for hiding this comment

ljacobsson commented Nov 6, 2020

chrisoverzero commented Nov 6, 2020

53ningen commented Oct 31, 2019 •

edited

Loading

codecov-io commented Oct 31, 2019 •

edited

Loading

53ningen commented Nov 8, 2019 •

edited

Loading

53ningen Nov 8, 2019 •

edited

Loading

timoschilling commented Nov 12, 2019 •

edited

Loading