Skip to content
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
wants to merge 43 commits into
base: main
Choose a base branch
from

Conversation

wconrad265
Copy link
Contributor

@wconrad265 wconrad265 commented Oct 15, 2024

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.

Worked on this with @tlane25

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:

  • Creating a dedicated prompts folder under the utils directory to store logic and messages for each command. Each command with prompts now has a corresponding file (e.g., env-set-prompts.ts for env:set) to keep the code modular and organized.
  • Leveraging the existing 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

image

after

image


env:unset

The user will be prompted to confirm deletion if they don’t pass the --force flag

before

image

after

image


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

image

after

image


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

image

after

image


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

image

after

image


Testing and Validation

  • Manually tested each command to ensure that confirmation prompts appear under the correct conditions.
    • Example: For env:set, the prompt only appears if the environment variable already exists.
    • The -force flag correctly skips the prompt where applicable.
  • Added tests in the corresponding test files for each command:
    • The tests validate that the prompt appears under the correct conditions.
    • The tests verify that the -force flag bypasses the confirmation prompt as intended.
    • The tests also confirm that if the user opts not to continue, the process exits as expected.

Testing and Setup Explanation

In the test setups using setupFixtureTests('empty-project'), we needed to ensure that commands like blobs:set function as expected. However, since callCli spins up a separate Node.js process for each test and interacts with the command-line interface, we encountered issues with inquirer prompts that require user input.

Issues

  • CLI prompts: The commands now include inquirer confirmation prompts that expect user interaction. When running tests with callCli, these prompts cause the tests to hang and eventually time out, as no input is provided to the prompts.
  • Solution: We added the -force flag to all tests invoking commands with confirmation prompts. This bypasses the interactive prompts, ensuring the tests proceed without waiting for user input.
setupFixtureTests('empty-project', { mockApi: { routes } }, () => {
  const expectedSuccessMessage = 'Success: Blob my-key set in store my-store';

  test<FixtureTestContext>('should set, get, list, and delete blobs', async ({ fixture }) => {
    // The --force flag is used to bypass the inquirer prompt and prevent timeouts
    expect(
      await fixture.callCli(['blobs:set', 'my-store', 'my-key', 'Hello world', '--force'], {
        offline: false,
      }),
    ).toBe(expectedSuccessMessage);
  });
});

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 prompts

We used withMockApi, which spins up a local Express server to simulate the API for testing. Then, we mocked inquirer.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:

  • Open a bug/issue before writing your code 🧑‍💻. This ensures we can discuss the changes and get feedback from everyone that should be involved. If you`re fixing a typo or something that`s on fire 🔥 (e.g. incident related), you can skip this step.
  • Read the contribution guidelines 📖. This ensures your code follows our style guide and
    passes our tests.
  • Update or add tests (if any source code was changed or added) 🧪
  • Update or add documentation (if features were changed or added) 📝
  • Make sure the status checks below are successful ✅

A picture of a cute animal (not mandatory, but encouraged)

image
image

wconrad265 and others added 17 commits October 9, 2024 11:32
Will prompt the user if scope and/or context is not provided

Co-authored-by: Thomas Lane <[email protected]>
Co-authored-by: Will <[email protected]>
Co-authored-by: Will <[email protected]>
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 <[email protected]>
Co-authored-by: Thomas Lane <[email protected]>
for blobl:set and blob:delete

Co-authored-by: Thomas Lane <[email protected]>
updated the documentation

Co-authored-by: Thomas Lane <[email protected]>
updated error handeling

Co-authored-by: Thomas Lane <[email protected]>
Co-authored-by: Thomas Lane <[email protected]>
updated prompts spacing for consistencey

Co-authored-by: Thomas Lane <[email protected]>
Co-authored-by: Thomas Lane <[email protected]>
@wconrad265 wconrad265 changed the title Prompts Add Confirmation Prompts to CLI Commands to Prevent Unsafe Production Modifications Oct 15, 2024
@wconrad265 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
@wconrad265 wconrad265 marked this pull request as ready for review October 15, 2024 15:38
@wconrad265 wconrad265 requested review from a team as code owners October 15, 2024 15:38
Copy link
Contributor

@DanielSLew DanielSLew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thank you for you doing this! Just a couple suggestions!

src/commands/env/env-set.ts Outdated Show resolved Hide resolved
src/utils/prompts/blob-set-prompt.ts Outdated Show resolved Hide resolved
src/utils/prompts/env-clone-prompt.ts Outdated Show resolved Hide resolved
wconrad265 and others added 7 commits October 21, 2024 10:15
refactored messages in env-set to a function that exports an object to be reused

Co-authored-by: Thomas Lane <[email protected]>
Co-authored-by: Thomas Lane <[email protected]>
Co-authored-by: Thomas Lane <[email protected]>
env and blob commands

