-
Notifications
You must be signed in to change notification settings - Fork 54
feat(cli): --yes option to accept all questions
#826
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?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #826 +/- ##
==========================================
- Coverage 83.91% 83.80% -0.11%
==========================================
Files 71 71
Lines 10401 10431 +30
Branches 1326 1329 +3
==========================================
+ Hits 8728 8742 +14
- Misses 1632 1650 +18
+ Partials 41 39 -2
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:
|
| // In non-interactive mode, always return success (true). | ||
| if (this.nonInteractive) { | ||
| return true; | ||
| } | ||
|
|
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.
Q. It should return msg.defaultResponse instead of true?
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.
Yes, we need to return true for isConfirmationPrompt, msg.defaultResponse if there is one, and continue otherwise.
I would also like us to still print the question and answer and indicating that --yes was used.
We should also only do this after the this.skipApprovalStep(msg) check. On the other hand, --yes should make scenarios work where we don't have a TTY or are using concurrency. So we will need to restructure the checks.
mrgrain
left a comment
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.
Two existing uses of promptly that should be changed over as well as part of this:
| const confirmed = await promptly.confirm( |
| const confirmed = await promptly.confirm(`${chalk.cyan(question)} (y/n)?`); |
Refactor should be a confirm:
aws-cdk-cli/packages/@aws-cdk/toolkit-lib/lib/toolkit/toolkit.ts
Lines 1193 to 1196 in 5bb90cb
| const response = await ioHelper.requestResponse(IO.CDK_TOOLKIT_I8910.req(question, { | |
| responseDescription: '[Y]es/[n]o', | |
| }, 'y')); | |
| return ['y', 'yes'].includes(response.toLowerCase()); |
| // In non-interactive mode, always return success (true). | ||
| if (this.nonInteractive) { | ||
| return true; | ||
| } | ||
|
|
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.
Yes, we need to return true for isConfirmationPrompt, msg.defaultResponse if there is one, and continue otherwise.
I would also like us to still print the question and answer and indicating that --yes was used.
We should also only do this after the this.skipApprovalStep(msg) check. On the other hand, --yes should make scenarios work where we don't have a TTY or are using concurrency. So we will need to restructure the checks.
…rmationRequest interface (#904) This PR standardizes user confirmation flows across the CDK CLI by migrating from ad-hoc `promptly.confirm` usage to the structured `ConfirmationRequest` interface. These are the changes as suggested here: #826 (review) ## Changes - **Convert CDK_TOOLKIT_I8910**: Changed from `DataRequest` to `ConfirmationRequest` for refactor confirmation - **Standardize destroy flow**: Replace `promptly.confirm` with `IO.CDK_TOOLKIT_I7010` message - **Standardize deploy flow**: Update `askUserConfirmation` to use `ActionLessRequest<ConfirmationRequest>` pattern - **Clean up test infrastructure**: Remove deprecated `markTesting` function and `TESTING` variable - **Update tests**: Replace `promptly` mocks with `requestSpy` for consistency ## Benefits - Consistent user interaction patterns across all CLI commands - Better testability through standardized mocking - Improved error handling and TTY detection through the IO system - Cleaner separation of concerns between UI logic and business logic --- By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license
…rmationRequest interface (#904) This PR standardizes user confirmation flows across the CDK CLI by migrating from ad-hoc `promptly.confirm` usage to the structured `ConfirmationRequest` interface. These are the changes as suggested here: #826 (review) ## Changes - **Convert CDK_TOOLKIT_I8910**: Changed from `DataRequest` to `ConfirmationRequest` for refactor confirmation - **Standardize destroy flow**: Replace `promptly.confirm` with `IO.CDK_TOOLKIT_I7010` message - **Standardize deploy flow**: Update `askUserConfirmation` to use `ActionLessRequest<ConfirmationRequest>` pattern - **Clean up test infrastructure**: Remove deprecated `markTesting` function and `TESTING` variable - **Update tests**: Replace `promptly` mocks with `requestSpy` for consistency ## Benefits - Consistent user interaction patterns across all CLI commands - Better testability through standardized mocking - Improved error handling and TTY detection through the IO system - Cleaner separation of concerns between UI logic and business logic --- By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license
I went ahead and made these unrelated changes in: #904 |
--yes option to accept all questions
cf7a224 to
b670116
Compare
47e36e3 to
2acab46
Compare
Adds a new
--yescli option so that when provided, commands can continue without confirmation. Any confirmation prompts will be accepted. Other user input will use a default value if available. The CLI might still prompt for input in situations where there is no default answer.Fixes #732
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license