-
Notifications
You must be signed in to change notification settings - Fork 352
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
feat: add confirmation prompts to unsafe cli commands #6878
Open
wconrad265
wants to merge
17
commits into
netlify:main
Choose a base branch
from
wconrad265:prompts
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+1,144
−105
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Will prompt the user if scope and/or context is not provided Co-authored-by: Thomas Lane <tlane25@users.noreply.github.com>
Co-authored-by: Will <wconrad265@users.noreply.github.com>
Co-authored-by: Will <wconrad265@users.noreply.github.com>
Co-authored-by: Will <wconrad265@users.noreply.github.com>
Co-authored-by: Will <wconrad265@users.noreply.github.com>
…nantly across contexts Co-authored-by: Will <wconrad265@users.noreply.github.com>
Created several tests to check env:test prompts
created a new directory in utils called prompts, to store all future prompts. rewrote the prompts to only check for destructive actions. added tests for each of the destructive prompts Co-authored-by: Thomas Lane <163203257+tlane25@users.noreply.github.com>
Co-authored-by: Thomas Lane <163203257+tlane25@users.noreply.github.com>
Co-authored-by: Thomas Lane <163203257+tlane25@users.noreply.github.com>
for blobl:set and blob:delete Co-authored-by: Thomas Lane <163203257+tlane25@users.noreply.github.com>
updated the documentation Co-authored-by: Thomas Lane <163203257+tlane25@users.noreply.github.com>
updated error handeling Co-authored-by: Thomas Lane <163203257+tlane25@users.noreply.github.com> Co-authored-by: Thomas Lane <tlane25@users.noreply.github.com>
updated prompts spacing for consistencey Co-authored-by: Thomas Lane <163203257+tlane25@users.noreply.github.com>
Co-authored-by: Thomas Lane <163203257+tlane25@users.noreply.github.com>
wconrad265
changed the title
Prompts
Add Confirmation Prompts to CLI Commands to Prevent Unsafe Production Modifications
Oct 15, 2024
wconrad265
changed the title
Add Confirmation Prompts to CLI Commands to Prevent Unsafe Production Modifications
feat: add confirmation prompts to unsafe cli commands
Oct 15, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
Several CLI commands currently allow users to perform actions that may accidentally modify production environments. Some actions, like setting environment variables without specifying context (or scope) can inadvertently affect all contexts (or scopes) leading to unintended consequences.
Solution
To address this, we added confirmation prompts to critical commands. These prompts inform users of potentially unsafe changes and give them a chance to cancel the operation. The prompts can be bypassed using the
-f
or--force
flag for advanced users who understand the risks.Additionally, we refactored the codebase to improve prompt management by:
prompts
folder under theutils
directory to store logic and messages for each command. Each command with prompts now has a corresponding file (e.g.,env-set-prompts.ts
forenv:set
) to keep the code modular and organized.inquirer
package for prompts, since it was already installed in the project.Commands Updated with Prompts
env:set
The current code already checks if the environment variable exists. If it does, and the user does not pass the
--force
or-f
flag, a prompt now displays, informing the user that they are about to modify the existing variable.before
after
env:unset
The user will be prompted to confirm deletion if they don’t pass the
--force
flagbefore
after
env:clone
The original code already checks, if there are environment variables of the same in name in the target site. If an environment variable with the same name already exists on the target site, a prompt informs the user that variables will be overwritten.
The
-f
flag is already used for the--from
flag, so users can only bypass it with the--force
flag.before
after
blob:set
Will check to see if a key in the store exists. If the key does exist, the user will be prompted with a warning message.
Also a confirmation message has been added, letting the user know the operation was successful, as there was not one before.
before
after
blob:delete
A prompt now displays to the user if they are deleting a key of a store.
Also a confirmation message was added.
before
after
Testing and Validation
env:set
, the prompt only appears if the environment variable already exists.-force
flag correctly skips the prompt where applicable.-force
flag bypasses the confirmation prompt as intended.Testing and Setup Explanation
In the test setups using
setupFixtureTests('empty-project')
, we needed to ensure that commands likeblobs:set
function as expected. However, sincecallCli
spins up a separate Node.js process for each test and interacts with the command-line interface, we encountered issues withinquirer
prompts that require user input.Issues
inquirer
confirmation prompts that expect user interaction. When running tests withcallCli
, these prompts cause the tests to hang and eventually time out, as no input is provided to the prompts.-force
flag to all tests invoking commands with confirmation prompts. This bypasses the interactive prompts, ensuring the tests proceed without waiting for user input.This setup allows us to validate command behavior without requiring manual input or causing test timeouts.
I wanted to mention this here, because I had to add to
--force
flag to several test files. This might be good to add to the documentation somewhere. I wasn’t sure where to add it.Testing the
inquirer
promptsWe used
withMockApi
, which spins up a local Express server to simulate the API for testing. Then, we mockedinquirer.prompt
to simulate user interaction, ensuring the correct prompts and messages were displayed.This is consistent with other tests that mock inquirer prompts as well.
Each of these commands has tests in their corresponding test files. If a command did not have a test file that corresponded to it, it was created.
Documentation
Documentation has been updated to reflect the new prompts and the option to use the force flag for bypassing.
Looking forward to comments!
Also here is some dog pictures of my dogs!
Looking forward to comments!
Also here is some dog pictures of my dogs!
For us to review and ship your PR efficiently, please perform the following steps:
passes our tests.
A picture of a cute animal (not mandatory, but encouraged)