Co-authored-by: Thomas Lane <[email protected]>
Co-authored-by: Thomas Lane <[email protected]>
Co-authored-by: Thomas Lane <[email protected]>
Co-authored-by: Thomas Lane <[email protected]>
@wconrad265 wconrad265 requested a review from a team as a code owner October 22, 2024 17:57
@tlane25
Copy link
Contributor

tlane25 commented Oct 22, 2024

Updates (w/ @wconrad265 )

  1. Scripted command prompt override:
    • added the property scriptedCommand to BaseCommand to override prompts when hazardous commands run in non-interactive shell or CI environment.
  2. Testing for Scripted command prompt override
  3. Centralized Message Storage:
    • Created a new file called prompt-messages in the prompts folder
    • This file now serves as a central repository for all prompt-related messages
  4. Streamlined Prompt Files:
    • Cleaned up and standardized the format of all prompt files
    • Made them more concise and consistent with each other
  5. Test Updates:
    • Modified existing tests to reference messages from the new centralized store
    • This ensures all tests now use the same standardized messages

Copy link
Contributor

@DanielSLew DanielSLew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Just have a request to clean up the prompt files a little bit!

src/commands/blobs/blobs-delete.ts Outdated Show resolved Hide resolved
src/utils/prompts/blob-delete-prompts.ts Outdated Show resolved Hide resolved
wconrad265 and others added 16 commits October 24, 2024 10:46
scripted commands automatically given the force flag
Co-authored-by: Will <[email protected]>
fixed lint issue that was casuing test in ci enviroment to fail

Co-authored-by: Thomas Lane <[email protected]>
Co-authored-by: Thomas Lane <[email protected]>
Co-authored-by: Thomas Lane <[email protected]>
@wconrad265
Copy link
Contributor Author

wconrad265 commented Oct 30, 2024

All of the tests now pass, and we are ready for a re-review. Big Thanks to @tlane25 for working on this with me.

  • Created central location for prompt messages
  • Refactored the individual prompt files to not de-structure messages
  • Prompts will no longer show in non-interactive shell sessions or CI/CD environment
    • These environments are automatically detected and the force flag is only added to the appropriate commands
    • extracted this logic to a reusable function that can also be reused for the tests.
  • Created a set for the commands that need the --force flag added, commands in this set will have the --force flag automatically added as a flag option
  • Refactored tests for new prompts files
    • Created a new ENV variable that is added, for testing prompts so they can be tested in CI/CD environments
  • Added tests to verify that prompts will not show in non-interactive shell sessions or CI/CD environments
  • Fixed a bug where tests that mutated the proces.env were not restoring the env variable to the original state, that was causing tests to fail. These tests would pass if you ran the test file by its self, but fail when running multiple tests together such as the integration tests above.
  • Fixed a bug in the src/commands/intergration/deploy.ts file, that caused the corresponding test to be flakey.
  • When you import { env } from 'process', it takes a snapshot of the env variable. If tests before this call { env }, then it would not have the properties that were added by the test, causing the env.INTEGRATION_URL to be undefined. This happened when multiple tests files were run in a certain order together. Test should no longer be flakey
  • docs have been updated.

The commands that had the --force flag originally have been left commented out, so you can the original messages. They can be deleted, when everything looks good. If we would want to keep the original messages, we can always change the set to an object, and have the value the message.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the old --force option, the -f option no longer works. Do we want to add -f for just this option. Is this ok?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is another spot, where the message will be changed from this message to a more generic message, for --force

@@ -33,7 +33,7 @@ and configures your Git environment with the right credentials.`,
.command('lm:setup', { hidden: true })
.description('Configures your site to use Netlify Large Media')
.option('-s, --skip-install', 'Skip the credentials helper installation check')
.option('-f, --force-install', 'Force the credentials helper installation')
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is another location where we use --force-install, and -f. In the new update, it is a unified command of --force

@@ -23,7 +23,7 @@ export const createLmCommand = (program: BaseCommand) => {
It installs the required credentials helper for Git,
and configures your Git environment with the right credentials.`,
)
.option('-f, --force', 'Force the credentials helper installation')
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is another place as well, the -f option no longer works. It is now just --force with a generic message

@@ -81,7 +81,7 @@ export const createSitesCommand = (program: BaseCommand) => {
.command('sites:delete')
.description('Delete a site\nThis command will permanently delete the site on Netlify. Use with caution.')
.argument('<siteId>', 'Site ID to delete.')
.option('-f, --force', 'delete without prompting (useful for CI)')
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another place where just --force will now work

@@ -230,5 +242,11 @@ export const createMainCommand = () => {
})
.action(mainCommand)

program.commands.forEach((cmd) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is where we add the --force option to each command that is in the set above. This allows the --force option to be unified across all commands, for adding the --force flag in ci/cd environments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants