fix(lambda): fix so that the `commandHook` function is called only once during bundling. #32776

ayame113 · 2025-01-07T11:07:37Z

This is my first contribution to aws-cdk! So please let me know if I'm doing something wrong.

Issue # (if applicable)

Closes #31973.

Reason for this change

When using local bundling, the commandHook function was being called twice.
This PR fixes it so that it is only called once.

Description of changes

When the variable shouldBuildImage is true, a createBundlingCommand for the Docker build will be created as before.
When the variable shouldBuildImage is false, a createBundlingCommand for the local build will be created when bundler.local.tryBundle() is called. (Previously, both a createBundlingCommand for the Docker build and for the local build were created.)

After making this change, some tests failed because they were expecting the createBundlingCommand to be called immediately, so I added forcedDockerBundling: true to some of the tests.

Note: I was going to send a patch to aws-cdk-lib, but I found a similar code in aws-lambda-go-alpha, so I fixed it at the same time. Is that okay?

Description of how you validated changes

I've added a unit test.

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

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

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

ayame113 · 2025-01-07T11:20:55Z

@Mergifyio update

mergify · 2025-01-07T11:21:10Z

update

✅ Branch has been successfully updated

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

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

lpizzinidev

Thanks 👍
A couple of comments:

I'd classify the PR as a fix(lambda) instead of a chore (that will require an integration test update as well to verify the change)
Do we need to fix the behavior for Python runtimes as well?

aws-cdk-automation

The pull request linter fails with the following errors:

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

If you believe this pull request should receive an exemption, please comment and provide a justification. A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed, add Clarification Request to a comment.

✅ A exemption request has been requested. Please wait for a maintainer's review.

ayame113 · 2025-01-09T07:49:40Z

@lpizzinidev

Thank you for the review! 👍

I'd classify the PR as a fix(lambda) instead of a chore (that will require an integration test update as well to verify the change)

I fixed the PR title. Thanks.

I have a question about integration testing.
What kind of integration testing is necessary?

a. Deploy the Lambda stack and confirm that the commandHook is called only once.
b. Deploy the Lambda stack and confirm that no errors occur.

In the case of a, I'm wondering how to test it. Can I use jest for integration testing?
In the case of b, can I be exempted from adding a test because it is covered by existing tests?

Do we need to fix the behavior for Python runtimes as well?

I think this logic is unnecessary for the Python runtime because there is no distinction between Docker bundling and Local bundling.

lpizzinidev · 2025-01-10T07:28:51Z

@ayame113 Thanks for the follow up 👍

What kind of integration testing is necessary?

Looking at the docs, following

Changing functionality that affects asset bundling

I guess an update/re-run to this Node test and this GO test should be enough.

ayame113 · 2025-01-13T06:59:38Z

@lpizzinidev

I guess an update/re-run to this Node test and this GO test should be enough.

Thanks for the clarification! 👍
I reran those two tests and confirmed there were no snapshot changes that needed to be committed.
(This is expected, since the bundling results do not change whether the commandHooks are called twice or once.)

Please let me know if there are any other changes that should be added.

Verifying integration test snapshots...

   Waiting for 1 more (integ.function)
  UNCHANGED  integ.function 419.943s

Snapshot Results: 

Tests:    1 passed, 1 total

lpizzinidev · 2025-01-14T15:20:43Z

@ayame113
Thanks for running the tests 👍
I think it would still be useful to update an integration test to make the build ✅ and have this PR merged earlier.

a. Deploy the Lambda stack and confirm that the commandHook is called only once.

Can we use an invokeFunction assertion to verify that the Lambda is bundling once? (sample code)

ayame113 · 2025-01-17T13:19:31Z

@lpizzinidev

I think it would still be useful to update an integration test to make the build ✅ and have this PR merged earlier.

Understood. Thanks for pointing that out!

Can we use an invokeFunction assertion to verify that the Lambda is bundling once? (sample code)

I think it's difficult.
In the previous behavior, the commandHook functions (beforeBundling, afterBundling) are called twice, but the command is actually executed once.
It would be nice if I could change the result of the Lambda invocation depending on the number of times commandHook was called and assert it, but I don't think there is a way to do that.

The test I can think of is the following.
Record the number of times beforeBundling and afterBundling are called, and if they are called more than twice, generate an error.
How about adding this inside an existing test?

