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

(aws-lambda-nodejs): bundling logLevel property is ignored when nodeModules are required #18383

Closed
flavioleggio opened this issue Jan 12, 2022 · 1 comment · Fixed by #18456
Closed
Assignees
Labels

Comments

@flavioleggio
Copy link
Contributor

flavioleggio commented Jan 12, 2022

What is the problem?

When a stack contains a NodejsFunction construct requiring node modules and with a log level in the bundling configuration, every cdk command in the same app starts the asset bundling ignoring that logging configuration.

Reproduction Steps

Create a sample application and a stack containing this resource.

new NodejsFunction(this, "MyNodejsFunction", {
  bundling: {
    nodeModules: ["request", "request-promise"],
    logLevel: LogLevel.SILENT
  },
  entry: "path/to/lambda/code/index.js",
  runtime: Runtime.NODEJS_14_X
});

Now run whatever cdk command on that app.

What did you expect to happen?

Every command in the bundling phase should respect the specified log level.

What actually happened?

Commands like cdk ls, cdk diff, cdk deploy produce a log output even if the log level is set to LogLevel.SILENT. It seems that only the esbuild command is provided with the log level, whereas subsequent commands in the bundling chain such as npm ci run with no logging configuration.

CDK CLI Version

2.5.0

Framework Version

1.125.0

Node.js Version

v16.13.0

OS

Linux Mint 20.3

Language

Typescript

Language Version

4.1.3

Other information

I managed to temporarily work around the bug by manually setting the log level for npm ci command adding --loglevel=silent here in my compiled module under node_modules/@aws-cdk/aws-lambda-nodejs/lib/package-manager.js.

I guess this is a small effort task, as one shold just propagate the configured bundling log level to the PackageManager class and use that same value to configure the log level also for other commands than esbuild (as done here).

@flavioleggio flavioleggio added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Jan 12, 2022
@ryparker ryparker added p2 and removed needs-triage This issue or PR still needs to be triaged. labels Jan 12, 2022
@mergify mergify bot closed this as completed in #18456 Feb 28, 2022
mergify bot pushed a commit that referenced this issue Feb 28, 2022
…ed when `nodeModules` are defined (#18456)

In this pull request I managed to propagate the effect of the `logLevel` property downstream to the `PackageManager` class.

I had to refactor the static approach for constructing handler objects for the various package managers as we need to pass a new optional property in the `fromLockFile(lockFilePath: string)` static builder method.

The value for `installCommand` attribute in the constructor now depends on the log level and the approach varies between package managers: while `npm ci` supports a flexible `--loglevel` command option matching each of the `LogLevel` enum existing values, `yarn install` only supports a `--silent` option and similarly `pnpm install` supports a `--reporter=silent` which suppresses the command output except for errors and warnings. For `yarn` and `pnpm` my idea is to include the silent options whenever a log level is specified and its value is different from the default `LogLevel.INFO`.

I also adapted existing unit tests and added new ones taking into account the property's value. I have no experience with yarn and pnpm and I am not sure that any tests exist using bundling with yarn- and pnpm-based lambda function handlers, so I cannot guarantee that my changes work for that too.

Fixes #18383

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

TheRealAmazonKendra pushed a commit to TheRealAmazonKendra/aws-cdk that referenced this issue Mar 11, 2022
…ed when `nodeModules` are defined (aws#18456)

In this pull request I managed to propagate the effect of the `logLevel` property downstream to the `PackageManager` class.

I had to refactor the static approach for constructing handler objects for the various package managers as we need to pass a new optional property in the `fromLockFile(lockFilePath: string)` static builder method.

The value for `installCommand` attribute in the constructor now depends on the log level and the approach varies between package managers: while `npm ci` supports a flexible `--loglevel` command option matching each of the `LogLevel` enum existing values, `yarn install` only supports a `--silent` option and similarly `pnpm install` supports a `--reporter=silent` which suppresses the command output except for errors and warnings. For `yarn` and `pnpm` my idea is to include the silent options whenever a log level is specified and its value is different from the default `LogLevel.INFO`.

I also adapted existing unit tests and added new ones taking into account the property's value. I have no experience with yarn and pnpm and I am not sure that any tests exist using bundling with yarn- and pnpm-based lambda function handlers, so I cannot guarantee that my changes work for that too.

Fixes aws#18383

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants