Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(cli): explicit defaults to yargs definition and cli arguments #32596

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
+380 −126
Open

chore(cli): explicit defaults to yargs definition and cli arguments #32596

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
1b6a563
explicit default
kaizencc Dec 19, 2024
741676a
Merge branch 'main' into conroy/explicit-default
kaizencc Dec 20, 2024
ec4358c
formatting
kaizencc Dec 20, 2024
2a6c637
fix defaults
kaizencc Dec 23, 2024
dc6f1f9
remove prints
kaizencc Dec 23, 2024
66cfea0
fix cli-args-gen defaults
kaizencc Dec 23, 2024
339abe1
refactor
kaizencc Dec 23, 2024
8aaa26d
Merge branch 'main' into conroy/explicit-default
kaizencc Dec 23, 2024
7deb217
finally the right config
kaizencc Dec 24, 2024
499f3da
special cli-argument too
kaizencc Dec 24, 2024
1c6d25a
update cli-type-gen
kaizencc Dec 24, 2024
b643089
Merge branch 'main' into conroy/explicit-default
kaizencc Dec 24, 2024
bb976ee
spacing
kaizencc Dec 24, 2024
2702326
spacing
kaizencc Dec 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 22 additions & 22 deletions packages/aws-cdk/lib/cli-arguments.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,14 +143,14 @@ export interface GlobalOptions {
/**
* Add contextual string parameter (KEY=VALUE)
*
* @default - undefined
* @default - []
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a function somewhere that defines rules for array options:

  • undefined => not provided
  • empty array => deliberately an empty list

That might be why your tests are failing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep. I changed the default to [] because providing undefined was also causing problems. elsewhere we are expecting the context field to at least exist, which it doesn't if we default to undefined. I know why this is happening, but still thinking through what the best path forward is.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this PR is not meant to change any functionality. since yargs normally defaults to undefined, even for arrays, I will update this PR to reflect that and fix the unit tests that are failing in that case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok i got to the bottom of this and essentially this bug in yargs is the problem: yargs/yargs#2443

i'm back to defaulting arrays as [], except for notification-arns which should be defaulted to undefined, but right now that leads to notification-arns being [undefined].

going forward, it should be the case that unless otherwise specified, arrays are defaulted as []

*/
readonly context?: Array<string>;

/**
* Name or path of a node package that extend the CDK features. Can be specified multiple times
*
* @default - undefined
* @default - []
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

arrays now default to []

*/
readonly plugin?: Array<string>;

Expand Down Expand Up @@ -297,7 +297,7 @@ export interface GlobalOptions {
/**
* 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 - undefined
* @default - []
*/
readonly unstable?: Array<string>;
}
Expand Down Expand Up @@ -429,7 +429,7 @@ export interface BootstrapOptions {
*
* aliases: t
*
* @default - undefined
* @default - []
*/
readonly tags?: Array<string>;

Expand All @@ -443,21 +443,21 @@ export interface BootstrapOptions {
/**
* The AWS account IDs that should be trusted to perform deployments into this environment (may be repeated, modern bootstrapping only)
*
* @default - undefined
* @default - []
*/
readonly trust?: Array<string>;

/**
* The AWS account IDs that should be trusted to look up values in this environment (may be repeated, modern bootstrapping only)
*
* @default - undefined
* @default - []
*/
readonly 'trust-for-lookup'?: Array<string>;

/**
* The Managed Policy ARNs that should be attached to the role performing deployments into this environment (may be repeated, modern bootstrapping only)
*
* @default - undefined
* @default - []
*/
readonly 'cloudformation-execution-policies'?: Array<string>;

Expand Down Expand Up @@ -515,28 +515,28 @@ export interface GcOptions {
/**
* The action (or sub-action) you want to perform. Valid entires are "print", "tag", "delete-tagged", "full".
*
* @default - full
* @default - "full"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this represents an improvement for string defaults

*/
readonly action?: string;

/**
* Specify either ecr, s3, or all
*
* @default - all
* @default - "all"
*/
readonly type?: string;

/**
* Delete assets that have been marked as isolated for this many days
*
* @default - undefined
* @default - 0
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this fixes a bug where numbers did not render the right default

*/
readonly 'rollback-buffer-days'?: number;

/**
* Never delete assets younger than this (in days)
*
* @default - undefined
* @default - 1
*/
readonly 'created-buffer-days'?: number;

Expand Down Expand Up @@ -573,7 +573,7 @@ export interface DeployOptions {
*
* aliases: E
*
* @default - undefined
* @default - []
*/
readonly 'build-exclude'?: Array<string>;

Expand All @@ -596,7 +596,7 @@ export interface DeployOptions {
/**
* ARNs of SNS topics that CloudFormation will notify with stack related events. These will be added to ARNs specified with the 'notificationArns' stack property.
*
* @default - undefined
* @default - []
*/
readonly 'notification-arns'?: Array<string>;

Expand All @@ -605,7 +605,7 @@ export interface DeployOptions {
*
* aliases: t
*
* @default - undefined
* @default - []
*/
readonly tags?: Array<string>;

Expand Down Expand Up @@ -645,7 +645,7 @@ export interface DeployOptions {
/**
* Additional parameters passed to CloudFormation at deploy time (STACK:KEY=VALUE)
*
* @default - undefined
* @default - {}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is probably a bug in config. either way i will look at it in a separate PR

*/
readonly parameters?: Array<string>;

Expand Down Expand Up @@ -717,7 +717,7 @@ export interface DeployOptions {
/**
* Maximum number of simultaneous deployments (dependency permitting) to execute.
*
* @default - undefined
* @default - 1
*/
readonly concurrency?: number;

Expand Down Expand Up @@ -782,7 +782,7 @@ export interface RollbackOptions {
/**
* Orphan the given resources, identified by their logical ID (can be specified multiple times)
*
* @default - undefined
* @default - []
*/
readonly orphan?: Array<string>;
}
Expand Down Expand Up @@ -860,7 +860,7 @@ export interface WatchOptions {
*
* aliases: E
*
* @default - undefined
* @default - []
*/
readonly 'build-exclude'?: Array<string>;

Expand Down Expand Up @@ -934,7 +934,7 @@ export interface WatchOptions {
/**
* Maximum number of simultaneous deployments (dependency permitting) to execute.
*
* @default - undefined
* @default - 1
*/
readonly concurrency?: number;
}
Expand Down Expand Up @@ -989,7 +989,7 @@ export interface DiffOptions {
/**
* Number of context lines to include in arbitrary JSON diff rendering
*
* @default - undefined
* @default - 3
*/
readonly 'context-lines'?: number;

Expand Down Expand Up @@ -1129,7 +1129,7 @@ export interface MigrateOptions {
*
* aliases: l
*
* @default - typescript
* @default - "typescript"
*/
readonly language?: string;

Expand Down Expand Up @@ -1185,7 +1185,7 @@ export interface MigrateOptions {
* tag-key: a string that matches resources with at least one tag with the provided key. i.e. "myTagKey"
* tag-value: a string that matches resources with at least one tag with the provided value. i.e. "myTagValue"
*
* @default - undefined
* @default - []
*/
readonly filter?: Array<string>;

Expand Down
Loading
Loading