feat: create amplify skip prompts for `--yes` option #507

0618 · 2023-10-26T19:20:41Z

Issue #, if available:

Description of changes:

If we detect --yes, skip prompts and use default options.

This PR is just a temporary change for setting up the test infra. Once the test is ready, the feature in this PR will be refactored.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

changeset-bot · 2023-10-26T19:20:45Z

🦋 Changeset detected

Latest commit: 01d8792

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

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

sobolk · 2023-10-26T19:25:46Z

packages/create-amplify/src/get_project_root.ts

+  const useDefault =
+    process.env.npm_config_yes === 'true' || process.env.CI === 'true';


We should invent our own env var to control this behavior and make it intentional.
Coupling to CI which is quite popular due to different reasons is potentially unwanted.

That makes sense. I created another env var, PM, for temporary use in the test infra. Once it's done, we can be more confidently officially implement it. What do you think?

Removed 7b88772

packages/create-amplify/src/get_project_root.test.ts

edwardfoyle · 2023-10-26T21:05:29Z

packages/create-amplify/src/get_project_root.test.ts

@@ -68,4 +68,19 @@ void describe('getProjectRoot', () => {
    );
    assert.equal(projectRoot, path.resolve(userInput));
  });
+
+  void it('use default options if `yes`', async (ctx) => {


if 'yes' is intended to mean 'default', we should name the flag 'default'. This is a huge source of confusion in the current CLI

That makes sense. I almost udpated the PR, but just realized that we can't change it for npm because npm_config_yes is set by --yes.

Do we want to have --default works for all, and --yes only works for npm?

sobolk

I think we should just create long living branch for package manager support and not merge this to main until we have bigger picture established.

sobolk · 2023-10-27T16:09:07Z

.changeset/metal-tomatoes-check.md

+'create-amplify': patch
+---
+
+Create Amplify uses default options and log verbosely when it's CI env


this needs update.

Updated a085162

sobolk · 2023-10-27T16:13:17Z

packages/create-amplify/src/logger.ts

+  argv.debug || argv.verbose || process.env.CI === 'true'
+    ? LogLevel.DEBUG
+    : LogLevel.INFO;


I was thinking it might be nice to log more info in CI 😂 but not necessary. I can remove the change

no, this has to be explicit intent.
magic , aka implicit behaviors like that are not good.

Removed 7b88772

sobolk · 2023-10-27T16:13:43Z

packages/create-amplify/src/npm_package_manager_controller.ts

@@ -15,7 +15,7 @@ export class NpmPackageManagerController implements PackageManagerController {
    private readonly projectRoot: string,
    private readonly execa = _execa
  ) {}
-  private readonly executableName = 'npm';
+  private readonly executableName = process.env.PM || 'npm'; // TODO: replace `process.env.PM` with `getPackageManagerName()` once the test infra is ready.


if we want to do this the variable name must be verbose.
E.g. AMPLIFY_CREATE_AMPLIFY_PACKAGE_MANAGER_EXECUTABLE or so.

this controller should not be modified to handle yarn and npm. It's the npm package manager controller. We need another controller for yarn.

also, the package manager to use should not come from an env var. We should use the one that the command was executed with (can be discovered in process.argv)

this controller should not be modified to handle yarn and npm. It's the npm package manager controller. We need another controller for yarn.

We can rename the npm_package_manager_controller into package_manager_controller

also, the package manager to use should not come from an env var. We should use the one that the command was executed with (can be discovered in process.argv)

It's a temporary implementation for the GH workflow. We'll eventrually use a getPackageManager() function to detect which package manager the user is using

We can rename the npm_package_manager_controller into package_manager_controller

Likelihood that we'll be able to find common set of flags/installation procedure for all package managers is low.
NpmPackageManager implements PackageManager seems to be correct model.

The env var is fine for workflow/infra testing though. But ultimately we need OO model for this.

I agree, having a NpmPackageManager implements PackageManager is more scalable, but so far the only difference is yarn add, and it's very unlikely to have more.

Anyway, even though we'll use a PackageManager, it's not in this PR. I'll set up the test infra first, then confidently mess with all the Pakcage Manager Support change 😁

I'll create a new branch, so that we don't have to merge to main

Done! poc/package-manager-support is created and changed as base branch

sobolk · 2023-10-27T16:14:33Z

packages/create-amplify/src/npm_package_manager_controller.ts

    if (type === 'dev') {
-      args.push('--save-dev');
+      args.push('-D');


does this work for both ?

Yes, -D works for all. --save-dev works for npm, --dev works for yarn

we shouldn't be making admissions for yarn in the "npm_package_manager_controller". We need a completely separate controller for yarn support

The command and options of npm, yarn, pnpm, bun are mostly the same, it isn't worth it to have 4 package_manager_controller

We should be careful not to create accidental coupling in the name of reducing duplication. Just because two implementations are the same now doesn't mean they will evolve in the same way. There's nothing but convention keeping the args of these tools similar. But they are all separate and will have their own evolutions over time. I still think we should have completely separate controllers for each one even if each controller does 90% of the same stuff as the others.

Consider the case where pnpm updates some flag and we make a change to our uber controller to handle this but it also borks yarn and npm. Vs a separate controller for each where we can update the pnpm controller with confidence that there won't be side-effects for the other controllers.

That makes sense! It'll be handled in a separated issue #517 before the feature branch poc/package-manager-support merge to main

edwardfoyle · 2023-10-27T17:43:01Z

packages/create-amplify/src/get_project_root.ts

+      default: false,
+    },
+  }).argv;
+  const useDefault = process.env.npm_config_yes === 'true' || argv.yes === true;


