fix(sns): create subscription object under subscriber #1645
Conversation
|
Make sure to indicate this is a breaking change since it will cause resource replacement |
| @@ -169,14 +169,18 @@ export abstract class TopicBase extends cdk.Construct implements ITopic { | |||
| * @param queue The target queue | |||
| */ | |||
| public subscribeQueue(queue: sqs.IQueue): Subscription { | |||
| const subscriptionName = queue.node.id + 'Subscription'; | |||
| if (this.node.tryFindChild(subscriptionName)) { | |||
There was a problem hiding this comment.
We should make IQueue extend IConstruct
There was a problem hiding this comment.
It does, but scope: Construct and not scope: IConstruct.
There was a problem hiding this comment.
Damn, right. I feel this is going to bite us at some point. But I really don't want people to need to specify scope: IConstruct in the initializer...
| throw new Error(`A subscription between the topic ${this.node.id} and the queue ${queue.node.id} already exists`); | ||
| } | ||
|
|
||
| // we use the queue name as the subscription's. there's no meaning to subscribing | ||
| // the same queue twice on the same topic. | ||
| const sub = new Subscription(this, subscriptionName, { | ||
| const sub = new Subscription(queue, subscriptionName, { |
There was a problem hiding this comment.
Explain why you use the queue as a scope
Don't think this is necessarily true. It's not breaking at the API level, and the replacement will happen creation-first, then deletion, so should be interrupt-free. We also need a different term for "impacts existing deployments" than BREAKING CHANGE I feel. |
You are right. It's going to recreate a resource, but without data or availability loss, so I am okay with not indicating as a breaking change. Maybe we can devise some sort of a code in the commit title that we can later on expand in CHANGELOG? |
That's what I was thinking as well. What would be a good term though? DEPLOYMENT IMPACT? |
Create the SNS Subscription object under the subscriber
instead of the subscribee, avoiding cyclic stack dependencies
if those objects would be in different stacks.
Fixes #1643, fixes #1534.
Pull Request Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.