feat(sns): support multiple tokens as url and email subscriptions #6357
Conversation
fixes aws#3996 to allow using tokens in email subscriptions, additionally fixes a bug with URL subscriptions when using more then one token subscription.
|
Title does not follow the guidelines of Conventional Commits. Please adjust title before merge. |
strazeadin
changed the title
strazeadin
changed the title
strazeadin
changed the title
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
nija-at
changed the title
There was a problem hiding this comment.
Thanks for getting this through. Looks mostly ok to me; a few things below.
Given the non-trivial nature of this change, I would expect the commit message to be more detailed. Start with a short paragraph on what the problem that this change is fixing followed by what the change is doing, i.e., storing tokens as construct nodes with a special prefix and counting them.
…adin/aws-cdk into fix/sns-subscriptions-with-tokens
mergify
bot
dismissed
nija-at’s stale review
Pull request has been modified.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
There was a problem hiding this comment.
Code looks good to me.
As claimed by the failing PR check, this change needs an update to the package's README. Can you add this information to the appropriate sections?
A URL section is missing, let's add that. As well as, state in both the email and url sections that they support tokens with a code example. A good example would be to show the use of CfnParameter (which is powered by tokens),
const emailAddress = new CfnParameter(this, 'email-param');
myTopic.addSubscription(new subscriptions.EmailSubscription(emailAddress.valueAsString());
mergify
bot
dismissed
nija-at’s stale review
Pull request has been modified.
|
Pushed an update to the readme file (Although I believe this is a bug fix and not a new feat). The URL section already existed (under HTTPS) so I added a note regarding support for parameters as I haven't found many README files directly calling out support for tokens so wasn't sure what terminology to use. Happy to make it more generic based on existing examples if you have any. |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
I've pushed a slightly modified README into this PR. Hope it looks ok to you. Thanks for submitting this change 😊 |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Thank you for contributing! Your pull request is now being automatically merged. |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Thank you for contributing! Your pull request is now being automatically merged. |
Commit Message
feat(sns): support multiple tokens as url and email subscriptions (#6357)
fixes #3996 to allow using tokens in email subscriptions, additionally
fixes a bug with URL subscriptions when using more than one token
subscription.
The Issue
Email Subscriptions currently use the value passed in as the construct
ID, when the value passed in is a token (For example a parameter) it
causes an error as tokens aren't supported as construct IDs. A previous
fix was done for URL Subscriptions but it also errors when more than
one URL subscription with a token is used.
The fix
In the topic base, identify if the subscription ID is a token and
override it to a valid construct ID. The method of ensuring a valid ID
is to convert it to a special prefix suffixed by a number and doing an
increment of the number for each new topic created with a token.
Subscriptions not utilizing a token are not effected.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license