why couple ourselves to npm_config_yes at all? That's a behavior of npm/npx that we shouldn't need to depend on

When create amplify, user are not able to use npm_config_yes for anything else, so it's safe to borrow it for our purpose. Passing --yes is a better DX than passing -- --yes, which is why we chose to use npm_config_yes

In that case, why have argv.yes?

yarn is an exceptional ☹️

sobolk

This is fine for testing. Please create GH issue that summarizes shortcuts taken in this PR to make it work. We'll need them addressed before going to main.

0618 · 2023-10-27T21:53:57Z

This is fine for testing. Please create GH issue that summarizes shortcuts taken in this PR to make it work. We'll need them addressed before going to main.

Issue cut #517

* 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 <[email protected]> Co-authored-by: Spencer Stolworthy <[email protected]> Co-authored-by: awsluja <[email protected]> * 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 <[email protected]> * 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 <[email protected]> * 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 <[email protected]> * 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 <[email protected]> * clean up commented code * use amplify-backend_windows-latest_8-core * Update .changeset/new-timers-warn.md Co-authored-by: Edward Foyle <[email protected]> * remove inaccurate JSdoc * remove integration-tests from changeset --------- Co-authored-by: Amplifiyer <[email protected]> Co-authored-by: Spencer Stolworthy <[email protected]> Co-authored-by: awsluja <[email protected]> Co-authored-by: Edward Foyle <[email protected]> Co-authored-by: Kamil Sobol <[email protected]>

0618 added 2 commits October 26, 2023 11:58

feat: add env ci support

b41bb05

test: update unit tests

e722f11

0618 requested review from edwardfoyle, sobolk and Amplifiyer as code owners October 26, 2023 19:20

sobolk reviewed Oct 26, 2023

View reviewed changes

0618 closed this Oct 26, 2023

0618 reopened this Oct 26, 2023

0618 marked this pull request as draft October 26, 2023 19:38

feat: create amplify --yes option

7dca6da

github-advanced-security bot found potential problems Oct 26, 2023

View reviewed changes

packages/create-amplify/src/get_project_root.test.ts Fixed Show fixed Hide fixed

chore: remove un-used code

70fad76

edwardfoyle reviewed Oct 26, 2023

View reviewed changes

0618 added 2 commits October 26, 2023 21:45

feat: support Panage Manager env var

dc2df4b

chore: add comments

c423170

0618 changed the title ~~feat: skip prompts for CI env~~ feat: skip prompts for --default option Oct 27, 2023

0618 changed the title ~~feat: skip prompts for --default option~~ feat: create amplify skip prompts for --default option Oct 27, 2023

0618 changed the title ~~feat: create amplify skip prompts for --default option~~ feat: create amplify skip prompts for --yes option Oct 27, 2023

Merge branch 'main' into env-ci

518a43c

0618 marked this pull request as ready for review October 27, 2023 05:20

sobolk reviewed Oct 27, 2023

View reviewed changes

edwardfoyle reviewed Oct 27, 2023

View reviewed changes

0618 added 4 commits October 27, 2023 13:35

chore: remove env.CI

7b88772

chore: update changeset

a085162

chore: rename PACKAGE_MANAGER_EXECUTABLE

988637e

fix: tests

be3a456

0618 changed the base branch from main to poc/package-manager-support October 27, 2023 21:08

sobolk approved these changes Oct 27, 2023

View reviewed changes

0618 mentioned this pull request Oct 27, 2023

Refactor code to support various Package Managers #517

Closed

fix npm -> npx

01d8792

0618 merged commit fe038a4 into aws-amplify:poc/package-manager-support Oct 30, 2023

		const useDefault =
		process.env.npm_config_yes === 'true' \|\| process.env.CI === 'true';

feat: create amplify skip prompts for --yes option #507

feat: create amplify skip prompts for --yes option #507

Conversation

0618 commented Oct 26, 2023 • edited Loading

changeset-bot bot commented Oct 26, 2023 • 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0618 Oct 27, 2023 • 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

0618 Oct 27, 2023 • 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

sobolk left a comment

Choose a reason for hiding this comment

0618 commented Oct 27, 2023

feat: create amplify skip prompts for `--yes` option #507

feat: create amplify skip prompts for `--yes` option #507

0618 commented Oct 26, 2023 •

edited

Loading

changeset-bot bot commented Oct 26, 2023 •

edited

Loading

0618 Oct 27, 2023 •

edited

Loading

0618 Oct 27, 2023 •

edited

Loading