fix(cli): --require-approval any-change only asks for confirmation on broadening changes#1246
Conversation
| private skipApprovalStep(msg: IoRequest<any, any>): boolean { | ||
| const approvalToolkitCodes = ['CDK_TOOLKIT_I5060']; | ||
| if (!(msg.code && approvalToolkitCodes.includes(msg.code))) { | ||
| false; |
There was a problem hiding this comment.
This is something the original issue creator mention and indeed seems like a bug. Looks like we are missing some tests here but I'd like to not delay this fix for it.
| // the ioHost uses this internally to determine if a confirmation | ||
| // is actually needed, so it needs the same value we have here. | ||
| // ideally this would threaded into the request instead of it being an instance field. | ||
| this.ioHost.requireDeployApproval = requireApproval; |
There was a problem hiding this comment.
See
aws-cdk-cli/packages/aws-cdk/lib/cli/io-host/cli-io-host.ts
Lines 383 to 393 in ad90be6
There was a problem hiding this comment.
this is fine and im approving. but i feel like passing the requireApproval property to the iohost should happen when we define requireApproval on line 428 rather than here.
const requireApproval = options.requireApproval ?? RequireApproval.BROADENING;
this.ioHost.requireDeployApproval = requireApproval;and then maybe that can be a helper function to make those actions atomic.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1246 +/- ##
==========================================
+ Coverage 87.96% 88.09% +0.12%
==========================================
Files 74 74
Lines 10363 10371 +8
Branches 1385 1388 +3
==========================================
+ Hits 9116 9136 +20
+ Misses 1221 1209 -12
Partials 26 26
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Fast follow for #1246. Existing tests are sufficient. --- By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license
Fixes #1236
Problem
The value of
--require-approvalis not being passed to theCliIoHost, which defaults it toRequireApproval.BROADENING.Solution
The correct
requireApprovalvalue is already being passed todeploy, so we just need to propagate it into theCliIoHostvia the setter. This isn't ideal because it prevents concurrent deployments with differentrequireApprovalvalues, but the CLI can't do that anyway so its not an actual issue.Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license