chore(cli): refactor cli.ts to use `UserInput` #32760

kaizencc · 2025-01-06T22:19:38Z

This branch, conroy/refact is meant to be the branch that integrates the UserInput type into cli.ts. At a high level, we need UserInput to replace argv (or, the CLI input object) and configuration.settings (a combo of the CLI input and cdk.json). UserInput will be a one-stop shop for all user configurations, and will make it easier to manage cli.ts code. Currently, we use argv and configuration.settings interchangeably, with no type-safety, and there are many places in code where we use them inconsistently/incorrectly.

The following list represents work that needs to be done for this to be merged to main:

refactor configuration.settings to be of type UserInput (the underlying object must be a UserInput
bundlingStacks logic needs to be pulled out of settings -- its not user input
context and tags must be handled in user-input-gen. These properties are represented as a string[] in config.ts, since that's what yargs allows, but really need to be {[key: string]: string}.
bootstrap options kmsKeyId and bootstrapBucket must be handled ad-hoc because the cdk.json configuration differs from the CLI option
hotswap options must be handled ad-hoc because the cdk.json configuration differs from the CLI option
import.progress does not exist as a CLI option
watch.include and watch.exclude does not exist as a CLI option: chore(cli): watch is fully configurable from CLI #32903

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

…types

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

…onroy/refact

packages/aws-cdk/lib/cdk-toolkit.ts

mrgrain · 2025-01-09T12:31:21Z

packages/aws-cdk/lib/cdk-toolkit.ts

@@ -1792,14 +1795,14 @@ export interface GarbageCollectionOptions {
   *
   * @default 'full'
   */
-  readonly action: 'print' | 'tag' | 'delete-tagged' | 'full';
+  readonly action: ActionType;


Suggested change

readonly action: ActionType;

readonly action: GarbageCollectionAction;

im reverting these changes. tying them as any as they were before. can revisit in separate PR if we want to handle all cases where we turn a string into an enum in a better way

mrgrain · 2025-01-09T12:32:56Z

packages/aws-cdk/lib/api/cxapp/exec.ts

+  } else { // Skip bundling for all stacks
+    bundlingStacks = [];
+  }


The else is unnecessary if you assign bundlingStacks to the empty array earlier: let bundlingStacks = new Array<string>();

mrgrain · 2025-01-09T12:36:42Z

packages/aws-cdk/lib/api/cxapp/exec.ts

  context[cxapi.BUNDLING_STACKS] = bundlingStacks;

  debug('context:', context);

  return context;
 }

