feat(sns): adding support for firehose subscription protocol #15764
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -176,7 +176,7 @@ describe('Subscription', () => { | |
|
|
||
| }); | ||
|
|
||
| test('throws with raw delivery for lambda protocol', () => { | ||
|
There was a problem hiding this comment. ❓ Why did you make this change? While the text more accurately describes the specifics of the test, they lose the intent behind the test. There was a problem hiding this comment. @njlynch , the choice was between changing the name to "protocol other than http, https, sqs, or firehose" (since firehose supports raw delivery) or changing it to "lambda protocol", which has the advantage of being more accurate. As it is, the test name is misleading - it doesn't cover email or sms which also don't support raw delivery. Better just to name it more transparently. There was a problem hiding this comment. Ah. If Firehose supports raw delivery, then it looks like this check needs to be updated: aws-cdk/packages/@aws-cdk/aws-sns/lib/subscription.ts Lines 84 to 86 in b838f95 You're accurate the test name is now more accurate, but it points out the inadequacy of it. Could you swap it to use a |
||
| // GIVEN | ||
| const stack = new cdk.Stack(); | ||
| const topic = new sns.Topic(stack, 'Topic'); | ||
|
|
@@ -232,4 +232,17 @@ describe('Subscription', () => { | |
| })).toThrow(/\(120\) must not exceed 100/); | ||
|
|
||
| }); | ||
|
|
||
| test('throws an error when subscription role arn is not entered with firehose subscription protocol', () => { | ||
| // GIVEN | ||
| const stack = new cdk.Stack(); | ||
| const topic = new sns.Topic(stack, 'Topic'); | ||
|
|
||
| //THEN | ||
| expect(() => new sns.Subscription(stack, 'Subscription', { | ||
| endpoint: 'endpoint', | ||
| protocol: sns.SubscriptionProtocol.FIREHOSE, | ||
| topic, | ||
| })).toThrow(/Subscription role arn is required field for subscriptions with a firehose protocol./); | ||
| }); | ||
| }); | ||
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.
~(Nit) This is okay, if we're going to leave this implementation as the basics only. I'd greatly prefer we document it as part of aFirehoseSubscriptionin theaws-sns-subscriptionsREADME instead though.