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.

Sign up for GitHub

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

Jump to bottom

feat(lambda-python): introducing LambdaPython #9182

Merged
merged 13 commits into from
Jul 22, 2020
Merged

feat(lambda-python): introducing LambdaPython #9182

merged 13 commits into from
Jul 22, 2020

Conversation

adamdottv
Copy link
Contributor

Higher level API to work with Lambda functions written in Python.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@adamdottv
Copy link
Contributor Author

adamdottv commented Jul 21, 2020 •
edited
Loading

A few additional notes, if introducing this construct is agreed to be desirable:

  • There are chunks of code—mostly utils—that are straight copy/pasted from the Nodejs construct. I considered moving these into packages/@aws-cdk/aws-lambda/lib/util.ts but decided to wait for initial feedback.
  • I still need to work out some issues with the integration test; I'll add some comments to the test file shortly.
  • requirements.txt has to be alongside the entry file, meaning you couldn't have multiple Python lambdas (with independent requirements) in the same directory. Should it first look for stack.my_handler.requirements.txt, or something along those lines? Should the path to requirements.txt be configurable?

eladb
eladb previously requested changes Jul 22, 2020
View reviewed changes
Copy link
Contributor

@eladb eladb left a comment

Choose a reason for hiding this comment

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

Some initial comments.

@jogold can you please take a look, and consider what can we generalize between this and nodejs?

packages/@aws-cdk/aws-lambda-python/test/integ.function.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-lambda-python/lib/function.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-lambda-python/lib/function.ts Show resolved Hide resolved
packages/@aws-cdk/aws-lambda-python/lib/function.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-lambda-python/lib/function.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-lambda-python/lib/function.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-lambda-python/lib/index.ts Outdated Show resolved Hide resolved
adamdottv added 2 commits July 22, 2020 03:06
@adamdottv
feat(lambda): higher level construct for python
00fa2a3 
This construct is heavily inspired by @jogold's Nodejs construct
introduced in #5532. The Python equivalent is simpler in that no
external bundler is necessary; it's also possible that this construct
doesn't relieve enough pain to be introduced and maintained. I'd
personally love to remove some of the boilerplate needed to pip
install my python dependencies and enjoy the symmetry of defining
`lambda.PythonFunction` alongside `lambda.NodejsFunction`.
@adamdottv
address review feedback
52e3778
@mergify mergify bot dismissed eladb’s stale review July 22, 2020 08:07

Pull request has been modified.

@adamdottv
Copy link
Contributor Author

adamdottv commented Jul 22, 2020 •
edited
Loading

@eladb @jogold I've addressed all of the feedback thus far, and the result is much simpler. util.ts was removed completely which obviated the need for any sharing of code between aws-lambda-python and aws-lambda-nodejs, I believe.

@adamdottv adamdottv requested a review from eladb July 22, 2020 08:20
@adamdottv
Copy link
Contributor Author

@jogold I also considered carrying over the notion of externalModules here (defaulting to ['botocore', 'boto3']), but on the python side I've often read that it's best to include these explicitly in your lambda bundle. That sentiment could be due to requests being vendored from botocore, historically; this was changed late last year.

Not sure if you have an opinion, but wanted to bring visibility.

eladb
eladb reviewed Jul 22, 2020
View reviewed changes
Copy link
Contributor

@eladb eladb left a comment

Choose a reason for hiding this comment

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

I love how simple this is!

packages/@aws-cdk/aws-lambda-python/lib/bundling.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-lambda-python/lib/function.ts Show resolved Hide resolved
@adamdottv adamdottv requested a review from eladb July 22, 2020 11:54
@adamdottv
Copy link
Contributor Author

I love how simple this is!

Keeps getting simpler! 🎉

@jogold
Copy link
Contributor

jogold commented Jul 22, 2020

@jogold I also considered carrying over the notion of externalModules here (defaulting to ['botocore', 'boto3']), but on the python side I've often read that it's best to include these explicitly in your lambda bundle. That sentiment could be due to requests being vendored from botocore, historically; this was changed late last year.

Not sure if you have an opinion, but wanted to bring visibility.

You can control this by including/omitting boto3/botocore in your requirements.txt, no?

eladb
eladb approved these changes Jul 22, 2020
View reviewed changes
Copy link
Contributor

@eladb eladb left a comment

Choose a reason for hiding this comment

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

🚀🙏🎉

@eladb eladb changed the title feat(lambda): higher level construct for python feat(lambda-python): introducing LambdaPython Jul 22, 2020
@mergify
Copy link
Contributor

mergify bot commented Jul 22, 2020

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: b9f9cd1
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify
Copy link
Contributor

mergify bot commented Jul 22, 2020

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mergify mergify bot merged commit 4cc2834 into aws:master Jul 22, 2020
@adamdottv adamdottv deleted the adamelmore/lambda-python branch July 23, 2020 00:04
Chriscbr pushed a commit to Chriscbr/aws-cdk that referenced this pull request Jul 23, 2020
@adamdottv @Chriscbr
feat(lambda-python): introducing LambdaPython (aws#9182)
d7ca125 
Higher level API to work with Lambda functions written in Python.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@eladb eladb mentioned this pull request Jul 26, 2020
Closed
curtiseppel pushed a commit to curtiseppel/aws-cdk that referenced this pull request Aug 11, 2020
@adamdottv
feat(lambda-python): introducing LambdaPython (aws#9182)
5e86872 
Higher level API to work with Lambda functions written in Python.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jogold jogold jogold left review comments

@eladb eladb eladb approved these changes

Assignees

@ericzbeard ericzbeard

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@adamdottv @jogold @aws-cdk-automation @eladb @ericzbeard