-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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(lambda-python): add support for custom build image #15324
feat(lambda-python): add support for custom build image #15324
Conversation
ef68cfa
to
8feb3b7
Compare
8feb3b7
to
8a44747
Compare
@nija-at : This PR is ready for review. |
8a44747
to
417ccb1
Compare
The CI test is failing on a test related to |
Thanks so much for submitting this pull request. I am marking this pull request as We use +1s to help prioritize our work, and are happy to revaluate this pull request based on community feedback. |
Thanks, @nija-at. A few questions:
|
417ccb1
to
7adac47
Compare
@nija-at : Would appreciate any thoughts / comments about #15324 (comment) and this PR generally. Especially since without this PR, |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
@setu4993 whilst I think this is great, I don't think it actually solves the Code Artifact issue #10298, because as discussed in #16234 (comment), Code Artifact credentials are emphemeral, they are only valid for 12 hours. My understanding of Docker images is that they're relatively durable. I would expect that for a Function definition with |
@SamStephens : Thanks for your feedback. The reason I think this solves #10298 is that Code Artifact credentials can be passed into the Docker image at the build step. That allows installing private Python packages from Code Artifact, and then packaging the function to be executed. The credentials expiring later doesn't matter since the packages are already cached into the function once it has been built. So, the idea would be that a custom We have had success doing something similar for Fargate-based Docker images. |
924c117
to
ebfd5f2
Compare
```ts | ||
new PythonFunction(this, 'MyFunction', { | ||
... | ||
dockerBuildImageOptions: { |
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.
@setu4993 I think that I would prefer to adopt the same approach that is used by both NodejsFunction
and GoFunction
and just allow the user to supply their own DockerImage
. i.e.
new lambda.PythonFunction(this, 'my-handler', {
bundling: {
dockerImage: DockerImage.fromBuild('/path/to/Dockerfile'),
},
});
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.
Thanks for the feedback, @corymhall! I should have some space to work on this and update it over the weekend.
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.
@corymhall : Thinking through this a bit more, I'm open to switching to that structure, but I think it'd require quite a bit of refactoring of the current dependency installation system since this isn't inheriting from cdk.BundlingOptions
right now... I'm open to doing that (possibly in a separate PR first) and then inheriting those changes to support bundling here. Thoughts?
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.
@setu4993 If you are willing to work on that refactoring then I think it is a good idea! Before making this library stable we would want to bring it in line with the other two anyway.
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.
@corymhall : What do you think about #18082? The allure of getting lambda-Python
to stable (finally!) was enough motivation :).
Closing in favor of #18082. |
Adding support for custom build images for packaging Python dependencies. This resolves #10298 since Code Artifact login step with AWS CLI or credentials can be passed in with Docker build args, handled within the custom Docker build image.
This also makes #15306 redundant, so I closed it.
Fixes #10298, #12949 and #16234.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license