test(cli): cdk import works when used on a stack containing a nodejs lambda function #33372

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

Merged

mergify merged 15 commits into main from iankhou-import-bundle-test

Feb 12, 2025

Contributor

iankhou commented Feb 11, 2025 •

edited

Loading

Improves testing for #33322.

Reason for this change

The original test (removed below) does not adequately test the CDK import fix from the above-mentioned PR. It calls CDK synth, but does not use the CLI and does not test cdk import.

Description of changes

The old test is removed.

The new test ensures that cdk import works when an already-deployed stack contains a NodeJSFunction lambda. In our test, we deploy a stack containing a NodeJSFunction Lambda. We also create, orphan, and import an S3 bucket, which replicates the customer scenario in issue #31999.

The test:

Installs esbuild so that we can use it for bundling.
Deploys a NodeJSFunction and an S3 bucket.
Orphans the S3 bucket but allows the bucket to persist (not destroyed).
Imports the S3 bucket into the template using its BucketName property.

The test will fail if the import operation fails for any reason.

The test does not assert any specific values or patterns for asset metadata keys. When investigating the issue, the metadata keys were different when using import-synth (skipped bundling) and deploy/diff/synth (did not skip bundling). The difference in the keys was another symptom of the same issue.

Describe any new or updated permissions being added

No permissions changes.

Description of how you validated changes

This PR tests the feature. I ensured that the test failed on code prior to the change in PR #33322, and passes on code following the change.

Checklist

My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

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

iankhou added 2 commits

February 10, 2025 20:35


          test to ensure asset metadata emitted from cdk import is consistent w…

48b928f

…ith other synth methods


          remove ineffective test

f503ad4

iankhou self-assigned this

github-actions bot added the p2 label

aws-cdk-automation requested a review from a team

February 11, 2025 01:45

mergify bot added the contribution/core label

iankhou temporarily deployed to test-pipeline

February 11, 2025 01:46

— with

GitHub Actions Inactive

iankhou changed the title ~~Iankhou import bundle test~~ test(cli): cdk import is bundled and emits proper metadata

iankhou changed the title ~~test(cli): cdk import is bundled and emits proper metadata~~ test(cli): cdk import bundles and emits proper metadata

iankhou marked this pull request as ready for review

February 11, 2025 01:50

iankhou requested a review from a team as a code owner

February 11, 2025 01:50

codecov bot commented Feb 11, 2025 •

edited

Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.91%. Comparing base (cce7d3e) to head (82e9ad2).

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #33372   +/-   ##
=======================================
  Coverage   80.91%   80.91%           
=======================================
  Files         236      236           
  Lines       14258    14258           
  Branches     2492     2492           
=======================================
  Hits        11537    11537           
  Misses       2436     2436           
  Partials      285      285

Flag	Coverage Δ
suite.unit	`80.91% <ø> (ø)`

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

Components	Coverage Δ
packages/aws-cdk	`79.74% <ø> (ø)`
packages/aws-cdk-lib/core	`82.18% <ø> (ø)`


          remove unnecessary imports

5265e46

iankhou temporarily deployed to test-pipeline

February 11, 2025 02:10

— with

GitHub Actions Inactive

aws-cdk-automation added the pr/needs-maintainer-review label

iliapolo requested changes

View reviewed changes

packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/cli.integtest.ts Outdated

    
               *

               */

              integTest(

                'CDK import adds the metadata properties expected by sam, validates that bundling is not skipped',

Contributor

iliapolo Feb 11, 2025

Consider the issue we're trying to fix - does this test accurately simulate the scenario described in the issue?

It does address the root cause that we identified, but it doesn't give us enough guarantees that the original issue is actually fixed. It might be that something else will block the expected behavior. Think about the manual test you ran to validate the fix works, can you reproduce that manual test here?

Deploy a stack with NodeJsFunction construct.
Create a bucket out of band
Run cdk import --resource-mappings

Should be feasible...

As for the scenarios you put here, they can serve as additional, quicker tests, that help us diagnose the issue in case of failures, but should not be the only ones. Prefer many small tests over fewer large complicated ones.

Also is the withSamIntegrationFixture really needed here? I understand it might be convenient because it has constructs you want as well, but seems like it also has a bunch of other things that we don't need. See if you can write your own stack in the normal app fixture, tailored to your use case.

Contributor Author

iankhou Feb 11, 2025

Pushed some revisions with a test that reproduces the manual steps. I think validation of the metadata is something we can do later, as a nice to have.

aws-cdk-automation removed the pr/needs-maintainer-review label

iankhou added 2 commits

February 11, 2025 11:45


          added integration test to replicate cdk import failure for NodeJSFunc…

e181de3

…tion lambdas, removed asset metadata test


          linter change

bec67fa

iankhou had a problem deploying to test-pipeline

February 11, 2025 17:10

— with

GitHub Actions Failure


          remove dead code, improve comments

32a2026

iankhou temporarily deployed to test-pipeline

February 11, 2025 17:13

— with

GitHub Actions Inactive

iankhou changed the title ~~test(cli): cdk import bundles and emits proper metadata~~ test(cli): cdk import works when used on a stack containing a nodejs lambda function


          newline at end of file

99c032f

iankhou temporarily deployed to test-pipeline

February 11, 2025 17:21

— with

GitHub Actions Inactive

iankhou requested a review from iliapolo

February 11, 2025 17:22


          linter again

iankhou temporarily deployed to test-pipeline

February 11, 2025 17:59

— with

GitHub Actions Inactive

iliapolo requested changes

View reviewed changes

packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/cli.integtest.ts Outdated

    
                  const originalPath = process.env.PATH;

                  // Install esbuild locally in the temporary directory

                  execSync('npm init -y && npm install esbuild', { cwd: tempDir, stdio: 'inherit' });

Contributor

iliapolo Feb 11, 2025

Hmm, this shouldn't be necessary...we have esbuild a bunch in our devDependencies so it should already be installed.

For example: https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk-lib/package.json#L175

See if you can without it, maybe its just a matter of locating the installed one and adding it to the path, but I believe that shouldn't be necessary as well.

Contributor Author

iankhou Feb 11, 2025

esbuild is needed as a runtime dependency to use it in integration tests, but we only keep it as a dev dependency in some packages. We do not make it available as a runtime dependency because it is not available via JSII, according to @mrgrain. We don't have it as a dependency in @aws-cdk-testing, in either framework-integ or cli-integ package.jsons. I was able to remove the code above, though, by forcing the integ test to use Docker esbuild to bundle the code instead.

packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/cli.integtest.ts Outdated

    
                  // GIVEN

                  const randomPrefix = randomString();

                  const uniqueOutputsFileName = `${randomPrefix}Outputs.json`; // other tests use the outputs file. Make sure we don't collide.

Contributor

iliapolo Feb 11, 2025

Every test already gets its own unique directory in fixture.integTestDir so you don't need this.

(I realize its like this in other tests, but they might also be wrong too or just old. double check.)

Contributor Author

iankhou Feb 11, 2025

Removed this prefix.

packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/cli.integtest.ts Outdated

    
                    process.env.PATH = originalPath;

                    // Clean up the temporary directory

                    await fs.rm(tempDir, { recursive: true, force: true });

Contributor

iliapolo Feb 11, 2025

Won't be necessary given the esbuild installation step is removed.

iankhou commented

View reviewed changes

packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/cli.integtest.ts Outdated

    
                  const originalPath = process.env.PATH;

                  // Install esbuild locally in the temporary directory

                  execSync('npm init -y && npm install esbuild', { cwd: tempDir, stdio: 'inherit' });

Contributor Author

iankhou Feb 11, 2025

We need to install esbuild to bundle the NodeJSFunction code. We install it in a temp directory, which is added to PATH.

At the end of the test, the temp directory is removed, and PATH is set back to its original value.

Contributor

iliapolo Feb 11, 2025

But esbuild is already installed after you run yarn install - there should be no need to explicitly install it during the test. There must already be a test that defines a NodeJsFunction - does it do the same?

Contributor Author

iankhou Feb 11, 2025

I did find another NodeJsFunction, it passes the option forceDockerBundling: true, which doesn't require esbuild as a runtime dependency. I've added this option to my test.


          force docker bundling to prevent using esbuild as a dependency

1cb7c41

iankhou had a problem deploying to test-pipeline

February 11, 2025 22:25

— with

GitHub Actions Failure


          remove unnecessary randomized prefix

a25b748

iankhou commented

View reviewed changes

packages/@aws-cdk-testing/cli-integ/resources/cdk-apps/app/app.js

    
                          sourceMap: false,

                          sourcesContent: false,

                          target: 'ES2020',

                          forceDockerBundling: true,

Contributor Author

iankhou Feb 11, 2025

In cli-integ, we don't keep a runtime dependency for esbuild. Uses docker esbuild instead.

iankhou had a problem deploying to test-pipeline

February 11, 2025 22:27

— with

GitHub Actions Failure


          remove commented-out code

aee42a0

iankhou temporarily deployed to test-pipeline

February 11, 2025 22:30

— with

GitHub Actions Inactive


          added additional comments to test code, reused existing lambda code

bbaab91

iankhou had a problem deploying to test-pipeline

February 12, 2025 16:52

— with

GitHub Actions Failure


          include missed changes

e036c81

iankhou added the pr-linter/cli-integ-tested label

iankhou temporarily deployed to test-pipeline

February 12, 2025 16:53

— with

GitHub Actions Inactive

aws-cdk-automation previously requested changes

View reviewed changes

Collaborator

aws-cdk-automation left a comment •

edited

Loading

(This review is outdated)

iliapolo approved these changes

View reviewed changes

iankhou requested a review from iliapolo

February 12, 2025 17:08


          Merge branch 'main' into iankhou-import-bundle-test

dc3049a

iliapolo temporarily deployed to test-pipeline

February 12, 2025 17:09

— with

GitHub Actions Inactive

aws-cdk-automation dismissed their stale review

February 12, 2025 17:10

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

iliapolo approved these changes

View reviewed changes

Collaborator

aws-cdk-automation commented Feb 12, 2025

➡️ 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.

kaizencc approved these changes

View reviewed changes

Contributor

mergify bot commented Feb 12, 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).


          Merge branch 'main' into iankhou-import-bundle-test

82e9ad2

mergify bot had a problem deploying to test-pipeline

February 12, 2025 18:12

Failure

Collaborator

aws-cdk-automation commented Feb 12, 2025

AWS CodeBuild CI Report

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

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

Contributor

mergify bot commented Feb 12, 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 bot merged commit 47a3f90 into main

19 of 20 checks passed

mergify bot deleted the iankhou-import-bundle-test branch

February 12, 2025 18:42

Contributor

github-actions bot commented Feb 12, 2025

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 bot locked as resolved and limited conversation to collaborators

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

contribution/core p2 pr-linter/cli-integ-tested