+function setBundlingStacks(config: Configuration) {


This works but is hard to parse for a human. In general I prefer this style:

// explain how this is even possible if (!config.command) { return []; } const stackCommands = [...]; // command doesn't operate on a stack selection if (!stackCommands.includes(config.command) { return []; } // If we deploy, diff, synth or watch a list of stacks exclusively we skip // bundling for all other stacks. if (config.settings.get(['exclusively']) && config.settings.get(['STACKS'])) { return config.settings.get(['STACKS']); } return ['**'];

agreed. i had just copied over the code from elsewhere. will update

mrgrain · 2025-01-09T12:38:15Z

packages/aws-cdk/lib/cli.ts

+  const configuration = new Configuration({
+    commandLineArguments: argv,
+  });
+  await configuration.load();
+  const settings = configuration.settings.all;


Side note: With that we still have the weird duplicity of having Configuration and Settings

we're passing Configuration to too many places right now for it to make sense for me to change this in this PR. Agree that that's the correct end state.

mrgrain · 2025-01-09T12:40:48Z

packages/aws-cdk/lib/cli.ts

+        const diffOptions = settings.diff ?? {};
        return cli.diff({
-          stackNames: args.STACKS,
-          exclusively: args.exclusively,
-          templatePath: args.template,
-          strict: args.strict,
-          contextLines: args.contextLines,
-          securityOnly: args.securityOnly,
-          fail: args.fail != null ? args.fail : !enableDiffNoFail,
-          stream: args.ci ? process.stdout : undefined,
-          compareAgainstProcessedTemplate: args.processed,
-          quiet: args.quiet,
-          changeSet: args['change-set'],
+          stackNames: diffOptions.STACKS ?? [],
+          exclusively: diffOptions.exclusively,
+          templatePath: diffOptions.template,
+          strict: diffOptions.strict,
+          contextLines: diffOptions.contextLines,
+          securityOnly: diffOptions.securityOnly,
+          fail: diffOptions.fail != null ? diffOptions.fail : !enableDiffNoFail,
+          stream: globalOptions.ci ? process.stdout : undefined,
+          compareAgainstProcessedTemplate: diffOptions.processed,
+          quiet: diffOptions.quiet,
+          changeSet: diffOptions.changeSet,
          toolkitStackName: toolkitStackName,


This is good already, but ideally the next step would be a strongly typed helper function/method so we can do:

return cli.diff(diffSettingsToDiffOptions(settings.diff));

…onroy/refact

aws-cdk-automation · 2025-01-14T16:19:04Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 0bd551d
Result: FAILED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2025-01-14T19:34:48Z

The pull request linter fails with the following errors:

❌ CLI code has changed. A maintainer must run the code through the testing pipeline (git fetch origin pull/32760/head && git push -f origin FETCH_HEAD:test-main-pipeline), then add the 'pr-linter/cli-integ-tested' label when the pipeline succeeds.

PRs must pass status checks before we can provide a meaningful review.

If you would like to request an exemption from the status checks or clarification on feedback, please leave a comment on this PR containing Exemption Request and/or Clarification Request.

✅ A exemption request has been requested. Please wait for a maintainer's review.

aws-cdk-automation · 2025-01-14T19:42:13Z

The pull request linter fails with the following errors:

❌ CLI code has changed. A maintainer must run the code through the testing pipeline (git fetch origin pull/32760/head && git push -f origin FETCH_HEAD:test-main-pipeline), then add the 'pr-linter/cli-integ-tested' label when the pipeline succeeds.

PRs must pass status checks before we can provide a meaningful review.

If you would like to request an exemption from the status checks or clarification on feedback, please leave a comment on this PR containing Exemption Request and/or Clarification Request.

✅ A exemption request has been requested. Please wait for a maintainer's review.

kaizencc added 2 commits January 6, 2025 15:28

chore(cli): cli arguments with "count: true" are generated as number …

19d2e41

…types

chore(cli): refactor cli.ts to use CliArguments

2cc8fba

aws-cdk-automation requested a review from a team January 6, 2025 22:19

github-actions bot added the p2 label Jan 6, 2025

mergify bot added the contribution/core This is a PR that came from AWS. label Jan 6, 2025

kaizencc requested a deployment to test-pipeline January 6, 2025 22:20 — with GitHub Actions Waiting

aws-cdk-automation requested changes Jan 6, 2025

View reviewed changes

aws-cdk-automation added the pr/needs-cli-test-run This PR needs CLI tests run against it. label Jan 6, 2025

kaizencc and others added 2 commits January 7, 2025 14:46

builds with test failures

4bc293d

Merge branch 'main' into conroy/refact

76281b7

kaizencc requested a deployment to test-pipeline January 7, 2025 19:47 — with GitHub Actions Waiting

kaizencc added 3 commits January 7, 2025 15:04

still builds

b4757bf

more building

957f079

Merge branch 'conroy/refact' of https://github.com/aws/aws-cdk into c…

d3e9d97

…onroy/refact

kaizencc requested a deployment to test-pipeline January 8, 2025 23:21 — with GitHub Actions Waiting

use settinsg

2ea4181

kaizencc requested a deployment to test-pipeline January 9, 2025 00:03 — with GitHub Actions Waiting

small chagne

a5e6e8c

kaizencc requested a deployment to test-pipeline January 9, 2025 00:50 — with GitHub Actions Waiting

kaizencc added 2 commits January 8, 2025 23:06

oneline

ed19731

updates

8564109

kaizencc requested a deployment to test-pipeline January 9, 2025 04:08 — with GitHub Actions Waiting

mrgrain requested changes Jan 9, 2025

View reviewed changes

finish merge

145850f

kaizencc requested a deployment to test-pipeline January 10, 2025 20:01 — with GitHub Actions Waiting

kaizencc added 2 commits January 10, 2025 15:04

delete extra file

64e4404

builds now

3da904c

kaizencc requested a deployment to test-pipeline January 10, 2025 20:15 — with GitHub Actions Waiting

fixes the build

ecef343

kaizencc requested a deployment to test-pipeline January 10, 2025 20:46 — with GitHub Actions Waiting

still builds, better refacotrs some places

ad76f1f

kaizencc requested a deployment to test-pipeline January 13, 2025 20:39 — with GitHub Actions Waiting

more updates

03fe7d6

kaizencc mentioned this pull request Jan 13, 2025

chore(cli): watch is fully configurable from CLI #32903

Open

1 task

kaizencc and others added 2 commits January 14, 2025 10:23

Merge branch 'main' into conroy/refact

f247927

Merge branch 'conroy/refact' of https://github.com/aws/aws-cdk into c…

0bd551d

…onroy/refact

kaizencc requested a deployment to test-pipeline January 14, 2025 15:24 — with GitHub Actions Waiting

kaizencc requested a deployment to test-pipeline January 14, 2025 16:17 — with GitHub Actions Waiting

kaizencc changed the title ~~chore(cli): refactor cli.ts to use CliArguments~~ chore(cli): refactor cli.ts to use UserInput Jan 14, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(cli): refactor cli.ts to use `UserInput` #32760

chore(cli): refactor cli.ts to use `UserInput` #32760

kaizencc commented Jan 6, 2025 •

edited

Loading

aws-cdk-automation left a comment

mrgrain Jan 9, 2025

kaizencc Jan 10, 2025

mrgrain Jan 9, 2025

mrgrain Jan 9, 2025

kaizencc Jan 10, 2025

mrgrain Jan 9, 2025

kaizencc Jan 10, 2025

mrgrain Jan 9, 2025

aws-cdk-automation commented Jan 14, 2025

aws-cdk-automation commented Jan 14, 2025

aws-cdk-automation commented Jan 14, 2025

	readonly action: ActionType;
	readonly action: GarbageCollectionAction;

chore(cli): refactor cli.ts to use UserInput #32760

Are you sure you want to change the base?

chore(cli): refactor cli.ts to use UserInput #32760

Conversation

kaizencc commented Jan 6, 2025 • edited Loading

aws-cdk-automation 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aws-cdk-automation commented Jan 14, 2025

AWS CodeBuild CI Report

aws-cdk-automation commented Jan 14, 2025

aws-cdk-automation commented Jan 14, 2025

chore(cli): refactor cli.ts to use `UserInput` #32760

chore(cli): refactor cli.ts to use `UserInput` #32760

kaizencc commented Jan 6, 2025 •

edited

Loading