feat(backend-deploy): use packageManagerController #947

0618 · 2024-01-24T20:11:28Z

Problem

In #901 , we used process.env to detect the packge manager. This PR refactor that to PackageManagerController.

Issue number, if available:

Changes

Move PackageManagerController to cli-core package
Add runWithPackageManager to getPackageManagerCommandArgs packgeManagerController
Use PackageManagerController to detect the Package Manager

Corresponding docs PR, if applicable:

Validation

Checklist

If this PR includes a functional change to the runtime behavior of the code, I have added or updated automated test coverage for this change.
If this PR requires a change to the Project Architecture README, I have included that update in this PR.
If this PR requires a docs update, I have linked to that docs PR above.
If this PR modifies E2E tests, makes changes to resource provisioning, or makes SDK calls, I have run the PR checks with the run-e2e label set.

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

…ckageManager

changeset-bot · 2024-01-24T20:11:31Z

🦋 Changeset detected

Latest commit: c3c517b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 6 packages

Name	Type
@aws-amplify/integration-tests	Patch
@aws-amplify/backend-deployer	Patch
create-amplify	Patch
@aws-amplify/cli-core	Patch
@aws-amplify/backend-cli	Patch
@aws-amplify/sandbox	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

edwardfoyle · 2024-01-24T20:40:07Z

packages/backend-deployer/src/cdk_deployer.ts

@@ -8,6 +7,7 @@ import {
  DestroyResult,
 } from './cdk_deployer_singleton_factory.js';
 import { CdkErrorMapper } from './cdk_error_mapper.js';
+import { type PackageManagerControllerFactory } from '@aws-amplify/cli-core';


we should split this into types that come from plugin-types and impl in cli-core

edwardfoyle · 2024-01-24T20:44:02Z

packages/backend-deployer/src/cdk_deployer.ts

-    this.packageManager =
-      this.packageManager === 'npm' ? 'npx' : this.packageManager;
+    this.packageManagerController =
+      this.packageManagerControllerFactory.getPackageManagerController();


seems like we should inject the packageManagerController directly rather than injecting the factory?

edwardfoyle · 2024-01-24T20:46:11Z

