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

feat: lambda support for yarn2/3 and pnpm package managers #12750

Merged
merged 68 commits into from
Jun 22, 2023
Merged

feat: lambda support for yarn2/3 and pnpm package managers #12750

merged 68 commits into from
Jun 22, 2023

Conversation

lazpavel
Copy link
Contributor

@lazpavel lazpavel commented Jun 6, 2023

Description of changes

  • adds support for PNPM and Yarn 2 package managers in lambda functions
  • adds support for custom command or scripts to build lambda functions
  • adds support for PNPM and Yarn 2 package managers for custom resouces

Issue #12402 #6382 #12306

Description of how you validated changes

  • manual testing and e2e test added

Checklist

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

@lazpavel lazpavel requested a review from a team as a code owner June 6, 2023 14:48
@lazpavel lazpavel changed the title Lambda-package-manager feat: lambda support for yarn2/3 and pnpm package managers Jun 6, 2023
@codecov-commenter
Copy link

codecov-commenter commented Jun 6, 2023

Codecov Report

Merging #12750 (2690380) into dev (ec9a2ba) will increase coverage by 48.58%.
The diff coverage is 60.29%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@             Coverage Diff             @@
##              dev   #12750       +/-   ##
===========================================
+ Coverage    0.00%   48.58%   +48.58%     
===========================================
  Files        1296      842      -454     
  Lines      149743    38126   -111617     
  Branches     1296     7752     +6456     
===========================================
+ Hits            0    18524    +18524     
+ Misses     148447    18015   -130432     
- Partials     1296     1587      +291     
Impacted Files Coverage Δ
...y-category-function/src/commands/function/build.ts 0.00% <0.00%> (ø)
...ify-category-function/src/events/prePushHandler.ts 33.33% <0.00%> (+33.33%) ⬆️
...ider-utils/awscloudformation/utils/layerHelpers.ts 21.80% <0.00%> (+21.80%) ⬆️
...er-utils/awscloudformation/utils/storeResources.ts 30.38% <ø> (+30.38%) ⬆️
...ib/S3AndCloudFront/helpers/configure-CloudFront.js 87.06% <ø> (+87.06%) ⬆️
...lify-category-hosting/lib/S3AndCloudFront/index.js 89.65% <ø> (+89.65%) ⬆️
...ifications/src/commands/notifications/configure.ts 0.00% <0.00%> (ø)
...e/src/amplify-node-pkg-detector/lock-file-types.ts 100.00% <ø> (+100.00%) ⬆️
.../src/amplify-node-pkg-detector/yarn-lock-parser.ts 100.00% <ø> (+100.00%) ⬆️
packages/amplify-cli-core/src/constants.ts 100.00% <ø> (+100.00%) ⬆️
... and 45 more

... and 1250 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@@ -26,7 +26,7 @@ export class LockFileParserFactory {
}
default:
throw new AmplifyError('UnsupportedLockFileTypeError', {
message: 'Unsupported lockfile type ' + `${packageManager.lockFile} provided. Only 'npm' or 'yarn' is currently ` + 'supported.',
message: `Unsupported lockfile type ${packageManager.lockFile} provided. Only 'npm', 'yarn', and 'pnpm' are currently supported.`,
Copy link
Contributor

Choose a reason for hiding this comment

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

In this file above we have LockFileType which current has Yarn or NPM, do we not need PNPM here as well? and Yarn 3?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this parser-factory logic ends up being used only here, for a migration message: https://github.com/lazpavel/amplify-cli/blob/lambda-package-manager/packages/amplify-provider-awscloudformation/src/print-cdk-migration-warning.ts#L106-L133

since PNPM is newly added we don't currently need it being added here

…ces.ts

Co-authored-by: John Hockett <jhockett@users.noreply.github.com>
jhockett
jhockett previously approved these changes Jun 16, 2023
Copy link
Member

@edwardfoyle edwardfoyle left a comment

Choose a reason for hiding this comment

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

One small update, then I think this is g2g

Comment on lines 31 to 34
readonly packageManager = 'npm';
readonly displayValue = 'NPM';
lockFile;
executable;
version?: SemVer;

constructor() {
this.lockFile = 'package-lock.json';
this.executable = isWindows ? 'npm.cmd' : 'npm';
}
readonly executable = isWindows ? 'npm.cmd' : 'npm';
readonly lockFile = 'package-lock.json';
Copy link
Member

Choose a reason for hiding this comment

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

these props need to be readonly in the interface as well so TS will enforce it in other places

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

);

const packageManager = getPackageManagerByType('yarn');
packageManager.lockFile = 'mock.lock';
Copy link
Member

Choose a reason for hiding this comment

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

Once you make lockFile readonly on the interface you won't be able to do this, but I think that's okay

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that is the only reason I didn't make it readonly on the interface, if we are good to introduce as $TSAny, for me to keep the tests running then I can update

edwardfoyle
edwardfoyle previously approved these changes Jun 21, 2023
@@ -26,7 +27,7 @@ const npmPackageManager = getPackageManagerByType('npm');

describe('parsing yarn lock files', () => {
it('throws error when lock file not found', async () => {
yarnPackageManager.lockFile = 'yarn-test-not-found.lock';
(yarnPackageManager as $TSAny).lockFile = 'yarn-test-not-found.lock';
Copy link
Member

Choose a reason for hiding this comment

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

Seems like there's probably a way to updates these tests to check the same behavior without overriding this value, but I'm okay if that is out of scope for now

@lazpavel lazpavel merged commit fd18195 into aws-amplify:dev Jun 22, 2023
154 of 155 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants