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(lambda-nodejs): logLevel property of BundlingOptions is ignored when nodeModules are defined #18456

Merged
merged 6 commits into from
Feb 28, 2022

Conversation

flavioleggio
Copy link
Contributor

@flavioleggio flavioleggio commented Jan 15, 2022

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

@gitpod-io
Copy link

gitpod-io bot commented Jan 15, 2022

@flavioleggio flavioleggio changed the title fix(lambda-nodejs) logLevel property of BundlingOptions is ignored when nodeModules are defined feat(lambda-nodejs) logLevel property of BundlingOptions is ignored when nodeModules are defined Jan 15, 2022
@mergify
Copy link
Contributor

mergify bot commented Jan 15, 2022

Title does not follow the guidelines of Conventional Commits. Please adjust title before merge.

@flavioleggio flavioleggio changed the title feat(lambda-nodejs) logLevel property of BundlingOptions is ignored when nodeModules are defined fix(lambda-nodejs): logLevel property of BundlingOptions is ignored when nodeModules are defined Jan 15, 2022
@corymhall
Copy link
Contributor

@jogold can you take a look?

@jogold
Copy link
Contributor

jogold commented Jan 25, 2022

@jogold can you take a look?

Will do, this week or next week.

@flavioleggio
Copy link
Contributor Author

@jogold did you have a chance to take a look at this?

Copy link
Contributor

@jogold jogold left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the late reply @flavioleggio.

This looks good.

Can you update the jsdoc in types.ts both for the LogLevel enum and the logLevel prop to reflect that this log level doesn't only apply to esbuild now?

@flavioleggio
Copy link
Contributor Author

Hi @jogold

Sorry for the late reply @flavioleggio.

No worries!

Can you update the jsdoc in types.ts both for the LogLevel enum and the logLevel prop to reflect that this log level doesn't only apply to esbuild now?

Done

@jogold
Copy link
Contributor

jogold commented Feb 28, 2022

@corymhall LGTM

@mergify
Copy link
Contributor

mergify bot commented Feb 28, 2022

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

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
  • Commit ID: a53dafd
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@mergify mergify bot merged commit 5c40b90 into aws:master Feb 28, 2022
@mergify
Copy link
Contributor

mergify bot commented Feb 28, 2022

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

@flavioleggio flavioleggio deleted the feature/propagate-log-level branch February 28, 2022 19:16
TheRealAmazonKendra pushed a commit to TheRealAmazonKendra/aws-cdk that referenced this pull request 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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