Skip to content

Commit

Permalink
fix(cli): cdk deploy -R does not disable rollback (#32514)
Browse files Browse the repository at this point in the history
### Issue 

`cdk deploy -R` should be the same as `cdk deploy --no-rollback` or `cdk deploy --rollback=false`, but has no effect

### Reason for this change

PR #31850 introduced this bug by accidentally flipping the order of arguments passed to the `yargsNegativeAlias` helper. 
This caused the helper to have no effect.

### Description of changes

- Changed the codegen to fully generate negative aliases for the user
- Fixed the generate code to use the correct argument order again for `yargsNegativeAlias`
- Renamed the parameters to make it easier to understand.
- Made `nargs: 1` and `requiresArg: true` implied for all array options. Some options did miss one or both of these. This was an oversight.


### Description of how you validated changes

Added an explicit test case for `-R`.

### 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 13, 2024
1 parent f937d30 commit 2e75924
Show file tree
Hide file tree
Showing 10 changed files with 167 additions and 94 deletions.
3 changes: 2 additions & 1 deletion aws-cdk.code-workspace
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,8 @@
{
"name": "aws-custom-resource-sdk-adapter",
"rootPath": "packages/@aws-cdk/aws-custom-resource-sdk-adapter"
}
},
{ "name": "yargs-gen", "rootPath": "tools/@aws-cdk/yargs-gen" }
]
},
"extensions": {
Expand Down
2 changes: 1 addition & 1 deletion packages/aws-cdk/bin/cdk
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
#!/usr/bin/env node
// source maps must be enabled before importing files
process.setSourceMapsEnabled(true);
const { cli } = require("../lib/cli");
const { cli } = require("../lib");

cli();
13 changes: 1 addition & 12 deletions packages/aws-cdk/lib/cli.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ 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 Down Expand Up @@ -527,18 +528,6 @@ function arrayFromYargs(xs: string[]): string[] | undefined {
return xs.filter((x) => x !== '');
}

function yargsNegativeAlias<T extends { [x in S | L]: boolean | undefined }, S extends string, L extends string>(
shortName: S,
longName: L,
): (argv: T) => T {
return (argv: T) => {
if (shortName in argv && argv[shortName]) {
(argv as any)[longName] = false;
}
return argv;
};
}

function determineHotswapMode(hotswap?: boolean, hotswapFallback?: boolean, watch?: boolean): HotswapMode {
if (hotswap && hotswapFallback) {
throw new Error('Can not supply both --hotswap and --hotswap-fallback at the same time');
Expand Down
38 changes: 13 additions & 25 deletions packages/aws-cdk/lib/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@ export function makeConfig(): CliConfig {
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 },
'build': { type: 'string', desc: 'Command-line for a pre-synth build' },
'context': { type: 'array', alias: 'c', desc: 'Add contextual string parameter (KEY=VALUE)', nargs: 1, requiresArg: true },
'plugin': { type: 'array', alias: 'p', desc: 'Name or path of a node package that extend the CDK features. Can be specified multiple times', nargs: 1 },
'context': { type: 'array', alias: 'c', desc: 'Add contextual string parameter (KEY=VALUE)' },
'plugin': { type: 'array', alias: 'p', desc: 'Name or path of a node package that extend the CDK features. Can be specified multiple times' },
'trace': { type: 'boolean', desc: 'Print trace for stack warnings' },
'strict': { type: 'boolean', desc: 'Do not construct stacks with warnings' },
'lookups': { type: 'boolean', desc: 'Perform context lookups (synthesis fails if this is disabled and context lookups need to be performed)', default: true },
Expand Down Expand Up @@ -77,11 +77,11 @@ export function makeConfig(): CliConfig {
'bootstrap-customer-key': { type: 'boolean', desc: 'Create a Customer Master Key (CMK) for the bootstrap bucket (you will be charged but can customize permissions, modern bootstrapping only)', default: undefined, conflicts: 'bootstrap-kms-key-id' },
'qualifier': { type: 'string', desc: 'String which must be unique for each bootstrap stack. You must configure it on your CDK app if you change this from the default.', default: undefined },
'public-access-block-configuration': { type: 'boolean', desc: 'Block public access configuration on CDK toolkit bucket (enabled by default) ', default: undefined },
'tags': { type: 'array', alias: 't', desc: 'Tags to add for the stack (KEY=VALUE)', nargs: 1, requiresArg: true, default: [] },
'tags': { type: 'array', alias: 't', desc: 'Tags to add for the stack (KEY=VALUE)', default: [] },
'execute': { type: 'boolean', desc: 'Whether to execute ChangeSet (--no-execute will NOT execute the ChangeSet)', default: true },
'trust': { type: 'array', desc: 'The AWS account IDs that should be trusted to perform deployments into this environment (may be repeated, modern bootstrapping only)', default: [], nargs: 1, requiresArg: true },
'trust-for-lookup': { type: 'array', desc: 'The AWS account IDs that should be trusted to look up values in this environment (may be repeated, modern bootstrapping only)', default: [], nargs: 1, requiresArg: true },
'cloudformation-execution-policies': { type: 'array', desc: 'The Managed Policy ARNs that should be attached to the role performing deployments into this environment (may be repeated, modern bootstrapping only)', default: [], nargs: 1, requiresArg: true },
'trust': { type: 'array', desc: 'The AWS account IDs that should be trusted to perform deployments into this environment (may be repeated, modern bootstrapping only)', default: [] },
'trust-for-lookup': { type: 'array', desc: 'The AWS account IDs that should be trusted to look up values in this environment (may be repeated, modern bootstrapping only)', default: [] },
'cloudformation-execution-policies': { type: 'array', desc: 'The Managed Policy ARNs that should be attached to the role performing deployments into this environment (may be repeated, modern bootstrapping only)', default: [] },
'force': { alias: 'f', type: 'boolean', desc: 'Always bootstrap even if it would downgrade template version', default: false },
'termination-protection': { type: 'boolean', default: undefined, desc: 'Toggle CloudFormation termination protection on the bootstrap stacks' },
'show-template': { type: 'boolean', desc: 'Instead of actual bootstrapping, print the current CLI\'s bootstrapping template to stdout for customization', default: false },
Expand Down Expand Up @@ -109,12 +109,12 @@ export function makeConfig(): CliConfig {
description: 'Deploys the stack(s) named STACKS into your AWS account',
options: {
'all': { type: 'boolean', desc: 'Deploy all available stacks', default: false },
'build-exclude': { type: 'array', alias: 'E', nargs: 1, desc: 'Do not rebuild asset with the given ID. Can be specified multiple times', default: [] },
'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' },
'require-approval': { type: 'string', choices: [RequireApproval.Never, RequireApproval.AnyChange, RequireApproval.Broadening], desc: 'What security-sensitive changes need manual approval' },
'notification-arns': { type: 'array', desc: 'ARNs of SNS topics that CloudFormation will notify with stack related events. These will be added to ARNs specified with the \'notificationArns\' stack property.', nargs: 1, requiresArg: true },
'notification-arns': { type: 'array', desc: 'ARNs of SNS topics that CloudFormation will notify with stack related events. These will be added to ARNs specified with the \'notificationArns\' stack property.' },
// @deprecated(v2) -- tags are part of the Cloud Assembly and tags specified here will be overwritten on the next deployment
'tags': { type: 'array', alias: 't', desc: 'Tags to add to the stack (KEY=VALUE), overrides tags from Cloud Assembly (deprecated)', nargs: 1, requiresArg: true },
'tags': { type: 'array', alias: 't', desc: 'Tags to add to the stack (KEY=VALUE), overrides tags from Cloud Assembly (deprecated)' },
'execute': { type: 'boolean', desc: 'Whether to execute ChangeSet (--no-execute will NOT execute the ChangeSet) (deprecated)', deprecated: true },
'change-set-name': { type: 'string', desc: 'Name of the CloudFormation change set to create (only if method is not direct)' },
'method': {
Expand All @@ -125,7 +125,7 @@ export function makeConfig(): CliConfig {
desc: 'How to perform the deployment. Direct is a bit faster but lacks progress information',
},
'force': { alias: 'f', type: 'boolean', desc: 'Always deploy stack even if templates are identical', default: false },
'parameters': { type: 'array', desc: 'Additional parameters passed to CloudFormation at deploy time (STACK:KEY=VALUE)', nargs: 1, requiresArg: true, default: {} },
'parameters': { type: 'array', desc: 'Additional parameters passed to CloudFormation at deploy time (STACK:KEY=VALUE)', default: {} },
'outputs-file': { type: 'string', alias: 'O', desc: 'Path to file where stack outputs will be written as JSON', requiresArg: true },
'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)' },
'toolkit-stack-name': { type: 'string', desc: 'The name of the existing CDK toolkit stack (only used for app using legacy synthesis)', requiresArg: true },
Expand All @@ -136,11 +136,6 @@ export function makeConfig(): CliConfig {
'Note: do **not** disable this flag for deployments with resource replacements, as that will always fail',
negativeAlias: 'R',
},
'R': {
type: 'boolean',
hidden: true,
// Hack to get '-R' as an alias for '--no-rollback', suggested by: https://github.com/yargs/yargs/issues/1729
},
'hotswap': {
type: 'boolean',
desc: "Attempts to perform a 'hotswap' deployment, " +
Expand Down Expand Up @@ -199,8 +194,6 @@ export function makeConfig(): CliConfig {
'orphan': {
// alias: 'o' conflicts with --output
type: 'array',
nargs: 1,
requiresArg: true,
desc: 'Orphan the given resources, identified by their logical ID (can be specified multiple times)',
default: [],
},
Expand Down Expand Up @@ -261,7 +254,7 @@ export function makeConfig(): CliConfig {
// .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', nargs: 1, desc: 'Do not rebuild asset with the given ID. Can be specified multiple times', default: [] },
'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' },
'force': { alias: 'f', type: 'boolean', desc: 'Always deploy stack even if templates are identical', default: false },
Expand All @@ -271,12 +264,7 @@ export function makeConfig(): CliConfig {
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',
negativeAlias: '-R',
},
// same hack for -R as above in 'deploy'
'R': {
type: 'boolean',
hidden: true,
negativeAlias: 'R',
},
'hotswap': {
type: 'boolean',
Expand Down Expand Up @@ -436,7 +424,7 @@ export class DynamicValue {
public static fromInline(f: () => any): DynamicResult {
return {
dynamicType: 'function',
dynamicValue: f,
dynamicValue: f.toString(),
};
}
}
33 changes: 17 additions & 16 deletions packages/aws-cdk/lib/parse-command-line-arguments.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ export function parseCommandLineArguments(
alias: 'p',
desc: 'Name or path of a node package that extend the CDK features. Can be specified multiple times',
nargs: 1,
requiresArg: true,
})
.option('trace', {
type: 'boolean',
Expand Down Expand Up @@ -148,6 +149,8 @@ export function parseCommandLineArguments(
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: [],
nargs: 1,
requiresArg: true,
})
.command(['list [STACKS..]', 'ls [STACKS..]'], 'Lists all stacks in the app', (yargs: Argv) =>
yargs
Expand Down Expand Up @@ -231,9 +234,9 @@ export function parseCommandLineArguments(
type: 'array',
alias: 't',
desc: 'Tags to add for the stack (KEY=VALUE)',
default: [],
nargs: 1,
requiresArg: true,
default: [],
})
.option('execute', {
type: 'boolean',
Expand Down Expand Up @@ -339,9 +342,10 @@ export function parseCommandLineArguments(
.option('build-exclude', {
type: 'array',
alias: 'E',
nargs: 1,
desc: 'Do not rebuild asset with the given ID. Can be specified multiple times',
default: [],
nargs: 1,
requiresArg: true,
})
.option('exclusively', {
type: 'boolean',
Expand Down Expand Up @@ -391,9 +395,9 @@ export function parseCommandLineArguments(
.option('parameters', {
type: 'array',
desc: 'Additional parameters passed to CloudFormation at deploy time (STACK:KEY=VALUE)',
default: {},
nargs: 1,
requiresArg: true,
default: {},
})
.option('outputs-file', {
type: 'string',
Expand All @@ -420,11 +424,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('rollback', 'R'), true)
.option('R', {
type: 'boolean',
hidden: true,
})
.middleware(yargsNegativeAlias('R', 'rollback'), true)
.option('R', { type: 'boolean', hidden: 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 @@ -486,10 +487,10 @@ export function parseCommandLineArguments(
})
.option('orphan', {
type: 'array',
nargs: 1,
requiresArg: true,
desc: 'Orphan the given resources, identified by their logical ID (can be specified multiple times)',
default: [],
nargs: 1,
requiresArg: true,
})
)
.command('import [STACK]', 'Import existing resource(s) into the given STACK', (yargs: Argv) =>
Expand Down Expand Up @@ -535,9 +536,10 @@ export function parseCommandLineArguments(
.option('build-exclude', {
type: 'array',
alias: 'E',
nargs: 1,
desc: 'Do not rebuild asset with the given ID. Can be specified multiple times',
default: [],
nargs: 1,
requiresArg: true,
})
.option('exclusively', {
type: 'boolean',
Expand Down Expand Up @@ -568,11 +570,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('rollback', '-R'), true)
.option('R', {
type: 'boolean',
hidden: true,
})
.middleware(yargsNegativeAlias('R', 'rollback'), true)
.option('R', { type: 'boolean', hidden: 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 @@ -734,6 +733,8 @@ export function parseCommandLineArguments(
.option('filter', {
type: 'array',
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"',
nargs: 1,
requiresArg: true,
})
.option('compress', {
type: 'boolean',
Expand Down
21 changes: 21 additions & 0 deletions packages/aws-cdk/lib/util/yargs-helpers.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
/**
* 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
*/
export function yargsNegativeAlias<T extends { [x in S | L]: boolean | undefined }, S extends string, L extends string>(
negativeAlias: S,
optionToNegate: L,
): (argv: T) => T {
return (argv: T) => {
// if R in argv && argv[R]
// then argv[rollback] = false
if (negativeAlias in argv && argv[negativeAlias]) {
(argv as any)[optionToNegate] = false;
}
return argv;
};
}
8 changes: 8 additions & 0 deletions packages/aws-cdk/test/parse-command-line-arguments.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import { parseCommandLineArguments } from '../lib/parse-command-line-arguments';
import { yargsNegativeAlias } from '../lib/util/yargs-helpers';

test('cdk deploy -R sets rollback to false', async () => {
const argv = await parseCommandLineArguments(['deploy', '-R'], 'open', ['typescript'], ['typescript'], 'test', yargsNegativeAlias);

expect(argv.rollback).toBe(false);
});
Loading

0 comments on commit 2e75924

Please sign in to comment.