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

Lambda alias events #11788

Merged
merged 5 commits into from Mar 14, 2023
Merged

Lambda alias events #11788

merged 5 commits into from Mar 14, 2023

Conversation

rnielsen
Copy link
Contributor

@rnielsen rnielsen commented Mar 10, 2023

Most events were not using the lambda alias when calling the lambda when using the provisionedConcurrency or snapStart options, meaning these features weren't being used. I've created a new unit test alias.test.js which checks the following for all resources which have a reference to a provisionedConcurrency or snapStart lambda:

  • Make sure the alias is configured in the ARN
  • Make sure the alias is the DependsOn

Closes: #11662
Closes: #11246
Closes: #11484
Closes: #10764
Closes: #10167

@codecov
Copy link

codecov bot commented Mar 10, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.04 🎉

Comparison is base (8e244d5) 86.56% compared to head (e14947f) 86.60%.

❗ Current head e14947f differs from pull request most recent head 4bd7aaf. Consider uploading reports for the commit 4bd7aaf to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #11788      +/-   ##
==========================================
+ Coverage   86.56%   86.60%   +0.04%     
==========================================
  Files         314      314              
  Lines       13127    13161      +34     
==========================================
+ Hits        11363    11398      +35     
+ Misses       1764     1763       -1     
Impacted Files Coverage Δ
...kage/compile/events/api-gateway/lib/authorizers.js 100.00% <ø> (ø)
...ile/events/api-gateway/lib/method/authorization.js 100.00% <ø> (ø)
lib/plugins/aws/deploy-function.js 96.22% <100.00%> (+0.01%) ⬆️
lib/plugins/aws/package/compile/events/activemq.js 100.00% <100.00%> (ø)
...ws/package/compile/events/alb/lib/target-groups.js 100.00% <100.00%> (ø)
.../plugins/aws/package/compile/events/alexa-skill.js 96.29% <100.00%> (ø)
...ins/aws/package/compile/events/alexa-smart-home.js 100.00% <100.00%> (ø)
...age/compile/events/api-gateway/lib/method/index.js 100.00% <100.00%> (ø)
...ompile/events/api-gateway/lib/request-validator.js 98.07% <100.00%> (ø)
...ns/aws/package/compile/events/cloud-watch-event.js 97.77% <100.00%> (+0.05%) ⬆️
... and 19 more

... and 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

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.

@rnielsen great, thanks for addressing that. It looks impressive!

Still, please see my comment. I believe a lot of changes hare are not needed (and if they're needed, they're out of scope, and should be covered with different PR)

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.

@rnielsen again, that looks really good. I'm impressed.

I've proposed just one stylistic change, and can you also confirm on your side that integration tests are passing. See the instructions: https://github.com/serverless/serverless/tree/main/test#aws-integration-tests

lib/plugins/aws/package/compile/events/activemq.js Outdated Show resolved Hide resolved
lib/plugins/aws/package/compile/events/cloud-watch-log.js Outdated Show resolved Hide resolved
lib/plugins/aws/package/compile/events/kafka.js Outdated Show resolved Hide resolved
@rnielsen rnielsen requested a review from medikoo March 13, 2023 06:12
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.

@rnielsen Thank you! I've proposed just one last stylistic update and we should be good to go

@rnielsen rnielsen requested a review from medikoo March 13, 2023 08:56
@rnielsen rnielsen requested a review from medikoo March 13, 2023 22:53
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.

Thank you @rnielsen !

@medikoo medikoo merged commit 16dd286 into serverless:main Mar 14, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment