-
Notifications
You must be signed in to change notification settings - Fork 402
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
feat(layers): add support for publishing v2 layer #1558
Conversation
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.
as per peer review session
Codecov ReportBase: 99.42% // Head: 99.42% // No change to project coverage 👍
Additional details and impacted files@@ Coverage Diff @@
## v2 #1558 +/- ##
=======================================
Coverage 99.42% 99.42%
=======================================
Files 125 125
Lines 5726 5726
Branches 357 357
=======================================
Hits 5693 5693
Misses 18 18
Partials 15 15 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 at Codecov. |
Co-authored-by: Heitor Lessa <[email protected]>
5b516c2
to
84eb4b5
Compare
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.
Some quick suggestions, praises (my god, amazing work), and two issues spotted: 1/ Pip is bringing a wheel for X86 despite building for ARM (works but slower), 2/ Sin from the past on how we mistakenly accepted a PR with email-validator
where we're downloading all available versions before choosing one.
) | ||
|
||
execution_role.add_to_policy( | ||
PolicyStatement(effect=Effect.ALLOW, actions=["lambda:GetFunction"], resources=["*"]) | ||
PolicyStatement( | ||
effect=Effect.ALLOW, actions=["lambda:GetFunction"], resources=["*"] |
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.
@am29d please remind me again why do we need this permission in the Lambda itself? leftover?
Looking at the new canary I don't see an API call that would require this permission.
Disregard my comment on JMESPath - we need to cater for PyPi customers too! We just need to define the JMESPath as an optional dependency for extra to work |
Co-authored-by: Heitor Lessa <[email protected]>
Co-authored-by: Heitor Lessa <[email protected]>
Co-authored-by: Heitor Lessa <[email protected]>
@heitorlessa ready to go again, incorporated your feedback, and added an code path to run our e2e tests using arm64 (no CLI flag was created yet) |
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.
last changes and it's good to go! (pinky promise)
This will help customers prototyping locally without SAM CLI
This is due to jmespath and boto in general not being available locally, if a customer doesn't depend on boto3 as a dep (uses runtime)
Co-authored-by: Heitor Lessa <[email protected]>
Co-authored-by: Heitor Lessa <[email protected]>
Issue number: #1379
Summary
This PR adds support for publishing the v2 layer of Lambda Powertools for Python
Changes
User experience
After merging this we should be able to start testing the v2 layer on the beta branch.
In order for it to work, we need a new major release of the CDK Layer Construct
Checklist
If your change doesn't seem to apply, please leave them unchecked.
Is this a breaking change?
RFC issue number:
Checklist:
Acknowledgment
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.