cli icon indicating copy to clipboard operation
cli copied to clipboard

feat: add confirmation prompts to unsafe cli commands

Open wconrad265 opened this issue 1 year ago β€’ 0 comments

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:

  • [x] 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.
  • [x] Read the contribution guidelines πŸ“–. This ensures your code follows our style guide and passes our tests.
  • [x] Update or add tests (if any source code was changed or added) πŸ§ͺ
  • [x] Update or add documentation (if features were changed or added) πŸ“
  • [x] Make sure the status checks below are successful βœ…

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

image image

wconrad265 avatar Oct 15 '24 15:10 wconrad265