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.

Sign up for GitHub

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

Jump to bottom

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

pavellazar
Copy link
Contributor

@pavellazar pavellazar commented Jun 6, 2023 •
edited
Loading

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.

@pavellazar pavellazar requested a review from a team as a code owner June 6, 2023 14:48
@pavellazar pavellazar 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 •
edited
Loading

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

Amplifiyer
Amplifiyer reviewed Jun 15, 2023
View reviewed changes
packages/amplify-cli-core/src/amplify-node-pkg-detector/parser-factory.ts Outdated
@@ -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

jhockett
jhockett previously approved these changes Jun 16, 2023
View reviewed changes
edwardfoyle
edwardfoyle reviewed Jun 21, 2023
View reviewed changes
Copy link
Contributor

@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

packages/amplify-cli-core/src/utils/packageManager.ts Outdated
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
Contributor

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

packages/amplify-cli/src/__tests__/commands/init.test.ts Outdated
);

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

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
View reviewed changes
...-cli-core/src/__tests__/amplify-lockfile-dependency-detector/amplify-nodejs-detector.test.ts
@@ -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
Contributor

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

@pavellazar pavellazar merged commit fd18195 into aws-amplify:dev Jun 22, 2023
@rtpascual rtpascual mentioned this pull request Sep 26, 2023
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akshbhu akshbhu akshbhu left review comments

@Amplifiyer Amplifiyer Amplifiyer left review comments

@jhockett jhockett jhockett approved these changes

@edwardfoyle edwardfoyle edwardfoyle approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
7 participants
@pavellazar @codecov-commenter @jhockett @edwardfoyle @akshbhu @Amplifiyer @git-alazara