-
Notifications
You must be signed in to change notification settings - Fork 276
docs(examples): update aws lambda tooling and iam role names #1224
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
Conversation
seratch
left a comment
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.
Wow, you're quick! Thanks for working on this 🙇♂️
zimeg
left a comment
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.
📝 A few fast notes for reviewers!
| # role: lambda_basic_execution | ||
| # Have lambda:InvokeFunction & lambda:GetFunction in the allowed actions |
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.
The required roles are noted in the README.md, so these comments were removed to avoid confusion! 📚
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.
yeah, makes sense!
| # | ||
|
|
||
| # Experimental Environment variables | ||
| # Lambda environment variables |
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.
From what I can tell, these are no longer an experimental feature! 🎉
https://github.com/nficano/python-lambda?tab=readme-ov-file#environment-variables
Everything looks great; nothing to discuss further! |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1224 +/- ##
=======================================
Coverage 90.96% 90.96%
=======================================
Files 222 222
Lines 7501 7501
=======================================
Hits 6823 6823
Misses 678 678 ☔ View full report in Codecov by Sentry. |
|
|
||
|
|
||
| # export SLACK_SIGNING_SECRET=*** | ||
| # export SLACK_BOT_TOKEN=xoxb-*** |
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.
@zimeg quick question: our internal report mentioned the necessity to rename this file. when you did testing for this change, was it unnecessary?
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.
@seratch Thanks for checking on this! I noticed that updating the role used by deploy.sh was enough to resolve errors, but I will check this once more 🔍 ✨
|
@seratch Thanks so much for the fast and thorough review! My lambdas in the cloud are updating nicely after these changes so I'll go ahead and merge this now ☁️ 🐍 ⚡ |
|
Great! Thank you for resolving this issue quickly! |
Summary
This PR updates the
aws_lambdaexample app code with patches to a few snags noted in channel! These include:python-lambdaerroring when checking the status of an app deployment.deploy.shscript to fail.Also included are minor updates to inline documentation and formatting changes. Open to reverting these though!
Category
Reviewers
Both non-OAuth apps were tested in the creation of this PR. Events were sent to the Lambda URL and I got responses in channel! 🚀
Requirements
Please read the Contributing guidelines and Code of Conduct before creating this issue or pull request. By submitting, you are agreeing to those rules.
./scripts/install_all_and_run_tests.shafter making the changes.