Skip to content

Commit 2eff2bd

Browse files
kaizencckaizencc
authored
fix(cli): array arguments in cdk.json are ignored (#33107)
### Issue # (if applicable) Closes #32814 ### Reason for this change yargs treats arrays weirdly. setting `default: undefined` results in the default array as `['undefined']`. So instead, I set the default to be `default: []`. However, this triggers an issue with the order of combining all arguments, and the CLI default was overriding any values in `cdk.json` for array types. So instead, we must omit the `default` property entirely for arrays, in order to achieve the desired behavior of `undefined` as default. ### Description of changes Updated code generation to generate NO default property for array types ### Description of how you validated changes tests in `user-input-gen` and the diff of `parse-command-line-arguments.ts` show that unless already defined, we are omitting defaults for arrays ### 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*
1 parent 39e5578 commit 2eff2bd

File tree

9 files changed

+60
-40
lines changed

9 files changed

+60
-40
lines changed

‎packages/aws-cdk/lib/cli/parse-command-line-arguments.ts

Lines changed: 10 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -24,15 +24,13 @@ export function parseCommandLineArguments(args: Array<string>): any {
2424
desc: 'Command-line for a pre-synth build',
2525
})
2626
.option('context', {
27-
default: [],
2827
type: 'array',
2928
alias: 'c',
3029
desc: 'Add contextual string parameter (KEY=VALUE)',
3130
nargs: 1,
3231
requiresArg: true,
3332
})
3433
.option('plugin', {
35-
default: [],
3634
type: 'array',
3735
alias: 'p',
3836
desc: 'Name or path of a node package that extend the CDK features. Can be specified multiple times',
@@ -151,9 +149,9 @@ export function parseCommandLineArguments(args: Array<string>): any {
151149
desc: 'Force CI detection. If CI=true then logs will be sent to stdout instead of stderr',
152150
})
153151
.option('unstable', {
154-
default: [],
155152
type: 'array',
156153
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.',
154+
default: [],
157155
nargs: 1,
158156
requiresArg: true,
159157
})
@@ -237,10 +235,10 @@ export function parseCommandLineArguments(args: Array<string>): any {
237235
desc: 'Block public access configuration on CDK toolkit bucket (enabled by default) ',
238236
})
239237
.option('tags', {
240-
default: [],
241238
type: 'array',
242239
alias: 't',
243240
desc: 'Tags to add for the stack (KEY=VALUE)',
241+
default: [],
244242
nargs: 1,
245243
requiresArg: true,
246244
})
@@ -250,30 +248,30 @@ export function parseCommandLineArguments(args: Array<string>): any {
250248
desc: 'Whether to execute ChangeSet (--no-execute will NOT execute the ChangeSet)',
251249
})
252250
.option('trust', {
253-
default: [],
254251
type: 'array',
255252
desc: 'The AWS account IDs that should be trusted to perform deployments into this environment (may be repeated, modern bootstrapping only)',
253+
default: [],
256254
nargs: 1,
257255
requiresArg: true,
258256
})
259257
.option('trust-for-lookup', {
260-
default: [],
261258
type: 'array',
262259
desc: 'The AWS account IDs that should be trusted to look up values in this environment (may be repeated, modern bootstrapping only)',
260+
default: [],
263261
nargs: 1,
264262
requiresArg: true,
265263
})
266264
.option('untrust', {
267-
default: [],
268265
type: 'array',
269266
desc: 'The AWS account IDs that should not be trusted by this environment (may be repeated, modern bootstrapping only)',
267+
default: [],
270268
nargs: 1,
271269
requiresArg: true,
272270
})
273271
.option('cloudformation-execution-policies', {
274-
default: [],
275272
type: 'array',
276273
desc: 'The Managed Policy ARNs that should be attached to the role performing deployments into this environment (may be repeated, modern bootstrapping only)',
274+
default: [],
277275
nargs: 1,
278276
requiresArg: true,
279277
})
@@ -356,10 +354,10 @@ export function parseCommandLineArguments(args: Array<string>): any {
356354
desc: 'Deploy all available stacks',
357355
})
358356
.option('build-exclude', {
359-
default: [],
360357
type: 'array',
361358
alias: 'E',
362359
desc: 'Do not rebuild asset with the given ID. Can be specified multiple times',
360+
default: [],
363361
nargs: 1,
364362
requiresArg: true,
365363
})
@@ -382,7 +380,6 @@ export function parseCommandLineArguments(args: Array<string>): any {
382380
requiresArg: true,
383381
})
384382
.option('tags', {
385-
default: [],
386383
type: 'array',
387384
alias: 't',
388385
desc: 'Tags to add to the stack (KEY=VALUE), overrides tags from Cloud Assembly (deprecated)',
@@ -420,9 +417,9 @@ export function parseCommandLineArguments(args: Array<string>): any {
420417
desc: 'Always deploy stack even if templates are identical',
421418
})
422419
.option('parameters', {
423-
default: {},
424420
type: 'array',
425421
desc: 'Additional parameters passed to CloudFormation at deploy time (STACK:KEY=VALUE)',
422+
default: {},
426423
nargs: 1,
427424
requiresArg: true,
428425
})
@@ -524,9 +521,9 @@ export function parseCommandLineArguments(args: Array<string>): any {
524521
desc: "Whether to validate the bootstrap stack version. Defaults to 'true', disable with --no-validate-bootstrap-version.",
525522
})
526523
.option('orphan', {
527-
default: [],
528524
type: 'array',
529525
desc: 'Orphan the given resources, identified by their logical ID (can be specified multiple times)',
526+
default: [],
530527
nargs: 1,
531528
requiresArg: true,
532529
}),
@@ -578,10 +575,10 @@ export function parseCommandLineArguments(args: Array<string>): any {
578575
.command('watch [STACKS..]', "Shortcut for 'deploy --watch'", (yargs: Argv) =>
579576
yargs
580577
.option('build-exclude', {
581-
default: [],
582578
type: 'array',
583579
alias: 'E',
584580
desc: 'Do not rebuild asset with the given ID. Can be specified multiple times',
581+
default: [],
585582
nargs: 1,
586583
requiresArg: true,
587584
})
@@ -796,7 +793,6 @@ export function parseCommandLineArguments(args: Array<string>): any {
796793
desc: 'Determines if a new scan should be created, or the last successful existing scan should be used \n options are "new" or "most-recent"',
797794
})
798795
.option('filter', {
799-
default: [],
800796
type: 'array',
801797
desc: 'Filters the resource scan based on the provided criteria in the following format: "key1=value1,key2=value2"\n This field can be passed multiple times for OR style filtering: \n filtering options: \n resource-identifier: A key-value pair that identifies the target resource. i.e. {"ClusterName", "myCluster"}\n resource-type-prefix: A string that represents a type-name prefix. i.e. "AWS::DynamoDB::"\n tag-key: a string that matches resources with at least one tag with the provided key. i.e. "myTagKey"\n tag-value: a string that matches resources with at least one tag with the provided value. i.e. "myTagValue"',
802798
nargs: 1,

‎packages/aws-cdk/lib/cli/user-input.ts

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -143,14 +143,14 @@ export interface GlobalOptions {
143143
/**
144144
* Add contextual string parameter (KEY=VALUE)
145145
*
146-
* @default - []
146+
* @default - undefined
147147
*/
148148
readonly context?: Array<string>;
149149

150150
/**
151151
* Name or path of a node package that extend the CDK features. Can be specified multiple times
152152
*
153-
* @default - []
153+
* @default - undefined
154154
*/
155155
readonly plugin?: Array<string>;
156156

@@ -632,7 +632,7 @@ export interface DeployOptions {
632632
*
633633
* aliases: t
634634
*
635-
* @default - []
635+
* @default - undefined
636636
*/
637637
readonly tags?: Array<string>;
638638

@@ -1264,7 +1264,7 @@ export interface MigrateOptions {
12641264
* tag-key: a string that matches resources with at least one tag with the provided key. i.e. "myTagKey"
12651265
* tag-value: a string that matches resources with at least one tag with the provided value. i.e. "myTagValue"
12661266
*
1267-
* @default - []
1267+
* @default - undefined
12681268
*/
12691269
readonly filter?: Array<string>;
12701270

‎packages/aws-cdk/test/cli/cli-arguments.test.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -14,11 +14,11 @@ describe('yargs', () => {
1414
assetMetadata: undefined,
1515
build: undefined,
1616
caBundlePath: undefined,
17-
context: [],
17+
context: undefined,
1818
ignoreErrors: false,
1919
noColor: false,
2020
pathMetadata: undefined,
21-
plugin: [],
21+
plugin: undefined,
2222
profile: undefined,
2323
proxy: undefined,
2424
roleArn: undefined,
@@ -60,7 +60,7 @@ describe('yargs', () => {
6060
progress: undefined,
6161
requireApproval: undefined,
6262
rollback: false,
63-
tags: [],
63+
tags: undefined,
6464
toolkitStackName: undefined,
6565
watch: undefined,
6666
},

‎packages/aws-cdk/test/cli/user-config.test.ts

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ import * as os from 'os';
33
import * as fs_path from 'path';
44
import * as fs from 'fs-extra';
55
import { Configuration, PROJECT_CONFIG, PROJECT_CONTEXT } from '../../lib/cli/user-configuration';
6+
import { parseCommandLineArguments } from '../../lib/cli/parse-command-line-arguments';
67

78
// mock fs deeply
89
jest.mock('fs-extra');
@@ -112,3 +113,33 @@ test('Can specify the `quiet` key in the user config', async () => {
112113

113114
expect(config.settings.get(['quiet'])).toBe(true);
114115
});
116+
117+
test('array settings are not overridden by yarg defaults', async () => {
118+
// GIVEN
119+
const GIVEN_CONFIG: Map<string, any> = new Map([
120+
[PROJECT_CONFIG, {
121+
plugin: ['dummy'],
122+
}],
123+
]);
124+
const argsWithPlugin = await parseCommandLineArguments(['ls', '--plugin', '[]']);
125+
const argsWithoutPlugin = await parseCommandLineArguments(['ls']);
126+
127+
// WHEN
128+
mockedFs.pathExists.mockImplementation(path => {
129+
return GIVEN_CONFIG.has(path);
130+
});
131+
mockedFs.readJSON.mockImplementation(path => {
132+
return GIVEN_CONFIG.get(path);
133+
});
134+
135+
const configWithPlugin = await new Configuration({
136+
commandLineArguments: argsWithPlugin,
137+
}).load();
138+
const configWithoutPlugin = await new Configuration({
139+
commandLineArguments: argsWithoutPlugin,
140+
}).load();
141+
142+
// THEN
143+
expect(configWithPlugin.settings.get(['plugin'])).toEqual(['[]']);
144+
expect(configWithoutPlugin.settings.get(['plugin'])).toEqual(['dummy']);
145+
});

‎tools/@aws-cdk/user-input-gen/lib/user-input-gen.ts

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import { Module, SelectiveModuleImport, StructType, Type, TypeScriptRenderer } from '@cdklabs/typewriter';
22
import { EsLintRules } from '@cdklabs/typewriter/lib/eslint-rules';
33
import * as prettier from 'prettier';
4-
import { generateDefault, kebabToCamelCase, kebabToPascal } from './util';
4+
import { kebabToCamelCase, kebabToPascal } from './util';
55
import { CliConfig } from './yargs-types';
66

77
export async function renderUserInputType(config: CliConfig): Promise<string> {
@@ -46,7 +46,7 @@ export async function renderUserInputType(config: CliConfig): Promise<string> {
4646
name: kebabToCamelCase(optionName),
4747
type: convertType(option.type, option.count),
4848
docs: {
49-
default: normalizeDefault(option.default, option.type),
49+
default: normalizeDefault(option.default),
5050
summary: option.desc,
5151
deprecated: option.deprecated ? String(option.deprecated) : undefined,
5252
},
@@ -81,7 +81,7 @@ export async function renderUserInputType(config: CliConfig): Promise<string> {
8181
type: convertType(option.type, option.count),
8282
docs: {
8383
// Notification Arns is a special property where undefined and [] mean different things
84-
default: optionName === 'notification-arns' ? 'undefined' : normalizeDefault(option.default, option.type),
84+
default: optionName === 'notification-arns' ? 'undefined' : normalizeDefault(option.default),
8585
summary: option.desc,
8686
deprecated: option.deprecated ? String(option.deprecated) : undefined,
8787
remarks: option.alias ? `aliases: ${Array.isArray(option.alias) ? option.alias.join(' ') : option.alias}` : undefined,
@@ -140,7 +140,7 @@ function convertType(type: 'string' | 'array' | 'number' | 'boolean' | 'count',
140140
}
141141
}
142142

143-
function normalizeDefault(defaultValue: any, type: string): string {
143+
function normalizeDefault(defaultValue: any): string {
144144
switch (typeof defaultValue) {
145145
case 'boolean':
146146
case 'string':
@@ -153,7 +153,6 @@ function normalizeDefault(defaultValue: any, type: string): string {
153153
case 'undefined':
154154
case 'function':
155155
default:
156-
const generatedDefault = generateDefault(type);
157-
return generatedDefault ? JSON.stringify(generatedDefault) : 'undefined';
156+
return 'undefined';
158157
}
159158
}

‎tools/@aws-cdk/user-input-gen/lib/util.ts

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,5 @@
11
import { code, Expression } from '@cdklabs/typewriter';
22

3-
export function generateDefault(type: string) {
4-
return type === 'array' ? [] : undefined;
5-
}
6-
73
export function lit(value: any): Expression {
84
switch (value) {
95
case undefined:

‎tools/@aws-cdk/user-input-gen/lib/yargs-gen.ts

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import { $E, Expression, ExternalModule, FreeFunction, IScope, Module, SelectiveModuleImport, Statement, ThingSymbol, Type, TypeScriptRenderer, code, expr } from '@cdklabs/typewriter';
22
import { EsLintRules } from '@cdklabs/typewriter/lib/eslint-rules';
33
import * as prettier from 'prettier';
4-
import { generateDefault, lit } from './util';
4+
import { lit } from './util';
55
import { CliConfig, CliOption, YargsOption } from './yargs-types';
66

77
// to import lodash.clonedeep properly, we would need to set esModuleInterop: true
@@ -114,10 +114,10 @@ function makeOptions(prefix: Expression, options: { [optionName: string]: CliOpt
114114
let optionsExpr = prefix;
115115
for (const option of Object.keys(options)) {
116116
const theOption: CliOption = {
117-
// Make the default explicit (overridden if the option includes an actual default)
118-
// 'notification-arns' is a special snowflake that should be defaulted to 'undefined', but https://github.com/yargs/yargs/issues/2443
119-
// prevents us from doing so. This should be changed if the issue is resolved.
120-
...(option === 'notification-arns' ? {} : { default: generateDefault(options[option].type) }),
117+
// https://github.com/yargs/yargs/issues/2443 prevents us from supplying 'undefined' as the default
118+
// for array types, because this turns into ['undefined']. The only way to achieve yargs' default is
119+
// to provide no default.
120+
...(options[option].type == 'array' ? {} : { default: undefined }),
121121
...options[option],
122122
};
123123
const optionProps: YargsOption = cloneDeep(theOption);

‎tools/@aws-cdk/user-input-gen/test/user-input-gen.test.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -110,7 +110,7 @@ describe('render', () => {
110110
/**
111111
* plugins to load
112112
*
113-
* @default - []
113+
* @default - undefined
114114
*/
115115
readonly plugin?: Array<string>;
116116
}
@@ -205,7 +205,7 @@ describe('render', () => {
205205
/**
206206
* Other array
207207
*
208-
* @default - []
208+
* @default - undefined
209209
*/
210210
readonly otherArray?: Array<string>;
211211
}

‎tools/@aws-cdk/user-input-gen/test/yargs-gen.test.ts

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,6 @@ describe('render', () => {
5050
desc: 'text for two',
5151
})
5252
.option('three', {
53-
default: [],
5453
type: 'array',
5554
alias: 't',
5655
desc: 'text for three',
@@ -198,7 +197,6 @@ describe('render', () => {
198197
requiresArg: true,
199198
})
200199
.option('other-array', {
201-
default: [],
202200
type: 'array',
203201
desc: 'Other array',
204202
nargs: 1,

0 commit comments

Comments
 (0)