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 17 commits into
base: main
Choose a base branch
from

Conversation

wconrad265
Copy link

@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.

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 <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 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
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.

1 participant