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 terraform destroy command #674

Closed
wants to merge 2 commits into from

Conversation

dbanck
Copy link
Member

@dbanck dbanck commented Jun 20, 2021

Hi all!

This PR adds the Terraform: destroy command and resolves #646.

When using the existing terraformCommand function to run the command, the user will see this prompt before the command is executed inside the terminal.

CleanShot 2021-06-20 at 17 14 27@2x

I think this prompt doesn't convey the potential danger of running destroy.

Additional prompts

There are (at least) two more options for adding a prominent prompt before running the command.

Option A
This will open a modal at the position of the command palette of VSCode and ask for user confirmation.

I like this option because it keeps the user focus at the same position and locks the editor until a decision is made.

CleanShot 2021-06-20 at 17 22 23@2x

Option B
This will open a warning message in the bottom right corner where most notifications appear and ask for user confirmation.

Opening the message at this position could go unnoticed, and it could seem that the command didn't do anything at all.

CleanShot 2021-06-20 at 17 23 09@2x

Solution

Which option do you prefer? This PR currently implements option A.

I couldn't find any tests for the other registered commands, so I added none for this new command. But I would suggest adding some kind of tests for commands, too.

@hashicorp-cla
Copy link

hashicorp-cla commented Jun 20, 2021

CLA assistant check
All committers have signed the CLA.

@jpogran
Copy link
Contributor

jpogran commented Sep 23, 2021

Thanks @dbanck for the contribution! Apologies we haven't replied sooner.

I feel like the modal dialog is necessary here because the action is potentially destructive. The other option is too temporary and easily missed.

I don't see this handling the prompt for user confirmation by passing -auto-approve. Are you expecting the user to see the prompt in the terminal and know what to do? I'm asking because I'm walking through the UX here and trying to find a good balance. Longer term, we're trying to figure out how we surface these kinds of commands that either require user input or should show the user results. A similar scenario is #700.

While we figure this out, I'm going to mark this as a draft and add this to our planning board.

@jpogran jpogran added the enhancement New feature or request label Sep 23, 2021
@jpogran jpogran marked this pull request as draft September 23, 2021 15:33
@dbanck
Copy link
Member Author

dbanck commented Feb 14, 2022

I will close this issue as development has moved on, and the underlying issue is still tracked in #646.

@dbanck dbanck closed this Feb 14, 2022
@dbanck dbanck deleted the feat/646/terraform-destroy branch February 14, 2022 15:39
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement Terraform: Destroy as a command
3 participants