fix(lambda-nodejs): fixing esbuildArgs to take in account re-specified keys #29167

Vandita2020 · 2024-02-19T18:52:47Z

Issue # (if applicable)

Reason for this change

This PR fixes a bug in CDK where CDK does not take into account re-specified keys while doing bundling. The CLI supports flags in one of three forms: --foo, --foo=bar, or --foo:bar. However, the --foo:bar form was not initially supported.
With --foo:bar, users can now specify flags that have multiple values and can be re-specified multiple times.

Description of changes

The code has a list of keys that can be re-specified multiple times. While assigning the flags it checks whether the key is among the list of re-specified keys, if yes, it specifies the flag with :.

Description of how you validated changes

The PR includes unit test and integration test both to validate the changes.

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.

jonife · 2024-02-19T18:57:51Z

packages/aws-cdk-lib/aws-lambda-nodejs/test/bundling.test.ts

@@ -245,6 +245,7 @@ test('esbuild bundling with esbuild options', () => {
      '--resolve-extensions': '.ts,.js',
      '--splitting': true,
      '--keep-names': '',
+      '--out-extension': '.js=.mjs',


why do we need this?

There is a list of esbuildArgs that uses ':' to assign the flags. Here is the list with all of them. I have added this line to check if the code is working fine. out-extension is one of the fields that uses :

so why didn't we add the others here?

The reason I chose --out-extension to test the code is because it was the one among the list that could be set without modifying much of the test code or without breaking any existing functionality.

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

jonife · 2024-02-19T21:52:37Z

Great job on fixing the integration test. However it seems you need to rebase your changes on the latest changes in the main branch.

Also can you change the title to:
fix(lambda-nodejs): fixing esbuildArgs to consider re-specified keys.
Or
fix(lambda-nodejs): fixing Bundling command to take account re-specified keys

This should be clear to the reviewers what problems you are trying to fix.

valerena · 2024-02-26T18:21:26Z

packages/aws-cdk-lib/aws-lambda-nodejs/lib/bundling.ts

@@ -418,10 +418,13 @@ function toTarget(runtime: Runtime): string {

 function toCliArgs(esbuildArgs: { [key: string]: string | boolean }): string {
  const args = new Array<string>();
+  const ls = ['--alias', '--drop', '--pure', '--log-override', '--out-extension'];


what does ls stand for here? Should this variable have a better/clearer name?

Also, does it make sense maybe to have this as a top level constant instead of just being a local variable here?
(I'm not sure about this because I'm not familiar with other functionalities or the structure of this file)

I have changed the name to reSpecifiedKeys, making it more specific to what the list is about. And I have kept the list inside toCliAgs function as it is being used inside this function only and for other functions too they have their constants declared inside the Function only.

GavinZZ · 2024-03-07T17:07:05Z

I'm a bit hesitant to approve this PR because I'm not sure if this change will cause a breaking change as technically these new args will come in effect after this PR. And there may be users who used these args in their CDK stack before the PR (and CDK falsely ignore these args, now adding effect to these args cause a behavioural change, although very unlikely).

Will get another team member to review this for a second opinion.

GavinZZ · 2024-03-07T21:22:46Z

I'm a bit hesitant to approve this PR because I'm not sure if this change will cause a breaking change as technically these new args will come in effect after this PR. And there may be users who used these args in their CDK stack before the PR (and CDK falsely ignore these args, now adding effect to these args cause a behavioural change, although very unlikely).

Will get another team member to review this for a second opinion.

Dismissing my concern, I didn't notice that before the code change, it will cause an error during deployment. Happy to approve.

GavinZZ · 2024-03-07T21:23:05Z

@mergify update

mergify · 2024-03-07T21:23:48Z

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/github-merit-badger.yml without workflows permission

mergify · 2024-03-08T22:55:18Z

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 · 2024-03-08T22:58:42Z

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).

aws-cdk-automation · 2024-03-08T23:49:28Z

AWS CodeBuild CI Report

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

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

mergify · 2024-03-08T23:49:53Z

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).

github-actions bot added bug This issue is a bug. effort/small Small work item – less than a day of effort p1 beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK labels Feb 19, 2024

aws-cdk-automation requested a review from a team February 19, 2024 18:53

aws-cdk-automation previously requested changes Feb 19, 2024

View reviewed changes

jonife reviewed Feb 19, 2024

View reviewed changes

Vandita2020 force-pushed the bundle branch 2 times, most recently from 2394c07 to 5527941 Compare February 19, 2024 21:12

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Feb 19, 2024

Vandita2020 force-pushed the bundle branch from 5527941 to 62e932f Compare February 19, 2024 22:10

Vandita2020 changed the title ~~fix(lambda-nodejs): fixing esbuildArgs to accept --foo:bar~~ fix(lambda-nodejs): fixing Bundling command to take account re-specified keys Feb 19, 2024

Vandita2020 changed the title ~~fix(lambda-nodejs): fixing Bundling command to take account re-specified keys~~ fix(lambda-nodejs): fixing esbuildArgs to take in account re-specified keys Feb 19, 2024

valerena reviewed Feb 26, 2024

View reviewed changes

Vandita2020 added 2 commits February 28, 2024 11:41

Fixing esbuildArgs

c2ae59e

review

d99415e

Vandita2020 force-pushed the bundle branch from 62e932f to d99415e Compare February 28, 2024 20:26

github-actions bot mentioned this pull request Mar 1, 2024

Monthly PR metrics report - Feb 2023 #29328

Closed

GavinZZ and others added 2 commits March 7, 2024 13:32

Merge branch 'main' into bundle

44c349c

Merge branch 'main' into bundle

7ed5197

paulhcsun approved these changes Mar 8, 2024

View reviewed changes

aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Mar 8, 2024

Merge branch 'main' into bundle

ff243f9

mergify bot merged commit 919d16f into aws:main Mar 8, 2024
9 checks passed

This was referenced May 23, 2024

[Snyk] Upgrade: , aws-cdk NOUIY/aws-solutions-constructs#98

Open

[Snyk] Upgrade: , aws-cdk NOUIY/aws-solutions-constructs#99

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(lambda-nodejs): fixing esbuildArgs to take in account re-specified keys #29167

fix(lambda-nodejs): fixing esbuildArgs to take in account re-specified keys #29167

Vandita2020 commented Feb 19, 2024 •

edited

Loading

aws-cdk-automation left a comment

jonife Feb 19, 2024

Vandita2020 Feb 19, 2024 •

edited

Loading

jonife Feb 19, 2024

Vandita2020 Feb 19, 2024 •

edited

Loading

jonife commented Feb 19, 2024

valerena Feb 26, 2024

Vandita2020 Feb 28, 2024

GavinZZ commented Mar 7, 2024

GavinZZ commented Mar 7, 2024

GavinZZ commented Mar 7, 2024

mergify bot commented Mar 7, 2024

mergify bot commented Mar 8, 2024

mergify bot commented Mar 8, 2024

aws-cdk-automation commented Mar 8, 2024

mergify bot commented Mar 8, 2024

fix(lambda-nodejs): fixing esbuildArgs to take in account re-specified keys #29167

fix(lambda-nodejs): fixing esbuildArgs to take in account re-specified keys #29167

Conversation

Vandita2020 commented Feb 19, 2024 • edited Loading

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

jonife Feb 19, 2024

Choose a reason for hiding this comment

Vandita2020 Feb 19, 2024 • edited Loading

Choose a reason for hiding this comment

jonife Feb 19, 2024

Choose a reason for hiding this comment

Vandita2020 Feb 19, 2024 • edited Loading

Choose a reason for hiding this comment

jonife commented Feb 19, 2024

valerena Feb 26, 2024

Choose a reason for hiding this comment

Vandita2020 Feb 28, 2024

Choose a reason for hiding this comment

GavinZZ commented Mar 7, 2024

GavinZZ commented Mar 7, 2024

GavinZZ commented Mar 7, 2024

mergify bot commented Mar 7, 2024

❌ Mergify doesn't have permission to update

mergify bot commented Mar 8, 2024

mergify bot commented Mar 8, 2024

aws-cdk-automation commented Mar 8, 2024

AWS CodeBuild CI Report

mergify bot commented Mar 8, 2024

Vandita2020 commented Feb 19, 2024 •

edited

Loading

Vandita2020 Feb 19, 2024 •

edited

Loading

Vandita2020 Feb 19, 2024 •

edited

Loading