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

cli: show workspace name in destroy confirmation #18253

Merged
merged 2 commits into from
Jun 19, 2018

Conversation

mildwonkey
Copy link
Contributor

If the workspace name is not "default", include it in the confirmation
message for terraform destroy.

Fixes #15480

Plan: 0 to add, 0 to change, 1 to destroy.

Do you really want to destroy?
  Terraform will destroy all your managed infrastructure in the worksapce "test" as shown above.
  There is no undo. Only 'yes' will be accepted to confirm.

  Enter a value: yes

If the workspace name is not "default", include it in the confirmation
message for `terraform destroy`.

Fixes #15480
@mildwonkey
Copy link
Contributor Author

My own concerns:

  • the line length now seems long, should I wrap it?
  • " around the workspace name (and wording in general)

@apparentlymart
Copy link
Contributor

Cool!

We do usually try to keep messages wrapped at ~78 characters right now, but it can be tricky to do that with messages like this which contain dynamic elements that can be of any length (the workspace name). I expect that in a future release we'll start to make most of Terraform's output sensitive to the actual terminal size and do dynamic wrapping, but right now we don't have good helper functions to do that reasonably except for the recent addition of our diagnostic message formatter:

if desc.Detail != "" {
detail := desc.Detail
if width != 0 {
detail = wordwrap.WrapString(detail, uint(width))
}
fmt.Fprintf(&buf, "%s\n", detail)
}

So with that said, for now I think the best compromise would be to carefully design the wording so that the dynamic part goes somewhere that doesn't need to be wrapped, and we can potentially adjust it further later as part of a more comprehensive revamp of the output here.

One of our design mocks for some UI changes in a forthcoming release included this for the prompt:

Do you want to perform these actions in the workspace "production"?

In this case, the workspace name ends up in the "heading" portion instead of in the paragraph below, and so it is less likely to need to be wrapped (unless the workspace name is excessively long).

The mock above was for the terraform apply confirmation rather than for destroy, but I think we could do something similar for destroy with some light tweaks:

Do you want to destroy all resources in workspace "test"?
  Terraform will destroy all resources, as shown above.
  Only 'yes' will be accepted to approve.

  Enter a value:

What do you think about doing something like this for both apply and destroy, so that we'll keep them approx. consistent?

Making the message for the workspace "default" is not something I'd thought of, but it does seem like a good idea to avoid pre-emptively introducing the concept of workspaces while users are still getting started.

@mildwonkey
Copy link
Contributor Author

I agree that looks nicer, and I'll add it to the apply message as well.

Making the message for the workspace "default" is not something I'd thought of, but it does seem like a good idea to avoid pre-emptively introducing the concept of workspaces while users are still getting started.

That was my initial thought, though my other concern is that this will be confusing to users who use workspaces and make use of 'default'. With that in mind, I'm leaning towards including the 'workspace' in both messages. What do you think?

@apparentlymart
Copy link
Contributor

I think having the special case for "default" is the right tradeoff here, in retrospect. While you're right that it'll be a little weird for users that are using both default and non-default workspaces, it is at least still unambigious because the message for the default workspace is unique, and will hopefully also avoid users starting to use workspaces prematurely before they've got some Terraform experience in order to better understand the implications.

Another compromise I considered here was to switch the message if we detect that there are any namespaces other than default defined, but that'd introduce another backend API call in here to get the list of namespaces and so that feels safer to attack as a separate change, since in some more restrictive configurations the "list workspaces" operation might be protected (e.g. by IAM policies, for the s3 backend).

@mildwonkey mildwonkey merged commit 85be12d into master Jun 19, 2018
@mildwonkey mildwonkey deleted the mildwonkey/f-cli-env branch June 19, 2018 20:35
xiaozhu36 pushed a commit to xiaozhu36/terraform that referenced this pull request Aug 9, 2018
* cli: show workspace name in destroy confirmation

If the workspace name is not "default", include it in the confirmation
message for `terraform destroy`.

Fixes hashicorp#15480
xiaozhu36 pushed a commit to xiaozhu36/terraform that referenced this pull request Aug 9, 2018
* cli: show workspace name in destroy confirmation

If the workspace name is not "default", include it in the confirmation
message for `terraform destroy`.

Fixes hashicorp#15480
@ghost
Copy link

ghost commented Apr 3, 2020

I'm going to lock this issue 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 similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Apr 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

have destroy messaging (and plan -destroy ) include env
2 participants