Skip to content

Commit

Permalink
chore(cli): simplify config with helper functions (#32547)
Browse files Browse the repository at this point in the history
### Reason for this change

Simplify the codegen for CLI config. Previously the generated function expected a number of arguments at call time.
Instead we now require a single helpers object at build time. This holds typewriter references to any function called during config parsing.

Additionally, some the helper functions for init and migrate are now just called at build time. This is fine because these values don't change dynamically unless a code change is made to the CLI.

### Description of changes

- Moved all helper functions into a single file
- Created a typewriter proxy class for that file
- Use the module proxy instead passing arguments at runtime
- This allows us to get rid of the `DynamicValue` stuff all-together and instead we can just check if a value is a typewriter expression.

### Description of how you validated changes

Existing tests cover this. Run additional manual verification as well.

### Checklist
- [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
mrgrain authored Dec 17, 2024
1 parent 38116b0 commit 506d210
Show file tree
Hide file tree
Showing 8 changed files with 173 additions and 139 deletions.
16 changes: 3 additions & 13 deletions packages/aws-cdk/lib/cli.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,13 @@ import { CdkToolkit, AssetBuildTime } from '../lib/cdk-toolkit';
import { realHandler as context } from '../lib/commands/context';
import { realHandler as docs } from '../lib/commands/docs';
import { realHandler as doctor } from '../lib/commands/doctor';
import { MIGRATE_SUPPORTED_LANGUAGES, getMigrateScanType } from '../lib/commands/migrate';
import { availableInitLanguages, cliInit, printAvailableTemplates } from '../lib/init';
import { getMigrateScanType } from '../lib/commands/migrate';
import { cliInit, printAvailableTemplates } from '../lib/init';
import { data, debug, error, print, setCI, setLogLevel, LogLevel } from '../lib/logging';
import { Notices } from '../lib/notices';
import { Command, Configuration, Settings } from '../lib/settings';
import * as version from '../lib/version';
import { SdkToCliLogger } from './api/aws-auth/sdk-logger';
import { yargsNegativeAlias } from './util/yargs-helpers';

/* eslint-disable max-len */
/* eslint-disable @typescript-eslint/no-shadow */ // yargs
Expand All @@ -38,17 +37,8 @@ if (!process.stdout.isTTY) {
}

export async function exec(args: string[], synthesizer?: Synthesizer): Promise<number | void> {
function makeBrowserDefault(): string {
const defaultBrowserCommand: { [key in NodeJS.Platform]?: string } = {
darwin: 'open %u',
win32: 'start %u',
};

const cmd = defaultBrowserCommand[process.platform];
return cmd ?? 'xdg-open %u';
}

const argv = await parseCommandLineArguments(args, makeBrowserDefault(), await availableInitLanguages(), MIGRATE_SUPPORTED_LANGUAGES as string[], version.DISPLAY_VERSION, yargsNegativeAlias);
const argv = await parseCommandLineArguments(args);

// if one -v, log at a DEBUG level
// if 2 -v, log at a TRACE level
Expand Down
88 changes: 27 additions & 61 deletions packages/aws-cdk/lib/config.ts
Original file line number Diff line number Diff line change
@@ -1,14 +1,17 @@
import type { CliConfig, DynamicResult } from '@aws-cdk/yargs-gen';
// eslint-disable-next-line import/no-extraneous-dependencies
import { CliHelpers, type CliConfig } from '@aws-cdk/yargs-gen';
import { StackActivityProgress } from './api/util/cloudformation/stack-activity-monitor';
import { MIGRATE_SUPPORTED_LANGUAGES } from './commands/migrate';
import { RequireApproval } from './diff';
import { availableInitLanguages } from './init';

/* eslint-disable quote-props */
export const YARGS_HELPERS = new CliHelpers('./util/yargs-helpers');

/**
* Source of truth for all CDK CLI commands. `yargs-gen` translates this into the `yargs` definition
* in `lib/parse-command-line-arguments.ts`.
*/
export function makeConfig(): CliConfig {
export async function makeConfig(): Promise<CliConfig> {
return {
globalOptions: {
'app': { type: 'string', alias: 'a', desc: 'REQUIRED WHEN RUNNING APP: command-line for executing your app or a cloud assembly directory (e.g. "node bin/my-app.js"). Can also be specified in cdk.json or ~/.cdk.json', requiresArg: true },
Expand All @@ -34,11 +37,11 @@ export function makeConfig(): CliConfig {
'output': { type: 'string', alias: 'o', desc: 'Emits the synthesized cloud assembly into a directory (default: cdk.out)', requiresArg: true },
'notices': { type: 'boolean', desc: 'Show relevant notices' },
'no-color': { type: 'boolean', desc: 'Removes colors and other style from console output', default: false },
'ci': { type: 'boolean', desc: 'Force CI detection. If CI=true then logs will be sent to stdout instead of stderr', default: DynamicValue.fromInline(() => process.env.CI !== undefined) },
'ci': { type: 'boolean', desc: 'Force CI detection. If CI=true then logs will be sent to stdout instead of stderr', default: YARGS_HELPERS.isCI() },
'unstable': { type: 'array', desc: 'Opt in to unstable features. The flag indicates that the scope and API of a feature might still change. Otherwise the feature is generally production ready and fully supported. Can be specified multiple times.', default: [] },
},
commands: {
'list': {
list: {
arg: {
name: 'STACKS',
variadic: true,
Expand All @@ -50,17 +53,17 @@ export function makeConfig(): CliConfig {
'show-dependencies': { type: 'boolean', default: false, alias: 'd', desc: 'Display stack dependency information for each stack' },
},
},
'synthesize': {
synthesize: {
arg: {
name: 'STACKS',
variadic: true,
},
aliases: ['synth'],
description: 'Synthesizes and prints the CloudFormation template for this stack',
options: {
'exclusively': { type: 'boolean', alias: 'e', desc: 'Only synthesize requested stacks, don\'t include dependencies' },
'validation': { type: 'boolean', desc: 'After synthesis, validate stacks with the "validateOnSynth" attribute set (can also be controlled with CDK_VALIDATION)', default: true },
'quiet': { type: 'boolean', alias: 'q', desc: 'Do not output CloudFormation Template to stdout', default: false },
exclusively: { type: 'boolean', alias: 'e', desc: 'Only synthesize requested stacks, don\'t include dependencies' },
validation: { type: 'boolean', desc: 'After synthesis, validate stacks with the "validateOnSynth" attribute set (can also be controlled with CDK_VALIDATION)', default: true },
quiet: { type: 'boolean', alias: 'q', desc: 'Do not output CloudFormation Template to stdout', default: false },
},
},
bootstrap: {
Expand Down Expand Up @@ -242,18 +245,6 @@ export function makeConfig(): CliConfig {
variadic: true,
},
options: {
// I'm fairly certain none of these options, present for 'deploy', make sense for 'watch':
// .option('all', { type: 'boolean', default: false, desc: 'Deploy all available stacks' })
// .option('ci', { type: 'boolean', desc: 'Force CI detection', default: process.env.CI !== undefined })
// @deprecated(v2) -- tags are part of the Cloud Assembly and tags specified here will be overwritten on the next deployment
// .option('tags', { type: 'array', alias: 't', desc: 'Tags to add to the stack (KEY=VALUE), overrides tags from Cloud Assembly (deprecated)', nargs: 1, requiresArg: true })
// .option('execute', { type: 'boolean', desc: 'Whether to execute ChangeSet (--no-execute will NOT execute the ChangeSet)', default: true })
// These options, however, are more subtle - I could be convinced some of these should also be available for 'watch':
// .option('require-approval', { type: 'string', choices: [RequireApproval.Never, RequireApproval.AnyChange, RequireApproval.Broadening], desc: 'What security-sensitive changes need manual approval' })
// .option('parameters', { type: 'array', desc: 'Additional parameters passed to CloudFormation at deploy time (STACK:KEY=VALUE)', nargs: 1, requiresArg: true, default: {} })
// .option('previous-parameters', { type: 'boolean', default: true, desc: 'Use previous values for existing parameters (you must specify all parameters on every deployment if this is disabled)' })
// .option('outputs-file', { type: 'string', alias: 'O', desc: 'Path to file where stack outputs will be written as JSON', requiresArg: true })
// .option('notification-arns', { type: 'array', desc: 'ARNs of SNS topics that CloudFormation will notify with stack related events', nargs: 1, requiresArg: true })
'build-exclude': { type: 'array', alias: 'E', desc: 'Do not rebuild asset with the given ID. Can be specified multiple times', default: [] },
'exclusively': { type: 'boolean', alias: 'e', desc: 'Only deploy requested stacks, don\'t include dependencies' },
'change-set-name': { type: 'string', desc: 'Name of the CloudFormation change set to create' },
Expand Down Expand Up @@ -295,9 +286,9 @@ export function makeConfig(): CliConfig {
variadic: true,
},
options: {
'all': { type: 'boolean', default: false, desc: 'Destroy all available stacks' },
'exclusively': { type: 'boolean', alias: 'e', desc: 'Only destroy requested stacks, don\'t include dependees' },
'force': { type: 'boolean', alias: 'f', desc: 'Do not ask for confirmation before destroying the stacks' },
all: { type: 'boolean', default: false, desc: 'Destroy all available stacks' },
exclusively: { type: 'boolean', alias: 'e', desc: 'Only destroy requested stacks, don\'t include dependees' },
force: { type: 'boolean', alias: 'f', desc: 'Do not ask for confirmation before destroying the stacks' },
},
},
diff: {
Expand Down Expand Up @@ -336,7 +327,7 @@ export function makeConfig(): CliConfig {
notices: {
description: 'Returns a list of relevant notices',
options: {
'unacknowledged': { type: 'boolean', alias: 'u', default: false, desc: 'Returns a list of unacknowledged notices' },
unacknowledged: { type: 'boolean', alias: 'u', default: false, desc: 'Returns a list of unacknowledged notices' },
},
},
init: {
Expand All @@ -346,16 +337,16 @@ export function makeConfig(): CliConfig {
variadic: false,
},
options: {
'language': { type: 'string', alias: 'l', desc: 'The language to be used for the new project (default can be configured in ~/.cdk.json)', choices: DynamicValue.fromParameter('availableInitLanguages') } as any, // TODO: preamble, this initTemplateLanguages variable needs to go as a statement there.
'language': { type: 'string', alias: 'l', desc: 'The language to be used for the new project (default can be configured in ~/.cdk.json)', choices: await availableInitLanguages() },
'list': { type: 'boolean', desc: 'List the available templates' },
'generate-only': { type: 'boolean', default: false, desc: 'If true, only generates project files, without executing additional operations such as setting up a git repo, installing dependencies or compiling the project' },
},
},
'migrate': {
migrate: {
description: false as any,
options: {
'stack-name': { type: 'string', alias: 'n', desc: 'The name assigned to the stack created in the new project. The name of the app will be based off this name as well.', requiresArg: true },
'language': { type: 'string', default: 'typescript', alias: 'l', desc: 'The language to be used for the new project', choices: DynamicValue.fromParameter('migrateSupportedLanguages') as any },
'language': { type: 'string', default: 'typescript', alias: 'l', desc: 'The language to be used for the new project', choices: MIGRATE_SUPPORTED_LANGUAGES },
'account': { type: 'string', desc: 'The account to retrieve the CloudFormation stack template from' },
'region': { type: 'string', desc: 'The region to retrieve the CloudFormation stack template from' },
'from-path': { type: 'string', desc: 'The path to the CloudFormation template to migrate. Use this for locally stored templates' },
Expand All @@ -379,54 +370,29 @@ export function makeConfig(): CliConfig {
'compress': { type: 'boolean', desc: 'Use this flag to zip the generated CDK app' },
},
},
'context': {
context: {
description: 'Manage cached context values',
options: {
'reset': { alias: 'e', desc: 'The context key (or its index) to reset', type: 'string', requiresArg: true },
'force': { alias: 'f', desc: 'Ignore missing key error', type: 'boolean', default: false },
'clear': { desc: 'Clear all context', type: 'boolean' },
reset: { alias: 'e', desc: 'The context key (or its index) to reset', type: 'string', requiresArg: true },
force: { alias: 'f', desc: 'Ignore missing key error', type: 'boolean', default: false },
clear: { desc: 'Clear all context', type: 'boolean' },
},
},
'docs': {
docs: {
aliases: ['doc'],
description: 'Opens the reference documentation in a browser',
options: {
'browser': {
browser: {
alias: 'b',
desc: 'the command to use to open the browser, using %u as a placeholder for the path of the file to open',
type: 'string',
default: DynamicValue.fromParameter('browserDefault'),
default: YARGS_HELPERS.browserForPlatform(),
},
},
},
'doctor': {
doctor: {
description: 'Check your set-up for potential problems',
},
},
};
}

/**
* Informs the code library, `@aws-cdk/yargs-gen`, that
* this value references an entity not defined in this configuration file.
*/
export class DynamicValue {
/**
* Instructs `yargs-gen` to retrieve this value from the parameter with passed name.
*/
public static fromParameter(parameterName: string): DynamicResult {
return {
dynamicType: 'parameter',
dynamicValue: parameterName,
};
}

public static fromInline(f: () => any): DynamicResult {
const ARROW = '=>';
const body = f.toString();
return {
dynamicType: 'function',
dynamicValue: body.substring(body.indexOf(ARROW) + ARROW.length).trim(),
};
}
}
24 changes: 9 additions & 15 deletions packages/aws-cdk/lib/parse-command-line-arguments.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,16 +4,10 @@
// -------------------------------------------------------------------------------------------
/* eslint-disable @typescript-eslint/comma-dangle, comma-spacing, max-len, quotes, quote-props */
import { Argv } from 'yargs';
import * as helpers from './util/yargs-helpers';

// @ts-ignore TS6133
export function parseCommandLineArguments(
args: Array<string>,
browserDefault: string,
availableInitLanguages: Array<string>,
migrateSupportedLanguages: Array<string>,
version: string,
yargsNegativeAlias: any
): any {
export function parseCommandLineArguments(args: Array<string>): any {
return yargs
.env('CDK')
.usage('Usage: cdk -a <cdk-app> COMMAND')
Expand Down Expand Up @@ -143,7 +137,7 @@ export function parseCommandLineArguments(
.option('ci', {
type: 'boolean',
desc: 'Force CI detection. If CI=true then logs will be sent to stdout instead of stderr',
default: process.env.CI !== undefined,
default: helpers.isCI(),
})
.option('unstable', {
type: 'array',
Expand Down Expand Up @@ -424,8 +418,8 @@ export function parseCommandLineArguments(
type: 'boolean',
desc: "Rollback stack to stable state on failure. Defaults to 'true', iterate more rapidly with --no-rollback or -R. Note: do **not** disable this flag for deployments with resource replacements, as that will always fail",
})
.middleware(yargsNegativeAlias('R', 'rollback'), true)
.option('R', { type: 'boolean', hidden: true })
.middleware(helpers.yargsNegativeAlias('R', 'rollback'), true)
.option('hotswap', {
type: 'boolean',
desc: "Attempts to perform a 'hotswap' deployment, but does not fall back to a full deployment if that is not possible. Instead, changes to any non-hotswappable properties are ignored.Do not use this in production environments",
Expand Down Expand Up @@ -570,8 +564,8 @@ export function parseCommandLineArguments(
type: 'boolean',
desc: "Rollback stack to stable state on failure. Defaults to 'true', iterate more rapidly with --no-rollback or -R. Note: do **not** disable this flag for deployments with resource replacements, as that will always fail",
})
.middleware(yargsNegativeAlias('R', 'rollback'), true)
.option('R', { type: 'boolean', hidden: true })
.middleware(helpers.yargsNegativeAlias('R', 'rollback'), true)
.option('hotswap', {
type: 'boolean',
desc: "Attempts to perform a 'hotswap' deployment, but does not fall back to a full deployment if that is not possible. Instead, changes to any non-hotswappable properties are ignored.'true' by default, use --no-hotswap to turn off",
Expand Down Expand Up @@ -679,7 +673,7 @@ export function parseCommandLineArguments(
type: 'string',
alias: 'l',
desc: 'The language to be used for the new project (default can be configured in ~/.cdk.json)',
choices: availableInitLanguages,
choices: ['csharp', 'fsharp', 'go', 'java', 'javascript', 'python', 'typescript'],
})
.option('list', {
type: 'boolean',
Expand All @@ -704,7 +698,7 @@ export function parseCommandLineArguments(
default: 'typescript',
alias: 'l',
desc: 'The language to be used for the new project',
choices: migrateSupportedLanguages,
choices: ['typescript', 'go', 'java', 'python', 'csharp'],
})
.option('account', {
type: 'string',
Expand Down Expand Up @@ -765,11 +759,11 @@ export function parseCommandLineArguments(
alias: 'b',
desc: 'the command to use to open the browser, using %u as a placeholder for the path of the file to open',
type: 'string',
default: browserDefault,
default: helpers.browserForPlatform(),
})
)
.command('doctor', 'Check your set-up for potential problems')
.version(version)
.version(helpers.cliVersion())
.demandCommand(1, '')
.recommendCommands()
.help()
Expand Down
36 changes: 35 additions & 1 deletion packages/aws-cdk/lib/util/yargs-helpers.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
import * as version from '../../lib/version';

/**
* yargs middleware to negate an option if a negative alias is provided
* E.g. `-R` will imply `--rollback=false`
*
* @param optionToNegate The name of the option to negate, e.g. `rollback`
* @param negativeAlias The alias that should negate the option, e.g. `R`
* @returns
* @returns a middleware function that can be passed to yargs
*/
export function yargsNegativeAlias<T extends { [x in S | L]: boolean | undefined }, S extends string, L extends string>(
negativeAlias: S,
Expand All @@ -19,3 +21,35 @@ export function yargsNegativeAlias<T extends { [x in S | L]: boolean | undefined
return argv;
};
}

/**
* Returns true if the current process is running in a CI environment
* @returns true if the current process is running in a CI environment
*/
export function isCI(): boolean {
return process.env.CI !== undefined;
}

/**
* Returns the current version of the CLI
* @returns the current version of the CLI
*/
export function cliVersion(): string {
return version.DISPLAY_VERSION;
}

/**
* Returns the default browser command for the current platform
* @returns the default browser command for the current platform
*/
export function browserForPlatform(): string {
switch (process.platform) {
case 'darwin':
return 'open %u';
case 'win32':
return 'start %u';
default:
return 'xdg-open %u';
}
}

4 changes: 2 additions & 2 deletions packages/aws-cdk/scripts/yargs-gen.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
import * as fs from 'fs';
// eslint-disable-next-line import/no-extraneous-dependencies
import { renderYargs } from '@aws-cdk/yargs-gen';
import { makeConfig } from '../lib/config';
import { makeConfig, YARGS_HELPERS } from '../lib/config';

async function main() {
fs.writeFileSync('./lib/parse-command-line-arguments.ts', await renderYargs(makeConfig()));
fs.writeFileSync('./lib/parse-command-line-arguments.ts', await renderYargs(await makeConfig(), YARGS_HELPERS));
}

main().then(() => {
Expand Down
Loading

0 comments on commit 506d210

Please sign in to comment.