@@ -15,11 +15,30 @@ class TestStack extends Stack {
   constructor(scope: Construct, id: string, props?: StackProps) {
     super(scope, id, props);
 
+    let afterBundlingCallCount = 0;
+    let beforeBundlingCallCount = 0;
+
     this.lambdaFunction = new lambda.GoFunction(this, 'go-handler-docker', {
       entry: path.join(__dirname, 'lambda-handler-vendor', 'cmd', 'api'),
       bundling: {
         forcedDockerBundling: true,
         goBuildFlags: ['-mod=readonly', '-ldflags "-s -w"'],
+        commandHooks: {
+          afterBundling() {
+            if (1 < afterBundlingCallCount) {
+              throw new Error('afterBundling should called only once');
+            }
+            afterBundlingCallCount++;
+            return [];
+          },
+          beforeBundling() {
+            if (1 < beforeBundlingCallCount) {
+              throw new Error('afterBundling should called only once');
+            }
+            beforeBundlingCallCount++;
+            return [];
+          },
+        },
       },
     });
   }

lpizzinidev · 2025-01-17T16:55:26Z

@ayame113 Thanks a lot for looking into this!
Please adjust the integration tests as you suggest.
After looking into it further, I don't see a way to test it with assertions either.
I'm sure maintainers will provide more feedback if necessary.
I'll approve when the code is updated 🙏

…led only once

ayame113 · 2025-01-18T10:32:04Z

Please adjust the integration tests as you suggest.
After looking into it further, I don't see a way to test it with assertions either.
I'm sure maintainers will provide more feedback if necessary.
I'll approve when the code is updated 🙏

@lpizzinidev
Thanks for looking into it! I've updated the integration tests according to your suggestions.

lpizzinidev

👍

ayame113 · 2025-01-28T11:13:01Z

Hi! Can I get a review from the maintainer for this PR?

@pahud
(I mentioned you because you commented on the referenced issue.)

aws-cdk-automation · 2025-01-31T00:11:46Z

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.

codecov · 2025-02-01T00:08:45Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.39%. Comparing base (ac3ffa5) to head (d36dede).
Report is 262 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #32776      +/-   ##
==========================================
- Coverage   81.52%   81.39%   -0.14%     
==========================================
  Files         222      225       +3     
  Lines       13715    13714       -1     
  Branches     2417     2411       -6     
==========================================
- Hits        11181    11162      -19     
- Misses       2254     2277      +23     
+ Partials      280      275       -5

Flag	Coverage Δ
suite.unit	`81.39% <ø> (-0.14%)`	⬇️

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

Components	Coverage Δ
packages/aws-cdk	`80.70% <ø> (-0.27%)`	⬇️
packages/aws-cdk-lib/core	`82.10% <ø> (+<0.01%)`	⬆️

ayame113 · 2025-02-01T23:38:24Z

The pull request linter fails with the following errors:
❌ Fixes must contain a change to an integration test file and the resulting snapshot.
❌ CodeCov is indicating a drop in code coverage
If you believe this pull request should receive an exemption, please comment and provide a justification. A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed, add Clarification Request to a comment.

I have already addressed this change request.

Exemption Request

Is there anything else I should change? Or can I get it reviewed by the maintainers?

ayame113 · 2025-02-01T23:42:29Z

@Mergifyio update

mergify · 2025-02-01T23:42:38Z

update

❌ Mergify doesn't have permission to update

For security reasons, Mergify can't update this pull request. Try updating locally.
GitHub response: refusing to allow a GitHub App to create or update workflow .github/workflows/README.md without workflows permission

aws-cdk-automation · 2025-02-02T10:13:07Z

AWS CodeBuild CI Report

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

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

GavinZZ · 2025-02-21T19:33:42Z

Closing this PR in regards to the discussion #31973. Feel free to draft up a new PR. Thank you!

github-actions · 2025-02-21T19:33:55Z

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.

fix: commandHook should only be called once in local bundling

1e10bed

ayame113 changed the title ~~chore(aws-cdk-lib): Fix so that the commandHook` function is called only once during bundling.~~ chore(aws-cdk-lib): Fix so that the commandHook function is called only once during bundling. Jan 7, 2025

github-actions bot added beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK bug This issue is a bug. effort/medium Medium work item – several days of effort p2 labels Jan 7, 2025

aws-cdk-automation requested a review from a team January 7, 2025 11:07

aws-cdk-automation previously requested changes Jan 7, 2025

View reviewed changes

ayame113 changed the title ~~chore(aws-cdk-lib): Fix so that the commandHook function is called only once during bundling.~~ chore(aws-cdk-lib): fix so that the commandHook function is called only once during bundling. Jan 7, 2025

Merge branch 'main' into fix-commandhook-called-twice-in-local-bundling

2210271

ayame113 changed the title ~~chore(aws-cdk-lib): fix so that the commandHook function is called only once during bundling.~~ fix(aws-cdk-lib): fix so that the commandHook function is called only once during bundling. Jan 7, 2025

aws-cdk-automation previously requested changes Jan 7, 2025

View reviewed changes

ayame113 changed the title ~~fix(aws-cdk-lib): fix so that the commandHook function is called only once during bundling.~~ chore(aws-cdk-lib): fix so that the commandHook function is called only once during bundling. Jan 7, 2025

aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jan 7, 2025

lpizzinidev suggested changes Jan 7, 2025

View reviewed changes

ayame113 changed the title ~~chore(aws-cdk-lib): fix so that the commandHook function is called only once during bundling.~~ fix(lambda): fix so that the commandHook function is called only once during bundling. Jan 9, 2025

aws-cdk-automation requested changes Jan 9, 2025

View reviewed changes

aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jan 9, 2025

update integ test to assert that beforeBundling/afterBundling are cal…

d36dede

…led only once

lpizzinidev approved these changes Jan 18, 2025

View reviewed changes

ayame113 requested a review from aws-cdk-automation January 28, 2025 01:21

github-actions bot mentioned this pull request Feb 1, 2025

Monthly PR metrics report #33266

Closed

ayame113 added 2 commits February 2, 2025 08:59

Merge branch 'main' into fix-commandhook-called-twice-in-local-bundling

7603b88

updating test

781d441

aaythapa assigned aaythapa and unassigned aaythapa Feb 7, 2025

GavinZZ mentioned this pull request Feb 19, 2025

(aws-lambda-nodejs): commandHooks is called twice when local bundling #31973

Closed

1 task

GavinZZ closed this Feb 21, 2025

github-actions bot locked as resolved and limited conversation to collaborators Feb 21, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(lambda): fix so that the `commandHook` function is called only once during bundling. #32776

fix(lambda): fix so that the `commandHook` function is called only once during bundling. #32776

ayame113 commented Jan 7, 2025

aws-cdk-automation left a comment

ayame113 commented Jan 7, 2025

mergify bot commented Jan 7, 2025

aws-cdk-automation left a comment

lpizzinidev left a comment

aws-cdk-automation left a comment •

edited

Loading

ayame113 commented Jan 9, 2025

lpizzinidev commented Jan 10, 2025

ayame113 commented Jan 13, 2025 •

edited

Loading

lpizzinidev commented Jan 14, 2025

ayame113 commented Jan 17, 2025

lpizzinidev commented Jan 17, 2025

ayame113 commented Jan 18, 2025

lpizzinidev left a comment

ayame113 commented Jan 28, 2025

aws-cdk-automation commented Jan 31, 2025

codecov bot commented Feb 1, 2025 •

edited

Loading

ayame113 commented Feb 1, 2025

ayame113 commented Feb 1, 2025

mergify bot commented Feb 1, 2025

aws-cdk-automation commented Feb 2, 2025

GavinZZ commented Feb 21, 2025

github-actions bot commented Feb 21, 2025

fix(lambda): fix so that the commandHook function is called only once during bundling. #32776

fix(lambda): fix so that the commandHook function is called only once during bundling. #32776

Conversation

ayame113 commented Jan 7, 2025

Issue # (if applicable)

Reason for this change

Description of changes

Description of how you validated changes

Checklist

aws-cdk-automation left a comment

Choose a reason for hiding this comment

ayame113 commented Jan 7, 2025

mergify bot commented Jan 7, 2025

✅ Branch has been successfully updated

aws-cdk-automation left a comment

Choose a reason for hiding this comment

lpizzinidev left a comment

Choose a reason for hiding this comment

aws-cdk-automation left a comment • edited Loading

Choose a reason for hiding this comment

ayame113 commented Jan 9, 2025

lpizzinidev commented Jan 10, 2025

ayame113 commented Jan 13, 2025 • edited Loading

lpizzinidev commented Jan 14, 2025

ayame113 commented Jan 17, 2025

lpizzinidev commented Jan 17, 2025

ayame113 commented Jan 18, 2025

lpizzinidev left a comment

Choose a reason for hiding this comment

ayame113 commented Jan 28, 2025

aws-cdk-automation commented Jan 31, 2025

codecov bot commented Feb 1, 2025 • edited Loading

Codecov Report

ayame113 commented Feb 1, 2025

ayame113 commented Feb 1, 2025

mergify bot commented Feb 1, 2025

❌ Mergify doesn't have permission to update

aws-cdk-automation commented Feb 2, 2025

AWS CodeBuild CI Report

GavinZZ commented Feb 21, 2025

github-actions bot commented Feb 21, 2025

fix(lambda): fix so that the `commandHook` function is called only once during bundling. #32776

fix(lambda): fix so that the `commandHook` function is called only once during bundling. #32776

aws-cdk-automation left a comment •

edited

Loading

ayame113 commented Jan 13, 2025 •

edited

Loading

codecov bot commented Feb 1, 2025 •

edited

Loading