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

feat(core): local bundling provider #9564

Merged
merged 17 commits into from
Aug 11, 2020
Merged

feat(core): local bundling provider #9564

merged 17 commits into from
Aug 11, 2020

Conversation

jogold
Copy link
Contributor

@jogold jogold commented Aug 10, 2020

The local bundling provider implements a method tryBundle() which should
return true if local bundling was performed. If false is returned, docker
bundling will be done.

This allows to improve bundling performance when the required dependencies are
available locally.


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

@jogold
Copy link
Contributor Author

jogold commented Aug 10, 2020

@eladb let's first discuss the concept... currently works with aws-lambda-nodejs.

@jogold
Copy link
Contributor Author

jogold commented Aug 10, 2020

Bundling is currently documented here: https://github.com/aws/aws-cdk/blob/master/packages/%40aws-cdk/aws-s3-assets/README.md#asset-bundling

Should I add the doc about local bundling there? Move everything to core?

eladb
eladb previously requested changes Aug 10, 2020
packages/@aws-cdk/aws-lambda-nodejs/lib/bundling.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/core/lib/asset-staging.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/core/lib/bundling.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/core/lib/bundling.ts Outdated Show resolved Hide resolved
Co-authored-by: Elad Ben-Israel <benisrae@amazon.com>
@mergify mergify bot dismissed eladb’s stale review August 10, 2020 17:47

Pull request has been modified.

@jogold jogold changed the title feat(core): allow bundling to run locally feat(core): local bundling provider Aug 11, 2020
@jogold
Copy link
Contributor Author

jogold commented Aug 11, 2020

newly required property 'docker' used to be missing... breaking change in aws-lambda and core, should we just add local and not nest the existing props inside docker?

@eladb
Copy link
Contributor

eladb commented Aug 11, 2020

newly required property 'docker' used to be missing... breaking change in aws-lambda and core, should we just add local and not nest the existing props inside docker?

Yes I figured. Let's just keep the current api (without the docker indirection) and add local. Just make sure docstrings for all existing properties indicate that these are for the docker execution.

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.

Minor

packages/@aws-cdk/core/lib/bundling.ts Show resolved Hide resolved
packages/@aws-cdk/core/lib/bundling.ts Show resolved Hide resolved
packages/@aws-cdk/core/test/test.staging.ts Outdated Show resolved Hide resolved
@jogold
Copy link
Contributor Author

jogold commented Aug 11, 2020

@eladb will leave this in Draft while I test that I can implement local bundling correctly in aws-lambda-nodejs.

@jogold jogold marked this pull request as ready for review August 11, 2020 15:29
path: '/path/to/asset',
bundling: {
local: {
tryBundler(outputDir: string, options: BundlingOptions) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be tryBundle no?

@mergify
Copy link
Contributor

mergify bot commented Aug 11, 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: 398e8e2
  • 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 Aug 11, 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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants