core: add "apply -confirm-plan" flag

[glasser]

A common reason to want to use terraform plan is to have a chance to
review and confirm a plan before running it. If in fact that is the
only reason you are running plan, this new terraform apply -confirm-plan
flag provides an easier alternative to

P=$(mktemp -t plan)
terraform refresh
terraform plan -refresh=false -out=$P
terraform apply $P
rm $P

[jen20]

Hi @glasser! Thanks for opening a pull request. I'm going to loop in @phinze, @mitchellh or @jbardin here - I can understand the use case for this, but want to ensure that this is the direction we want to take it before merging.

[glasser]

Thanks. I was inspired to do this because of the 0.7 change to plan to not
write state, which makes the operation of "apply a change with precisely
one refresh and an opportunity to check the plan first" much more awkward
than it is in 0.6.

0.6:

P=$(mktemp -t plan)
terraform plan -out=$P
# decide whether to go forward
terraform apply $P
rm $P

0.7:

P=$(mktemp -t plan)
terraform refresh
terraform plan -refresh=false -out=$P
# decide whether to go forward
terraform apply $P
rm $P

This PR:

terraform apply -confirm

[jen20]

@glasser I like the UX of this approach - probably we should have made this the default, though it's a bit late to change that now! I'm going to wait for a 👍 on this from @phinze or @mitchellh still, but I am generally pro the idea.

[glasser]

Yeah, if folks like this then I do think it would be a nice default (maybe we can make it so in our setup by setting some env var or directive in the tf files) but figured that step 1 is having it available at all :)

[glasser]

OK, I should probably fix this so that it doesn't ask you to confirm empty plans:

This plan does nothing.
Do you want to apply the plan above?
  Terraform will apply the plan described above.
  Only 'yes' will be accepted to confirm.

  Enter a value: yes

[glasser]

Fixed.

[mitchellh]

The want for this seems sane to me and I like that it will likely drive more people to use plans. Funny enough, it actually feels like this should be inverted, but I think the BC implications of that are too great: terraform apply by default should plan and ask for confirmation, and -confirm should skip the confirmation. I've mentioned it to at least stir some thoughts around that.

My only other fear is a bikeshed but -confirm does seem very broad and may negatively impact future feature design on terraform apply ("confirm what?"). My only thought there is to make it slighly more verbose at -confirm-plan.

Otherwise, the feel and workflow of it feels good to me!

[jbardin]

I agree with @mitchellh. I think I'd rather see a more explicit --confirm-plan to prevent future confusion. Otherwise I like the idea as well.

[glasser]

I've updated the PR to change the flag name, as suggested.

I agree that it would be nice for this to be the default and that this isn't appropriate for BC reasons. Do you think it would make sense to allow users to set the default for this using some other mechanism such as an environment variable (I don't think Terraform really has per-project configuration files for things like this, right?)

[glasser]

Though at the risk of bikeshedding, I now can imagine "confirm" being interpreted as "I want to confirm" not "I want to be asked to confirm". Maybe even just -plan is fine?

[glasser]

Several of my co-workers have been confused by the name of this flag (our build contains this PR). @mitchellh How does just apply -plan sound to you?

[glasser]

Hmm, looks like the current implementation doesn't work well with data sources: if you have any data sources at all, it looks like you end up with a plan whose diff is not Empty() and it prompts instead of just saying there's no plan.

[glasser]

We've been using this for every deploy for 4 months now. I literally cannot fathom using terraform without this functionality and it saddens me that people outside of my project don't get to use it. What can I do to help get this merged? We might need to bikeshed about option naming, and there might be some tweaks necessary related to data sources?

[thegedge]

We write a lot of wrapper scripts just to ask for confirmation before any potentially destructive action. I think destructive actions should prompt by default. I don't think this PR solves all possible cases (e.g., terraform destroy), but does deal with the common "plan, confirm, apply" wrappers that we create.

If there's anything we can do to help push this PR along, or have more prompting in destructive / dangerous actions?

[glasser]

I've rebased this PR and ported it to work with the new Backend support.

I would love to see this merged! My team has used this for every apply for nearly a year. I literally cannot imagine using Terraform without this feature and I really think others would enjoy it too!

[apparentlymart]

Hi @glasser,

I'm just now taking a look over the things marked as "breaking change" that we might be able to land before the next major release.

I agree that this feature is a good idea. As you said, it seems like what's left to figure out is exactly what the right UX is.

I feel myself leaning towards changing the default to be the interactive confirmation, and having an option -auto-approve that does the current behavior, and making -input=false produce an error unless it's also combined with -auto-approve, to ensure that non-interactive Terraform runs don't get "stuck" waiting for confirmation.

It would be nice also to change the terraform destroy flow to behave in a similar way. Currently it prompts for you to confirm but gives no information on what exactly will happen if you say "yes", but it could potentially behave like an apply with no arguments, displaying the destroy plan and asking the user to confirm it.

The main downside of this is requiring updates for anyone who has Terraform wrapped in automation, though I expect that most people doing that are automating the explicit plan -out=..., apply workflow rather than the one-shot apply. So I think we could get away with this in a major release as long as we provide clear docs on how to update such automation.

Given the BC impact this has, I'm not going to try to make a decision unilaterally here 😀 so I'm curious to hear what @mitchellh thinks about the above, per his earlier comments.

[jen20]

@apparentlymart The destroy workflow is one that I meant to look at for a long time - it prompts you for confirmation of destruction before accepting variable input, making it very easy to destroy the wrong thing. I guess changing that behaviour wouldn't be a breaking change necessarily (short of people doing weird things), but unifying that problem with this issue and showing a plan before destruction after variable input would be a good idea.

