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

fix: do not skip bundling for stacks during CDK Import, otherwise fails #33322

Merged
merged 7 commits into from
Feb 10, 2025

Conversation

iankhou
Copy link
Contributor

@iankhou iankhou commented Feb 6, 2025

Issues #31999 #31677

Closes #31999 #31677

Reason for this change

cdk import reports changes in the stack when none are present, for NodeJS Lambda functions. This results in the command failing. The only way to work around this is to use the --force flag, which replaces stack resources. This is not ideal as it overwrites existing resources that may contain data, and is just generally unnecessary.

Description of changes

I added cdk import to a configuration that determines which CLI commands bundle code. Before this change, cdk import skipped code bundling. This is fine for regular Lambda functions that don't rely on bundling. However, NodeJSFunction does.

The original implementation of skipping bundling for certain CLI commands was introduced via Issue #9540.

cdk import was introduced two years later in PR #17666.

Describe any new or updated permissions being added

No permissions changes.

Description of how you validated changes

I added an integration test in packages/@aws-cdk-testing/framework-integ/test/aws-lambda-nodejs/test/integ.nodejs.build.images.ts.

Screenshot 2025-02-06 at 17 10 41

The test ensures that NodeJSFunction Lambdas are always bundled. I will add a canary in a follow-up PR to ensure that new CLI commands are explicitly determined to either need or not need bundling.

I manually validated changes:

  1. Created an S3 bucket: iankhou-1738867338384
  2. Ran cdk synth, cdk deploy on a stack with a NodeJSFunction, and without an S3 bucket
  3. Added code that initializes an S3 bucket and ran cdk import

Output and console:
Screenshot 2025-02-06 at 13 45 52
Screenshot 2025-02-06 at 13 45 08

Checklist


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

@github-actions github-actions bot added bug This issue is a bug. effort/medium Medium work item – several days of effort p1 labels Feb 6, 2025
@aws-cdk-automation aws-cdk-automation requested a review from a team February 6, 2025 18:34
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Feb 6, 2025
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.

(This review is outdated)

Copy link

codecov bot commented Feb 6, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.92%. Comparing base (ba485ef) to head (3d07ffe).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #33322   +/-   ##
=======================================
  Coverage   80.92%   80.92%           
=======================================
  Files         236      236           
  Lines       14253    14253           
  Branches     2490     2490           
=======================================
  Hits        11534    11534           
  Misses       2434     2434           
  Partials      285      285           
Flag Coverage Δ
suite.unit 80.92% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
packages/aws-cdk 79.73% <ø> (ø)
packages/aws-cdk-lib/core 82.20% <ø> (ø)

@iankhou iankhou changed the title do not skip bundling for stacks during CDK Import, otherwise fails fix: do not skip bundling for stacks during CDK Import, otherwise fails Feb 6, 2025
@iankhou iankhou added the pr-linter/cli-integ-tested Assert that any CLI changes have been integ tested label Feb 6, 2025
@iankhou iankhou force-pushed the iankhou-cdk-import-fails branch from 6e09df6 to 2becfd2 Compare February 6, 2025 20:57
@iankhou iankhou self-assigned this Feb 6, 2025
@iankhou iankhou marked this pull request as ready for review February 7, 2025 00:29
@iankhou iankhou requested a review from a team as a code owner February 7, 2025 00:29
@iankhou iankhou added the pr-linter/exempt-integ-test The PR linter will not require integ test changes label Feb 7, 2025
@aws-cdk-automation aws-cdk-automation dismissed their stale review February 7, 2025 18:01

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

@aws-cdk-automation aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Feb 7, 2025
@iankhou iankhou removed the request for review from aws-cdk-automation February 7, 2025 18:54
Copy link
Contributor

mergify bot commented Feb 10, 2025

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

@mergify mergify bot temporarily deployed to test-pipeline February 10, 2025 15:04 Inactive
@aws-cdk-automation
Copy link
Collaborator

➡️ PR build request submitted to test-main-pipeline ⬅️

A maintainer must now check the pipeline and add the pr-linter/cli-integ-tested label once the pipeline succeeds.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

Copy link
Contributor

mergify bot commented Feb 10, 2025

Thank you for contributing! Your pull request will be updated from main 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 5160796 into main Feb 10, 2025
20 checks passed
@mergify mergify bot deleted the iankhou-cdk-import-fails branch February 10, 2025 15:35
Copy link

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 10, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug This issue is a bug. contribution/core This is a PR that came from AWS. effort/medium Medium work item – several days of effort p1 pr/needs-maintainer-review This PR needs a review from a Core Team Member pr-linter/cli-integ-tested Assert that any CLI changes have been integ tested pr-linter/exempt-integ-test The PR linter will not require integ test changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cdk import picking false changes for NodejsFunction
3 participants