-
Notifications
You must be signed in to change notification settings - Fork 108
feat(toolkit-lib): simplify deploy approval message and surface auto-confirm #1585
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.
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
base: main
Are you sure you want to change the base?
Changes from 14 commits
90ff4a1
41c4504
5e9031f
a1f1697
0eea727
67cd2b0
176ee61
01637af
a98a20c
5839bb4
6ac4963
e2f9454
8f46813
052988a
a9204b2
a41d958
cad6741
6d8e25e
3ceeb11
c34ca43
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -857,18 +857,19 @@ export class Toolkit extends CloudAssemblySourceBuilder { | |||
| const stackDiff = formatter.formatStackDiff(); | ||||
|
|
||||
| // Send a request response with the diff as part of the message, | ||||
| // and the template diff as data | ||||
| // (IoHost decides whether to print depending on permissionChangeType) | ||||
| // and the template diff as data. The IoHost decides how to handle the | ||||
| // request — interactively prompt the user, auto-confirm, or suppress. | ||||
| const hasSecurityChanges = securityDiff.permissionChangeType !== PermissionChangeType.NONE; | ||||
| const deployMotivation = hasSecurityChanges | ||||
| ? '"--require-approval" is enabled and stack includes security-sensitive updates.' | ||||
| : '"--require-approval" is enabled and stack includes updates.'; | ||||
| ? 'Stack includes security-sensitive updates' | ||||
| : 'Stack includes updates'; | ||||
| const diffOutput = hasSecurityChanges ? securityDiff.formattedDiff : stackDiff.formattedDiff; | ||||
| const deployQuestion = `${diffOutput}\n\n${deployMotivation}\nDo you wish to deploy these changes`; | ||||
| const deployQuestion = `${diffOutput}\n\n${deployMotivation}. Do you wish to deploy these changes?`; | ||||
| const deployConfirmed = await ioHelper.requestResponse(IO.CDK_TOOLKIT_I5060.req(deployQuestion, { | ||||
| motivation: deployMotivation, | ||||
| concurrency, | ||||
| permissionChangeType: securityDiff.permissionChangeType, | ||||
| hasSecurityChanges, | ||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. redundant
Suggested change
|
||||
| templateDiffs: formatter.diffs, | ||||
| })); | ||||
| if (!deployConfirmed) { | ||||
|
|
||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2334,9 +2334,11 @@ class WorkGraphDeploymentActions implements WorkGraphActions { | |
| const securityDiff = formatter.formatSecurityDiff(); | ||
| if (requiresApproval(this.options.requireApproval, securityDiff.permissionChangeType)) { | ||
| const hasSecurityChanges = securityDiff.permissionChangeType !== PermissionChangeType.NONE; | ||
| // Bare-fact motivation. `CliIoHost.augmentDeployApprovalMessage` | ||
| // adds the `--require-approval` framing for terminal users. | ||
| const motivation = hasSecurityChanges | ||
| ? '"--require-approval" is enabled and stack includes security-sensitive updates' | ||
| : `"--require-approval" is set to '${RequireApproval.ANYCHANGE}'`; | ||
| ? 'Stack includes security-sensitive updates' | ||
| : 'Stack includes updates'; | ||
| const diffOutput = hasSecurityChanges ? securityDiff.formattedDiff : formatter.formatStackDiff().formattedDiff; | ||
| await this.ioHost.asIoHelper().defaults.info(diffOutput); | ||
|
|
||
|
|
@@ -2347,6 +2349,7 @@ class WorkGraphDeploymentActions implements WorkGraphActions { | |
| motivation, | ||
| concurrency: this.options.concurrency, | ||
| permissionChangeType: securityDiff.permissionChangeType, | ||
| hasSecurityChanges, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. see above |
||
| templateDiffs: formatter.diffs, | ||
| }), | ||
| ); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -398,6 +398,30 @@ export class CliIoHost implements IIoHost { | |
| } | ||
| } | ||
|
|
||
| /** | ||
| * Add the `--require-approval` flag to the deploy approval prompt. | ||
| */ | ||
| private augmentDeployApprovalMessage(msg: IoRequest<any, any>): string { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| // Library-emitted I5060 messages always carry data, but defend against | ||
| // partially-shaped inputs (e.g. tests, stubs) so we never crash a prompt. | ||
| if (!IO.CDK_TOOLKIT_I5060.is(msg) || !msg.data) { | ||
| return msg.message; | ||
| } | ||
|
|
||
| const hasSecurityChanges = msg.data.hasSecurityChanges; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. check |
||
|
|
||
| if (this.requireDeployApproval === RequireApproval.BROADENING && hasSecurityChanges) { | ||
| return 'Stack includes security-sensitive updates and "--require-approval" is set to \'broadening\'.\nDo you wish to deploy these changes?'; | ||
| } | ||
| if (this.requireDeployApproval === RequireApproval.ANYCHANGE && hasSecurityChanges) { | ||
| return 'Stack includes security-sensitive updates and "--require-approval" is set to \'any-change\'.\nDo you wish to deploy these changes?'; | ||
| } | ||
| if (this.requireDeployApproval === RequireApproval.ANYCHANGE && !hasSecurityChanges) { | ||
| return 'Stack includes updates and "--require-approval" is set to \'any-change\'.\nDo you wish to deploy these changes?'; | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm. I feel this much harder to read than the old system. I had to read three times to see the differences here. Also this if is going to be annoying if we are adding more states. I suggest you keep this as a templating string: const updateTypeText = hasSecurityChanges ? "security-sensitive updates" : "updates";
const requireApproval = this.requireDeployApproval; // maybe? not sure.
return `Stack includes ${updateTypeText} and "--require-approval" is set to '${requireApproval}'.\nDo you wish to deploy these changes?`;
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. switched to the suggested template string form. it's now in the rewrite listener in |
||
| return msg.message; | ||
| } | ||
|
|
||
| /** | ||
| * Determines the output stream, based on message and configuration. | ||
| */ | ||
|
|
@@ -464,13 +488,18 @@ export class CliIoHost implements IIoHost { | |
| return true; | ||
| } | ||
|
|
||
| // The library emits I5060 with a flag-free motivation. The CLI is the | ||
| // layer that knows about `--require-approval` and adds it to the | ||
| // user-facing message before any downstream rendering. | ||
| const promptMessage = this.augmentDeployApprovalMessage(msg); | ||
|
|
||
| // In --yes mode, respond for the user if we can | ||
| if (this.autoRespond) { | ||
| // respond with yes to all confirmations | ||
| if (isConfirmationPrompt(msg)) { | ||
| await this.notify({ | ||
| ...msg, | ||
| message: `${chalk.cyan(msg.message)} (auto-confirmed)`, | ||
| message: `${chalk.cyan(promptMessage)} (auto-confirmed)`, | ||
| }); | ||
| return true; | ||
| } | ||
|
|
@@ -479,7 +508,7 @@ export class CliIoHost implements IIoHost { | |
| if (msg.defaultResponse) { | ||
| await this.notify({ | ||
| ...msg, | ||
| message: `${chalk.cyan(msg.message)} (auto-responded with default: ${util.format(msg.defaultResponse)})`, | ||
| message: `${chalk.cyan(promptMessage)} (auto-responded with default: ${util.format(msg.defaultResponse)})`, | ||
| }); | ||
| return msg.defaultResponse; | ||
| } | ||
|
|
@@ -498,7 +527,7 @@ export class CliIoHost implements IIoHost { | |
| // Basic confirmation prompt | ||
| // We treat all requests with a boolean response as confirmation prompts | ||
| if (isConfirmationPrompt(msg)) { | ||
| const confirmed = await promptly.confirm(`${chalk.cyan(msg.message)} (y/n)`); | ||
| const confirmed = await promptly.confirm(`${chalk.cyan(promptMessage)} (y/n)`); | ||
| if (!confirmed) { | ||
| throw new ToolkitError('AbortedByUser', 'Aborted by user'); | ||
| } | ||
|
|
@@ -508,7 +537,7 @@ export class CliIoHost implements IIoHost { | |
| // Asking for a specific value | ||
| const prompt = extractPromptInfo(msg); | ||
| const desc = responseDescription ?? prompt.default; | ||
| const answer = await promptly.prompt(`${chalk.cyan(msg.message)}${desc ? ` (${desc})` : ''}`, { | ||
| const answer = await promptly.prompt(`${chalk.cyan(promptMessage)}${desc ? ` (${desc})` : ''}`, { | ||
| default: prompt.default, | ||
| trim: true, | ||
| }); | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is redundant,
permissionChangeTypealready includes this.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated and removed
hasSecurityChanges