[glasser]

Thanks for taking a look! I don't have any strong feelings about the best UI — I agree that prompting is a good default but I'm definitely not qualified to speak to BC concerns on this project. I'm happy to change any of the wording, flags, etc, if you let me know what design is acceptable!

Re destroy, I think we would want to move the current prompt into backend_apply.go, since the plan isn't available in that case.

I've updated the PR to resolve conflicts again.

[mitchellh]

@apparentlymart I like this line of thinking.

On the BC side: I recommend we introduce it as a non-default in 0.10 and put deprecation warning (yellow text) that this will change in 0.11 so the user should specify -confirm-plan (default false but will default true in the future) or a plan file itself.

There is likely a lot of automation out there that relies on terraform apply without a plan (for worse...). We introduced a major workflow change with 0.9 and terraform init and I don't want to make the same mistake again without a smoother warning period.

Alternately, we could introduce the deprecation warning in 0.9.x as long as there is sufficient time prior to 0.10 and changing the default behavior. Regardless, I think we should warn users for some time.

[apparentlymart]

Cool. That makes sense to me.

I personally prefer the option name -auto-approve, which would default true in this release and then switch to false later, since I think in the long run that makes the most sense. So then people wanting to use this immediately, before waiting for the deprecation cycle to end, would say -auto-approve=false.

Feeling a little weird about a yellow deprecation warning though, since that would be something right in the main user flow but yet it's not really something that requires any immediate action... I don't think we'd say e.g. "WARNING: You should use -auto-approve=false because that will be the default in future", since that would be rather baffling to people who aren't using Terraform wrapped in automation.

Do we have some other, less-mainline channel by which we can let people know what we have planned? I'm thinking some combination of documenting the best-practices for running Terraform in automation (which I've had on my to-do list forever) and including something in the release notes to explicitly call out that we plan to break this in a future release.

[mitchellh]

@apparentlymart We could potentially make the release somehow force a terraform init and show the warning once during init. It might be interesting to build a mechanism to force a terraform init (without reconfiguring backends and such) as a way for us to highlight backwards incompatibilities between releases...

Going off topic but just thinking off the top of my head.

I agree that showing a warning on every apply is not ideal.

[glasser]

Another UX question: when -auto-approve is true (either by default in the next release or explicitly later), do we want to display the plan anyway? My current PR only displays the plan if it is going to prompt, but maybe it's interesting always.

…

On Thu, May 25, 2017 at 2:08 PM, Mitchell Hashimoto < ***@***.***> wrote: @apparentlymart <https://github.com/apparentlymart> We could potentially make the release somehow force a terraform init and show the warning once during init. It might be interesting to build a mechanism to force a terraform init (without reconfiguring backends and such) as a way for us to highlight backwards incompatibilities between releases... Going off topic but just thinking off the top of my head. I agree that showing a warning on every apply is not ideal. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#7251 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABBVOE1F8aEs50-qtv1YF_KdTxhZLHxks5r9e2_gaJpZM4I6RgG> .

[apparentlymart]

That's a great thought, @glasser! I've found myself several times wishing I could see what the plan looked like after I ran terraform apply (usually for local dev, rather than for real use, admittedly)... I suppose it might make the output a bit overwhelming since the apply log is already quite long for non-trival configuration. I think for the first pass I'd lean towards showing the plan only when we prompt for approval, and then we can experiment with the alternative as a separate change.

@mitchellh as of 0.10 it will be required to run terraform init to get the provider plugins initialized, so that sounds like a very plausible plan... just need to think a bit about what exactly we want the message to say, so it's not scary for new users that haven't even run any other commands yet.

[glasser]

I'm not really sure if the ball is in my court now to implement an approved design or there is still some thinking happening (eg around yellow warnings and such).

[apparentlymart]

@glasser I think these little UX touches under discussion should be easy to do, so I don't think there's any reason to block on the details of that discussion if you were otherwise ready and willing to work on this.

[apparentlymart]

On Friday I was working on an initial draft of an upgrade guide for 0.10, so ended up writing down some info for people to use when updating their wrapper scripts.

Having reflected on this for a while since my last comment, I'm of the opinion that we don't really need to put anything special in the mainline UX to warn about the deprecation cycle here, since the target audience of the message is wrapper script maintainers rather than direct users.

Instead, I think we should plan to include info on this in the upgrade guide, draw attention to it in the changelog and release notification for 0.10, and reach out directly to maintainers of known open source wrapper scripts to make sure they see it and can get their tools upgraded in time for the default to change in a future major release.

When we change the default later, we can detect when Terraform is running in a non-interactive environment (-input=false, stdin is closed, stdin isn't a pty) and produce a good error message to help any stragglers get their home-grown wrappers updated to include -auto-approve=true when running terraform apply and thus restore the previous behavior.

[glasser]

Updated to change the flag name to -auto-approve=true and to print the plan for destroy as well.

I haven't tested this as well as the previous version since the custom build I use for our infrastructure isn't updated for the other 0.10 changes yet.

[glasser]

Do I need to do anything to get this to move forward? Didn't @apparentlymart already add a reference to this to the docs?

[apparentlymart]

Hi @glasser,

As far as I'm concerned the ball here is in our court right now... just got a list of things to work through, and this is on it, but didn't get there yet. Sorry for the delay.

[apparentlymart]

This was merged in c487c0b.

Thanks for this, @glasser! This new workflow is really nice, and I look forward to it becoming the default in future.

[ghost]

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.