Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix queue url generation for redrivePolicy in SNS events. #7277

Merged
merged 14 commits into from
Feb 19, 2020
Merged

Fix queue url generation for redrivePolicy in SNS events. #7277

merged 14 commits into from
Feb 19, 2020

Conversation

tcastelli
Copy link
Contributor

Fixes the error introduced in #7239 by not generating the queue Url and allowing to pass Ref as an input. Now the deadLetterTargetArn can be either defined as a string or using Fn::GetAtt, and the url will be generated automatically from that.

Still needs more testing but can be reviewed for structural decissions

Closes #7276

@tcastelli tcastelli changed the title WIP -Fix queue url generation for redrivePolicy in SNS events. Fix queue url generation for redrivePolicy in SNS events. Jan 31, 2020
@tcastelli
Copy link
Contributor Author

I have tested this locally with one of my projects and I could deploy it sucessfully after this fix 👍

@@ -335,6 +335,11 @@ module.exports = {
getTopicLogicalId(topicName) {
return `SNSTopic${this.normalizeTopicName(topicName)}`;
},
getTopicDLQPolicyLogicalId(functionName, topicName) {
Copy link
Contributor Author

@tcastelli tcastelli Jan 31, 2020

Choose a reason for hiding this comment

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

I've created this, since the previously generated names for the queue policy looked wrong (were referencing eventMapping) since they were designed for a Queue and not for a QueuePolicy

@tcastelli
Copy link
Contributor Author

tcastelli commented Feb 5, 2020

Since this implementation was lacking the option to use any arn defined without GetAtt or a string, I've added the option to also define the url for the queue, so now people can also use Fn:ImportValue or Fn::Join
What do you think? @medikoo @maafk

Copy link
Contributor

@medikoo medikoo left a comment

Choose a reason for hiding this comment

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

Thnank you @tcastelli looks good. Let me know what you think about my simplification proposal

docs/providers/aws/events/sns.md Outdated Show resolved Hide resolved
docs/providers/aws/events/sns.md Outdated Show resolved Hide resolved
@tcastelli
Copy link
Contributor Author

Just updated the PR with latest changes requested. Please take a look when you can @medikoo

@codecov-io
Copy link

codecov-io commented Feb 18, 2020

Codecov Report

Merging #7277 into master will increase coverage by <.01%.
The diff coverage is 80.11%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7277      +/-   ##
==========================================
+ Coverage   87.95%   87.96%   +<.01%     
==========================================
  Files         240      240              
  Lines        8969     8972       +3     
==========================================
+ Hits         7889     7892       +3     
  Misses       1080     1080
Impacted Files Coverage Δ
lib/plugins/logs/logs.js 92.85% <ø> (ø) ⬆️
commitlint.config.js 0% <ø> (ø) ⬆️
lib/plugins/index.js 100% <ø> (ø) ⬆️
lib/plugins/aws/deploy/lib/uploadArtifacts.js 90.47% <100%> (ø) ⬆️
lib/plugins/aws/package/compile/functions/index.js 97.98% <100%> (ø) ⬆️
lib/plugins/aws/provider/awsProvider.js 93.22% <100%> (ø) ⬆️
...mpile/events/lib/ensureApiGatewayCloudWatchRole.js 100% <100%> (ø) ⬆️
lib/plugins/aws/package/lib/mergeIamTemplates.js 100% <100%> (ø) ⬆️
...kage/compile/events/apiGateway/lib/method/index.js 100% <100%> (ø) ⬆️
.../package/compile/events/apiGateway/lib/validate.js 96.94% <100%> (ø) ⬆️
... and 30 more

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 4a91910...26ae9a4. Read the comment docs.

Copy link
Contributor

@medikoo medikoo left a comment

Choose a reason for hiding this comment

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

Looks great @tcastelli ! Taking this in

@medikoo medikoo merged commit 292b1ca into serverless:master Feb 19, 2020
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.

Redrive Policy not properly referencing queue
4 participants