packages/backend-deployer/src/cdk_deployer_singleton_factory.ts

      BackendDeployerFactory.instance = new CDKDeployer(
        new CdkErrorMapper(),
        new BackendLocator(),
-        packageManager
+        new PackageManagerControllerFactory('./')


does ./ default to process.cwd()? If so, it might be better to just have that be the default with no required input (or no input at all if it's always the current directory)

Done f6ed3a6

edwardfoyle · 2024-01-24T20:49:23Z

packages/backend-deployer/src/cdk_deployer_singleton_factory.ts

@@ -2,6 +2,7 @@ import { BackendIdentifier } from '@aws-amplify/plugin-types';
 import { CDKDeployer } from './cdk_deployer.js';
 import { CdkErrorMapper } from './cdk_error_mapper.js';
 import { BackendLocator } from '@aws-amplify/platform-core';
+import { PackageManagerControllerFactory } from '@aws-amplify/cli-core';


My spidey senses are tingling with "backend-deployer" depending on "cli-core". I almost wonder if it would be better to have a completely separate package-manager-controller package 😂 . Or we can go with this for now with a GH issue to revisit if cli-core needs to be split into more pieces.

In addition.
This will trigger this check

amplify-backend/scripts/check_dependencies.ts

Lines 10 to 15 in 40298a4

'@aws-amplify/cli-core': {

allowList: [

'@aws-amplify/backend-cli',

'@aws-amplify/sandbox',

'create-amplify',

],

This would be solved by the discussion here: #947 (comment)

edwardfoyle · 2024-01-24T20:50:36Z

packages/cli-core/src/index.ts

@@ -1,3 +1,4 @@
 export * from './prompter/amplify_prompts.js';
 export { COLOR } from './colors.js';
 export * from './printer/printer.js';
+export * from './package-manager-controller/index.js';


I'd prefer to export directly from the files where these things are defined. Doing this makes it one extra click to drill down to the definitions from this file.

edwardfoyle · 2024-01-24T20:54:50Z

packages/cli-core/src/package-manager-controller/package_manager_controller.ts

+    options?: {
+      env?: Record<string, string>;
+      stdin?: 'inherit' | 'pipe' | 'ignore';
+      stdout?: 'inherit' | 'pipe' | 'ignore';
+      stderr?: 'inherit' | 'pipe' | 'ignore';
+      extendEnv?: boolean;
+    }


can we use a type from Execa for this?

Done and reverted 30cd61e, since it failed the build.

I'm trying again

edwardfoyle · 2024-01-24T21:01:15Z

packages/backend-deployer/src/cdk_deployer.ts

@@ -77,8 +78,7 @@ export class CDKDeployer implements BackendDeployer {
   * Wrapper for the child process executor. Helps in unit testing as node:test framework
   * doesn't have capabilities to mock exported functions like `execa` as of right now.
   */
-  executeChildProcess = async (
-    command: string,
+  executeChildProcessWithPackageManager = async (


Suggested change

executeChildProcessWithPackageManager = async (

executeCommand = async (

It's up to the implementation how that command gets executed

Done a2f0e5a

edwardfoyle · 2024-01-24T21:05:46Z

packages/cli-core/src/package-manager-controller/package_manager_controller.ts

+    );
+  }
+
+  getPackageManagerCommandArgs = (


this should go back to the cdk deployer. The only thing here that the package manager controller needs to expose is something like getCdkAppCommand which returns '${this.binaryRunner} tsx ${backendLocator.locate()}'. Then that can be plugged into the rest of the CDK args by the cdk deployer.

edwardfoyle · 2024-01-24T21:11:10Z

packages/cli-core/src/printer.ts

+  ? LogLevel.DEBUG
+  : LogLevel.INFO;
+
+export const printer = new Printer(minimumLogLevel);


We should either

Inject the printer from cli as needed into cli-core

Depend on this printer instance in cli and create-amplify rather than creating instances in those packages

Otherwise we are going to have multiple instances of the printer which is supposed to be a singleton. Option 2 is probably easier but option 1 is the "most correct" in terms of dependency inversion. However, we are already using the pattern of importing the printer across the cli package rather than injecting it, so perhaps option 2 is the way to go.

@sobolk thoughts?

also see https://github.com/aws-amplify/amplify-backend/pull/903/files

I think this #947 (comment) could be a solution for this too.
If package manager controller is type in platform types but it's factory is in cli-core then that factory may accept printer instance in some way (via type I guess, like we pass it to sandbox)

Yeah that would work. That would basically be option 1

We need to replace the printer instances here and here with this instance. This can go in a follow up PR if you want.

We should also consider making Printer a type in @aws-amplify/plugin-types and only export an instance from @aws-amplify/cli-core rather than the class. This would make testing more annoying so we could also consider a lint rule to forbid new instances of Printer. Probably better to cut a GH issue to track this than do it all in this PR.

Good catch. issue cut #958

edwardfoyle · 2024-01-24T21:12:31Z

packages/create-amplify/src/amplify_project_creator.ts

+            this.packageManagerController.projectRoot.replace(
+              process.cwd(),
+              ''
+            ) as string


why aren't the types for these values being inferred correctly?

Good catch! Done 44d99a7

sobolk · 2024-01-24T20:28:56Z

packages/backend-deployer/package.json

@@ -18,6 +18,7 @@
  },
  "license": "Apache-2.0",
  "dependencies": {
+    "@aws-amplify/cli-core": "^0.2.0",


if backend deployer is using package manager controller then it seems that package manager controller should go to platform-core instead.

edit:
however please see my other comment later.

There's one more follow up here. The @aws-amplify/cli-core dependency should be removed from deployer.
It can now accept type PackageManagerController which is defined in platform-types and that's the abstraction boundary here.

The new PackageManagerControllerFactory(...).getInstance() should be moved to CLI and create-amplify.

Looks like you're using the factory in tests, which is okay (although mocks would probably be better). But if you don't want to switch to mocks, moving this dep to devDeps should be okay.

moved @aws-amplify/cli-core to devDep 😬

Can tooling detect that we're using devDep in proper way ? i.e. in tests or as typings?
if not - then while test usage + devDep is legal - we may accidentally abuse it and have import in live prod execution path. Then we're at mercy of tests (e2e tests?) to detect that.

Alright. I'll mock it and remove the devDep

sobolk · 2024-01-24T21:05:26Z

packages/cli-core/src/package-manager-controller/package_manager_controller.ts

+enum InvokableCommand {
+  DEPLOY = 'deploy',
+  DESTROY = 'destroy',
+}


the CDK related stuff should remain in deployer package.

Done 1467f8f

sobolk · 2024-01-24T21:07:07Z

packages/cli-core/src/package-manager-controller/package_manager_controller.ts

+  getPackageManagerCommandArgs = (
+    invokableCommand: InvokableCommand,
+    backendLocator: BackendLocator
+  ) => [
+    'cdk',
+    invokableCommand.toString(),
+    // This is unfortunate. CDK writes everything to stderr without `--ci` flag and we need to differentiate between the two.
+    // See https://github.com/aws/aws-cdk/issues/7717 for more details.
+    '--ci',
+    '--app',
+    `'${this.binaryRunner} tsx ${backendLocator.locate()}'`,
+    '--all',
+    '--output',
+    '.amplify/artifacts/cdk.out',
+  ];


that should remain in deployer package too.

I.e. deployer should packageManagerController.runWithPackageManager(commandArgs) but it should be deployer who composes the commandArgs.

sobolk · 2024-01-24T21:14:21Z

packages/cli-core/src/package-manager-controller/package_manager_controller_factory.ts

+    const userAgent = process.env.npm_config_user_agent;
+    if (userAgent === undefined) {
+      throw new Error('npm_config_user_agent is undefined');
+    }
+    const packageManagerAndVersion = userAgent.split(' ')[0];


Since this piece is reading from process.env AND it's tightly coupled to how our CLIs are integrating with package managers I think this shouldn't be called in deployer package.

After reading this PR I think we should do something like this:

Create plain type type PackageManagerController in platform-types and extract interface.

Change existing abstract class PackageManagerController to abstract class PackageManagerControllerBase and make it package private (take it out from API.md).

Have deployer factory accept instance of type PackageManagerController

Have package manager controller factory in cli-core

Use package manager controller factory in cli and pass instance to deployer factory. (so they're decoupled via plain type).

@edwardfoyle what do you think?

definitely yes. I left a similar comment

2-3. this seems like a mixin pattern which I think might be overly complicating this. I think the package manager controller factory could just return subclasses of PackageManagerController without the additional abstraction. agreed that the only export should be the factory and the PackageManagerController type (not class)

yes

yes

edit: unless you just meant rename PackageManagerController to PackageManagerControllerBase so it doesn't conflict with the newly created PackageManagerController type. In which case, yeah that sounds fine.

I mentioned 2 in case we want to keep shared base - there's some shared logic there afaik. It's definietly optional and since it becomes package private then it becomes lesser implementation detail.

edit: unless you just meant rename PackageManagerController to PackageManagerControllerBase so it doesn't conflict with the newly created PackageManagerController type

yes that's what I meant.

1 & 2 are done

3,4,5 are done. (I think)

0618 · 2024-01-25T22:44:58Z

packages/cli-core/API.md

+}
+
+// @public
+export abstract class PackageManagerController {


It'll be moved to platform-types

sobolk · 2024-01-25T23:31:45Z

packages/cli-core/API.md

+// @public
+export class PackageManagerControllerFactory {
+    constructor(projectRoot: string);
+    getPackageManagerController(): NpmPackageManagerController | PnpmPackageManagerController | YarnClassicPackageManagerController | YarnModernPackageManagerController;


Suggested change

getPackageManagerController(): NpmPackageManagerController | PnpmPackageManagerController | YarnClassicPackageManagerController | YarnModernPackageManagerController;

getPackageManagerController(): PackageManagerController;

The return type should be type PackageManagerController.
This will allow to hide (i.e. not export) XPackageManagerController and PackageManagerControllerBase

This reverts commit 30cd61e.

edwardfoyle

Getting close! Left a few more comments and it looks like there are still some from previous reviews that need to be addressed

edwardfoyle · 2024-01-29T16:56:44Z

packages/backend-deployer/package.json

@@ -18,6 +18,7 @@
  },
  "license": "Apache-2.0",
  "dependencies": {
+    "@aws-amplify/cli-core": "^0.2.0",


Looks like you're using the factory in tests, which is okay (although mocks would probably be better). But if you don't want to switch to mocks, moving this dep to devDeps should be okay.

edwardfoyle · 2024-01-29T16:57:46Z

packages/backend-deployer/src/cdk_deployer.test.ts

+  const invoker = new CDKDeployer(
+    new CdkErrorMapper(),
+    backendLocator,
+    packageManagerControllerFactory.getPackageManagerController()


could we just mock the PackageManagerController?

Moved cli-core to devDep instead 😜 6e1257f

edwardfoyle · 2024-01-29T16:59:20Z

packages/backend-deployer/src/cdk_deployer.ts

@@ -201,7 +200,8 @@ export class CDKDeployer implements BackendDeployer {
      // See https://github.com/aws/aws-cdk/issues/7717 for more details.
      '--ci',
      '--app',
-      `'${this.packageManager} tsx ${this.backendLocator.locate()}'`,
+      // TODO: replace npx to dynamic package manager


seems like this TODO should be part of this PR?

Done 4b6b0bb

edwardfoyle · 2024-01-29T17:00:17Z

packages/backend-deployer/src/cdk_deployer_singleton_factory.ts

+   * constructor - sets the packageManagerController
+   */
+  constructor(
+    private readonly packageManagerController = packageManagerControllerFactory.getPackageManagerController()


No need for the default here. Just make it a required prop that is injected by the cli package

edwardfoyle · 2024-01-29T17:02:38Z

packages/cli-core/src/package-manager-controller/npm_package_manager_controller.ts

  /**
   * constructor
   */
  constructor(
-    readonly projectRoot: string,
+    readonly projectRoot: string = './',


also, if ./ is referring to process.cwd(), I would use that instead as it's more readable

edwardfoyle · 2024-01-29T17:24:23Z

packages/cli-core/src/package-manager-controller/package_manager_controller_base.ts

+    options?: {
+      env?: Record<string, string>;
+      stdin?: 'inherit' | 'pipe' | 'ignore';
+      stdout?: 'inherit' | 'pipe' | 'ignore';
+      stderr?: 'inherit' | 'pipe' | 'ignore';
+      extendEnv?: boolean;
+    }


how many of these options do callers of runWithPackageManager actually use? Should we expose a more limited set of props here like pipeOutput: true for example?

Done 47b6150

packages/plugin-types/src/package_manager_controller.ts

packages/plugin-types/package.json

sobolk

looks great.

sobolk · 2024-01-30T15:23:23Z

packages/backend-deployer/package.json

+  "devDependencies": {
+    "@aws-amplify/cli-core": "^0.3.0"


I think it's acceptable, but dangerous idea. (see my reply to #947 (comment) )

I'll remove it and mock the packageManagerControllerFactory in this PR

you can just mock type PackageManagerController after all refactorings right ?

edwardfoyle

Left a couple more comments but they can be follow ups

edwardfoyle · 2024-01-30T18:04:34Z

packages/backend-deployer/src/cdk_deployer.ts

@@ -92,17 +88,20 @@ export class CDKDeployer implements BackendDeployer {
      aggregatedStderr += chunk;
      done();
    };
+    const childProcess = this.packageManagerController.runWithPackageManager(
+      commandArgs,
+      './',


should this path be hardcoded here?

Good catch! done a300685

edwardfoyle · 2024-01-30T18:13:30Z

packages/cli-core/src/printer.ts

+  ? LogLevel.DEBUG
+  : LogLevel.INFO;
+
+export const printer = new Printer(minimumLogLevel);


We need to replace the printer instances here and here with this instance. This can go in a follow up PR if you want.

We should also consider making Printer a type in @aws-amplify/plugin-types and only export an instance from @aws-amplify/cli-core rather than the class. This would make testing more annoying so we could also consider a lint rule to forbid new instances of Printer. Probably better to cut a GH issue to track this than do it all in this PR.

packages/plugin-types/src/package_manager_controller.ts

update PR

* feat: create amplify skip prompts for `--yes` option (#507) * feat: add env ci support * test: update unit tests * feat: create amplify --yes option * chore: remove un-used code * feat: support Panage Manager env var * chore: add comments * chore: remove env.CI * chore: update changeset * chore: rename PACKAGE_MANAGER_EXECUTABLE * fix: tests * fix npm -> npx * feat: init e2e test workflow for Package Manager Support (#533) * test: init e2e flow test * fix GH hash * use dynamic pkg manager * setup nodejs with npm * setup more pkg managers * install pkg managers * fix yarn1 and change step name * fix yarn1 * fix yarn1 * split yarn1 into 2 steps * fix yarn1 * fix yarn1 win * exclude bun on windows * yarn1 windows * setup yarn3 * yarn3 * yarn3 yarnPath * yarn use 3rd party action * try yarn3 * set yarn 3.6.x * set yarn berry * set yarn stable and pass --yes * feat: add env ci support * test: update unit tests * feat: create amplify --yes option * chore: remove un-used code * feat: support Panage Manager env var * chore: add comments * chore: remove env.CI * chore: update changeset * chore: rename PACKAGE_MANAGER_EXECUTABLE * fix: tests * use Env Var PACKAGE_MANAGER_EXECUTABLE * fix npm -> npx, remove bun * fix npm -> npx * change -- --yes * chore: update package.lock * chore: remove @Alpha * fix: clean up yarn * fix: change yarn1 to yarn * chore: rename yarn step * chore: use env * fix: yarn-stable env var * add comments * chore: update sample app with new imports (#541) * feat: use "use client"; directive in generated react components (#540) * fix: flatten prop types auth (#534) * fix: flatten auth login with types * chore: update snapshots * chore: update api * chore: add changeset * chore: add TODO --------- Co-authored-by: Amplifiyer <51211245+Amplifiyer@users.noreply.github.com> Co-authored-by: Spencer Stolworthy <sstol@amazon.com> Co-authored-by: awsluja <110861985+awsluja@users.noreply.github.com> * fix: pnpm init and env var (#563) * fix: npm_project_initializer to use env var * fix: pnpm init * chore: update package.lock * chore: update package.lock * test: use GH workflow for e2e test of create amplify (#636) * feat: setup GH workflow for create amplify * chore: changeset * fix: add pkg type for modern yarn * fix npm install * feat: test Node v20 and Node v19 * feat: test Node v20 and yarn-stable with Node 19 * feat: exclude yarn-stable * feat: include node 20 and exclude node 19 * feat: use node 20 for others, use node 19 for yarn-stable * chore: add TODO to use Node 20 * fix: e2e tests (#771) * chore: change branch to poc/pms-create-amplify * fix: gitIgnore test * fix: refactor e2e for pms * chore: update package.lock * chore: update package.lock * temp: refactor * temp: run 1 test * temp: fix npx * temp: install packages for yarn * temp: update initial_project_file_generator * temp: install ts for yarn * temp: ignore node_modules and yarn.lock * temp: create yarn.lock for yarn stable * temp: fix yarn-stable init * temp: fix --help * fix: assert for gitignore * chore: change yarn stable install * fix: not install yarn stable globally * add @yarnpkg/sdks base * yarn stable use node-modules * remove emoji * remove @yarnpkg/sdks * yarn stable use node 18.18 * yarn stable use node 20 * chore: re-enable all tests * Revert "chore: re-enable all tests" This reverts commit f7a167b. * chore: uncomment test * chore: enable all initialStatues * test: change concurrencyLevel for yarn and yarn stable * add yarn-stable to test fails fast * chore: update changeset * chore: change workflow trigger * chore: yarn not install typescript in root folder * chore: update package.lock * Revert "chore: yarn not install typescript in root folder" This reverts commit 0d83dd0. * chore: remove comment * chore: fix typo * chore: rename yarn-classic and yarn modern * fix(create_amplify.test): after merge main * fix the concurrency for yarn classic and modern, and fix expect * feat: package manager support create amplify (#793) * chore: update package.lock * init * feat: dynamically get package manager * chore: change workflow trigger * chore: change workflow trigger * chore: rename npm functions * chore: update output log * fix: pnpm cache clear command * chore: refactor packageManager * fix: remove pnpm store * chore: create package_manager file * chore: change workflow trigger * chore: refactor PackageManager Controller and Initializer * fix: update types * chore: refactor to use factory * chore: remove try catch from the controllers * chore: change type * chore: use abstract class * fix tests * fix: PackageManagerBase * chore: refactor PackageManagerControllerFactory * chore: refactor packageJsonExists * chore: cleanup code * fix: projectJsonExists * remove package_manager file * chore: refactor package_manager props into package manager controllers * chore: rename packageManagerControllerFactory * chore: make projectRoot, userAgent, getPackageManagerName private * chore: refactor ensureInitialized * chore: refactor PackageManagerController to extend PackageManagerControllerFactory * Revert "chore: refactor PackageManagerController to extend PackageManagerControllerFactory" This reverts commit 7ddc64f. * chore: refactor to inject PackageManagerControllerFactory to xPackageManagerController * chore: move initialProjectFileGenerator to packageManagerController * fix: initializeAMplifyFolder * fix: generateInitialProjectFiles * fix: template path * chore: refactor getPackageManagerController yarn-modern * chore: create getWelcomeMessage * Revert "chore: refactor getPackageManagerController yarn-modern" This reverts commit 28da8a9. * chore: refactor generateInitialProjectFiles for yarn-modern * chore: cleanup code * chore: remove PackageManagerControllerFactory index * chore: remove ensureInitialized * Revert "chore: remove ensureInitialized" This reverts commit 96483ce. * Revert "Revert "chore: remove ensureInitialized"" This reverts commit 7c59c80. * temp: add initializeProject to PackageManagerController * temp: revert InitialProjectFileGenerator * fix: InitialProjectFileGenerator * test: restore initial_project_file_generator * move installDependencies and getWelcomeMessage to PackageManagerController * add addLockFile * add JSDocs and resolve some review comments * fix: create_amplify.test * chore: add addTypescript * chore: refactor generateInitialProjectFiles * Update packages/create-amplify/src/package-manager-controller/package_manager_controller_factory.ts Co-authored-by: Edward Foyle <foyleef@amazon.com> * chore: refactor generateInitialProjectFiles again * chore: refactor welcomeMessage * test: add package_manager_controller_factory * test: fix test types * chore: comments update * chore: handle process.env.npm_config_user_agent undefined * chore: move addLockFile and addTypescript into initializeTsConfig * test: refactor packageManagerControllerFactory * fix: yarn initializeTsConfig * fix: yarn modern initializeTsConfig * fix: amplify_project_creator test * chore: refactor contructor * chore: update package.lock * fix: pnpm init, remove --debug, etc * test: add test for NpmPackageManagerController * fix build error * chore: refactor NpmPackageManagerController * test: add test for xPackageManagerController * chore: convert fs to fsp * fix yarn modern --------- Co-authored-by: Edward Foyle <foyleef@amazon.com> * chore: update package-lock * test: add deploy test (#901) * chore: add e2e_flow.test * add deploy test but don't work * comment out deploy test * fix: e2e test assert * chore: update package.lock * add deploy test * use before and after * change amplifyCli to execa * change test trigger * Revert "change amplifyCli to execa" This reverts commit 9f85439. * setupProfile * Revert "Revert "change amplifyCli to execa"" This reverts commit 4950b7d. * remove fail test and initialStates * install @aws-amplify/backend-deployer * chore: update changeset * chore: set nodeLinker in test project * fix execaOptions syntax error * fix: npx in cdk deployer * chore: update changeset * fix: yarn modern build * chore: update package-lock * chore: remove some asserts * chore: remove synth * install packages to fix yarn * remove create-amplify help * simplify before and after * remove one packageManagerSetup * refactor packageManagerSetup * fix yarn- * move yarn add to setupPackageManager * fix type error * Revert "fix type error" and "move yarn add to setupPackageManager" This reverts commit d88ef61. * try fix pnpm for windows by adding @AWS-SDK+credential-providers * remove the pnpm patch * add nodir to dictionary * move setupPackageManager to a new file * rename PACKAGE_MANAGER_EXECUTABLE to PACKAGE_MANAGER * refactor packageManagerSetup to use switch * refactor packageManagerSetup to initializeX * use beforeEach and afterEach * add TODO comment * temp * simplify yarn-modern treatments * misc changes * add TODO comments * exclude pnpm on windows * Revert "exclude pnpm on windows" This reverts commit dd7b423. * exclude pnpm on windows * resolve review comments * fix syntax error * address review comments * move this.packageManager logic to ctor * fix yarn-modern * add packageManagerCli and packageManagerExecutable * fix yarn * address review comments * rename package_manager_sanity_check * update workflow yml * chore: update package-lock * move npm proxy to before and after * change comments * feat(backend-deploy): use packageManagerController (#947) * fix logger * fix logLevel * change workflow trigger to include poc/pms-cli-core * move package_manager_controller to cli-core * chore: update changeset * move npm_config_user_agent to getPackageManagerName and add runWithPackageManager * fix build error * modify executeWithDebugLogger * add getPackageManagerCommandArgs * refactor cdk_deployer to use package manager * install cli-core * fix execaChildProcess type * use PackageManagerControllerFactory in cdkDeployer * fix amplify/amplify path * chore: update API.md * clean up package-manager-controller export * update API.md * add type PackageManagerController * use PackageManagerController type from plugin-types * rename PackageManagerControllerBase and use plugin-types * change workflow trigger to exclude poc/pms-cli-core * refactor BackendDeployerFactory to take PackageManagerController * clean up getPackageManagerCommandArgs * add projectRoot and rename packageManagerControllerBase file * set projectRoot default value * rename execute_with_debugger_logger * use execa Options * Revert "use execa Options" This reverts commit 30cd61e. * rename executeChildProcessWithPackageManager to executeCommand * make BackendDeployerFactory param mandatory * make BackendDeployerFactory param mandatory * remove DependencyType * remove @aws-amplify/platform-core from cli-core * remove some default projectRoot * remove all default projectRoot * inject printer to factory * update package-lock * fix cli-core version in backend-deployer * move @aws-amplify/cli-core to devDep * add getCommand * change execa option type * change projectRoot to private * rename projectRoot to cwd * change ./ to cwd * remove @aws-amplify/cli-core from backend-deployer * patch dependencies_validator for execa * fix getCommand, cdk_deployer.test, amplify_project_creator.test * fix initial_project_file_generator.test * fix executeWithDebugLogger test * fix YarnModernPackageManagerController test * fix packageManagerControllerFactory test * fix lint error * Update packages/cli-core/src/package-manager-controller/package_manager_controller_factory.ts Co-authored-by: Kamil Sobol <sobkamil@amazon.com> * remove never used packageManagerExecutable * update changeset * feat: add Package Manager test to health_checks (#968) * add package-manager-tests to health-checks * remove poc-e2e-flow-test * add run_package_manager_tests_tests condition * fix run_package_manager_tests_tests condition * remove amplify-backend_windows-latest_8-core * use re-usable action for run-e2e * Revert "remove amplify-backend_windows-latest_8-core" This reverts commit 547834a. * Revert "use re-usable action for run-e2e" This reverts commit 3159e01. * revert re-usable action * Update .changeset/new-timers-warn.md Co-authored-by: Kamil Sobol <sobkamil@amazon.com> * clean up commented code * use amplify-backend_windows-latest_8-core * Update .changeset/new-timers-warn.md Co-authored-by: Edward Foyle <foyleef@amazon.com> * remove inaccurate JSdoc * remove integration-tests from changeset --------- Co-authored-by: Amplifiyer <51211245+Amplifiyer@users.noreply.github.com> Co-authored-by: Spencer Stolworthy <sstol@amazon.com> Co-authored-by: awsluja <110861985+awsluja@users.noreply.github.com> Co-authored-by: Edward Foyle <foyleef@amazon.com> Co-authored-by: Kamil Sobol <sobkamil@amazon.com>

0618 added 14 commits January 23, 2024 13:22

fix logger

31786b4

fix logLevel

e298c2c

change workflow trigger to include poc/pms-cli-core

383866c

move package_manager_controller to cli-core

c220b6e

chore: update changeset

e372a08

move npm_config_user_agent to getPackageManagerName and add runWithPa…

966c761

…ckageManager

fix build error

04a66d5

modify executeWithDebugLogger

7028672

add getPackageManagerCommandArgs

27cdbc6

refactor cdk_deployer to use package manager

56607c4

install cli-core

e86dd58

fix execaChildProcess type

c2ddf51

use PackageManagerControllerFactory in cdkDeployer

e85ce17

fix amplify/amplify path

ab0c07d

0618 requested review from edwardfoyle, sobolk, Amplifiyer and srquinn21 as code owners January 24, 2024 20:11

0618 added the run-e2e Label that will include e2e tests in PR checks workflow label Jan 24, 2024

edwardfoyle reviewed Jan 24, 2024

View reviewed changes

sobolk reviewed Jan 24, 2024

View reviewed changes

0618 added 2 commits January 24, 2024 16:16

Merge branch 'poc/package-manager-support' into poc/pms-cli-core

15b3f87

chore: update API.md

a90c36b

0618 commented Jan 25, 2024

View reviewed changes

sobolk reviewed Jan 25, 2024

View reviewed changes

0618 force-pushed the poc/pms-cli-core branch from 57f035f to a90c36b Compare January 25, 2024 23:50

0618 added 3 commits January 25, 2024 15:51

clean up package-manager-controller export

3ba6071

update API.md

9863924

add type PackageManagerController

a572c72

0618 added 2 commits January 28, 2024 20:59

Revert "use execa Options"

33d05dd

This reverts commit 30cd61e.

rename executeChildProcessWithPackageManager to executeCommand

a2f0e5a

edwardfoyle reviewed Jan 29, 2024

View reviewed changes

0618 added 15 commits January 29, 2024 09:44

make BackendDeployerFactory param mandatory

75bf30f

make BackendDeployerFactory param mandatory

7b19cef

remove DependencyType

d142221

remove @aws-amplify/platform-core from cli-core

c2e5194

remove some default projectRoot

2d94b47

remove all default projectRoot

e988c27

inject printer to factory

592cd72

Merge branch 'poc/package-manager-support' into poc/pms-cli-core

75657ec

update package-lock

4bd989c

fix cli-core version in backend-deployer

49f1b7f

move @aws-amplify/cli-core to devDep

6e1257f

add getCommand

4b6b0bb

change execa option type

47b6150

change projectRoot to private

0225440

rename projectRoot to cwd

995d05e

sobolk previously approved these changes Jan 30, 2024

View reviewed changes

edwardfoyle previously approved these changes Jan 30, 2024

View reviewed changes

0618 mentioned this pull request Jan 30, 2024

Create a printer type in plugin/types #958

Closed

0618 added 2 commits January 30, 2024 10:31

change ./ to cwd

a300685

remove @aws-amplify/cli-core from backend-deployer

c3c517b

sobolk approved these changes Jan 30, 2024

View reviewed changes

0618 merged commit 8b4619f into poc/package-manager-support Jan 30, 2024
15 checks passed

0618 deleted the poc/pms-cli-core branch January 30, 2024 22:24

0618 mentioned this pull request Jan 30, 2024

feat: package manager support #960

Merged

8 tasks

	'@aws-amplify/cli-core': {
	allowList: [
	'@aws-amplify/backend-cli',
	'@aws-amplify/sandbox',
	'create-amplify',
	],

	executeChildProcessWithPackageManager = async (
	executeCommand = async (

	getPackageManagerController(): NpmPackageManagerController \| PnpmPackageManagerController \| YarnClassicPackageManagerController \| YarnModernPackageManagerController;
	getPackageManagerController(): PackageManagerController;

feat(backend-deploy): use packageManagerController #947

feat(backend-deploy): use packageManagerController #947

Conversation

0618 commented Jan 24, 2024 • edited Loading

Problem

Changes

Validation

Checklist

changeset-bot bot commented Jan 24, 2024 • edited Loading

🦋 Changeset detected

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sobolk Jan 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edwardfoyle Jan 24, 2024 • edited Loading

Choose a reason for hiding this comment

sobolk Jan 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edwardfoyle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0618 Jan 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sobolk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edwardfoyle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0618 commented Jan 24, 2024 •

edited

Loading

changeset-bot bot commented Jan 24, 2024 •

edited

Loading

sobolk Jan 24, 2024 •

edited

Loading

edwardfoyle Jan 24, 2024 •

edited

Loading

sobolk Jan 24, 2024 •

edited

Loading

0618 Jan 30, 2024 •

edited

Loading