fix(cli): cdk deploy -R does not disable rollback #32514
Conversation
imply `nargs: 1` and `requiresArg: true` for all arrays
mrgrain
had a problem deploying
to
test-pipeline
GitHub Actions
Failure
There was a problem hiding this comment.
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.
aws-cdk-automation
added
the
pr/needs-cli-test-run
| @@ -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>( | |||
There was a problem hiding this comment.
Moved to a separate file so it can be used in tests.
mrgrain
had a problem deploying
to
test-pipeline
GitHub Actions
Failure
| @@ -436,7 +424,7 @@ export class DynamicValue { | |||
| public static fromInline(f: () => any): DynamicResult { | |||
| return { | |||
| dynamicType: 'function', | |||
| dynamicValue: f, | |||
| dynamicValue: f.toString(), | |||
There was a problem hiding this comment.
This makes the config object structuredClone-able. Previously toString() was just called a little later.
There was a problem hiding this comment.
All removals in this file are now codgened instead.
| @@ -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, | |||
There was a problem hiding this comment.
This is fixing a previous oversight.
| nargs: 1, | ||
| requiresArg: true, |
There was a problem hiding this comment.
This is fixing a previous oversight.
| desc: 'Do not rebuild asset with the given ID. Can be specified multiple times', | ||
| default: [], | ||
| nargs: 1, | ||
| requiresArg: true, |
There was a problem hiding this comment.
This is fixing a previous oversight.
| desc: 'Do not rebuild asset with the given ID. Can be specified multiple times', | ||
| default: [], | ||
| nargs: 1, | ||
| requiresArg: true, |
There was a problem hiding this comment.
This is fixing a previous oversight.
| nargs: 1, | ||
| requiresArg: true, |
There was a problem hiding this comment.
This is fixing a previous oversight.
| @@ -34,6 +26,9 @@ export interface YargsOption { | |||
| requiresArg?: boolean; | |||
| hidden?: boolean; | |||
| count?: boolean; | |||
| } | |||
|
|
|||
| export interface CliOption extends Omit<YargsOption, 'nargs' | 'hidden'> { | |||
There was a problem hiding this comment.
Not every YargsOption property is directly available to the CliConfig.
| nargs: 1, | ||
| requiresArg: true, |
There was a problem hiding this comment.
Removed here, but snapshot is unchanged.
mrgrain
force-pushed
the
mrgrain/chore/cleanup-cli-options
branch
from
f1912cd to
5b0881d
Compare
mrgrain
added
the
pr-linter/exempt-integ-test
|
Covered by existing CLI integ tests. The actual fix was verified through a unit test. |
mrgrain
temporarily deployed
to
test-pipeline
GitHub Actions
Inactive
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #32514 +/- ##
==========================================
+ Coverage 78.66% 78.77% +0.11%
==========================================
Files 107 108 +1
Lines 7161 7162 +1
Branches 1320 1320
==========================================
+ Hits 5633 5642 +9
+ Misses 1343 1335 -8
Partials 185 185
Flags with carried forward coverage won't be shown. Click here to find out more.
|
mrgrain
temporarily deployed
to
test-pipeline
GitHub Actions
Inactive
|
➡️ PR build request submitted to A maintainer must now check the pipeline and add the |
mrgrain
had a problem deploying
to
test-pipeline
GitHub Actions
Failure
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
mrgrain
removed
the
pr/needs-cli-test-run
mrgrain
added
the
pr-linter/cli-integ-tested
| @@ -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"); | |||
There was a problem hiding this comment.
This fixed the failing build introduced by #32476
aws-cdk-automation
dismissed
their stale review
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
|
Comments on closed issues and PRs are hard for our team to see. |
Issue
cdk deploy -Rshould be the same ascdk deploy --no-rollbackorcdk deploy --rollback=false, but has no effectReason for this change
PR #31850 introduced this bug by accidentally flipping the order of arguments passed to the
yargsNegativeAliashelper.This caused the helper to have no effect.
Description of changes
yargsNegativeAliasnargs: 1andrequiresArg: trueimplied 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
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license