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: lambda bundling with cdk-assets #22325

Closed
wants to merge 2 commits into from

Conversation

Hi-Fi
Copy link
Contributor

@Hi-Fi Hi-Fi commented Oct 2, 2022

Currently there's no code generating asset manifest that would be using bundling. That generation is easier to build when there's some verified way to make bundling with cdk-assets.


All Submissions:

Adding new Unconventional Dependencies:

  • This PR adds new unconventional dependencies following the process described here

New Features

  • Have you added the new feature to an integration test?
    • Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

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

@gitpod-io
Copy link

gitpod-io bot commented Oct 2, 2022

@github-actions github-actions bot added the repeat-contributor [Pilot] contributed between 3-5 PRs to the CDK label Oct 2, 2022
@aws-cdk-automation aws-cdk-automation requested a review from a team October 2, 2022 18:24
@github-actions github-actions bot added the p2 label Oct 2, 2022
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

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

The Pull Request Linter fails with the following errors:

❌ The title of this pull request does not follow the Conventional Commits format, see https://www.conventionalcommits.org/.

PRs must pass status checks before we can provide a meaningful review.

@aws-cdk-automation aws-cdk-automation dismissed their stale review October 2, 2022 18:26

Pull Request updated. Dissmissing previous PRLinter Review.

Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

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

The Pull Request Linter fails with the following errors:

❌ The title of this pull request does not follow the Conventional Commits format, see https://www.conventionalcommits.org/.

PRs must pass status checks before we can provide a meaningful review.

@Hi-Fi Hi-Fi changed the title Feat/lambda bundling with cdk assets feat: lambda bundling with cdk-assets Oct 2, 2022
@Hi-Fi Hi-Fi force-pushed the feat/lambda_bundling_with_cdk-assets branch from e02a2f2 to 1451315 Compare October 2, 2022 18:27
@mergify mergify bot dismissed aws-cdk-automation’s stale review October 2, 2022 18:28

Pull request has been modified.

Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

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

The Pull Request Linter fails with the following errors:

❌ Features must contain a change to a README file.
❌ Features must contain a change to an integration test file and the resulting snapshot.

PRs must pass status checks before we can provide a meaningful review.

@aws-cdk-automation aws-cdk-automation dismissed their stale review October 2, 2022 18:29

Pull Request updated. Dissmissing previous PRLinter Review.

Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

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

The Pull Request Linter fails with the following errors:

❌ Features must contain a change to a README file.
❌ Features must contain a change to an integration test file and the resulting snapshot.

PRs must pass status checks before we can provide a meaningful review.

@Hi-Fi Hi-Fi force-pushed the feat/lambda_bundling_with_cdk-assets branch from 1451315 to b4d0c87 Compare October 2, 2022 18:35
@mergify mergify bot dismissed aws-cdk-automation’s stale review October 2, 2022 18:36

Pull request has been modified.

Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

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

The Pull Request Linter fails with the following errors:

❌ Features must contain a change to an integration test file and the resulting snapshot.

PRs must pass status checks before we can provide a meaningful review.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: b4d0c87
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

Comment on lines +23 to +24
// Hack to get things tested
(this.asset.source.path as any) = bundledPath;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently added this hack to test things, but how this should be done?

  1. add output path to assembly and pre-generate it when generating assemby?
  2. modify publish part to allow overriding path with argument?

Copy link
Contributor

@corymhall corymhall left a comment

Choose a reason for hiding this comment

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

@Hi-Fi thanks for taking the time to create this PR! Since this is labeled as a p2 and is adding some pretty significant functionality we will most likely not be able to review this without an RFC. I think there are a lot of ideas around how to enhance/refactor the assets/bundling functionality and it needs a much bigger discussion than is appropriate for a PR review.

@corymhall corymhall marked this pull request as draft October 3, 2022 12:23
@Hi-Fi
Copy link
Contributor Author

Hi-Fi commented Oct 3, 2022

@Hi-Fi thanks for taking the time to create this PR! Since this is labeled as a p2 and is adding some pretty significant functionality we will most likely not be able to review this without an RFC. I think there are a lot of ideas around how to enhance/refactor the assets/bundling functionality and it needs a much bigger discussion than is appropriate for a PR review.

@corymhall Just interested, how that p2 got here? Both referred issues are p1. At least earlier copied priority from issue.

@Naumel Naumel added the @aws-cdk/aws-lambda Related to AWS Lambda label Oct 4, 2022
@aws-cdk-automation aws-cdk-automation dismissed their stale review October 4, 2022 10:00

Pull Request updated. Dissmissing previous PRLinter Review.

Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

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

The Pull Request Linter fails with the following errors:

❌ Features must contain a change to an integration test file and the resulting snapshot.

PRs must pass status checks before we can provide a meaningful review.

@aws-cdk-automation
Copy link
Collaborator

This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

@Hi-Fi
Copy link
Contributor Author

Hi-Fi commented Oct 26, 2022

@corymhall was there something to improve in this PR, or was it just waiting auto-closure die inactivity after change to draft?

@Hi-Fi Hi-Fi marked this pull request as ready for review October 26, 2022 18:12
@aws-cdk-automation
Copy link
Collaborator

This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error.

@aws-cdk-automation aws-cdk-automation added the closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. label Nov 2, 2022
@aws-cdk-automation
Copy link
Collaborator

The pull request linter fails with the following errors:

❌ Features must contain a change to an integration test file and the resulting snapshot.

PRs must pass status checks before we can provide a meaningful review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-lambda Related to AWS Lambda closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. p2 repeat-contributor [Pilot] contributed between 3-5 PRs to the CDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants