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

Change -force to -auto-approve when destroying #17218

Merged

Conversation

surminus
Copy link
Contributor

Since an early version of Terraform, the destroy command has always had the -force flag to allow an auto approval of the interactive prompt. 0.11 introduced -auto-approve as default to false when using the apply command.

People often use wrappers when automating commands in Terraform, and the inconsistency between apply and destroy means that additional logic must be added to the wrappers to do similar functions. Both commands are more or less able to run with similar syntax, and I could not find a reason for the inconsistency between the two commands despite the functionality being the same.

This commit updates the command in apply to use the -force flag making working with the Terraform CLI a more consistent experience.

I searched PRs and issues and could not find any context on why this was implemented the way that it was, or if there was a specific reason why the differences exist between apply and destroy. I also appreciate this is a breaking change.

@apparentlymart
Copy link
Member

Hi @surminus! Thanks for working on this.

The choice of -auto-approve here was intended to be a more specific name than "force", given that the apply command does a number of different things that could potentially be forced and we wanted to be more clear about what the flag does here. For example, there is an active proposal to add a "force-like" flag to override the prevent_destroy resource lifecycle setting, which ought to be separated due to the different level of risk.

As you noted, the -force flag on terraform destroy has been around for a long time, and we didn't catch this inconsistency when thinking about the approval feature on apply because -auto-approve was originally conceived as a way to phase this in without a breaking change, and only became the opt-out flag later. This was an oversight we became aware of only later, but didn't yet get around to fixing it... so thanks! 😀

Given the reasoning above, what do you think about making the opposite change here, so that terraform destroy uses -auto-approve instead? That way we retain the specificity while improving the consistency. We could also keep -force as a deprecated alias on the destroy command for now to give a migration path for anyone who is already wrapping/automating terraform destroy -force.

@surminus
Copy link
Contributor Author

Hi @apparentlymart! Thanks for getting back to me and providing the context on this. Appreciate ambiguity of the -force flag, so would be happy to reverse my changes onto the destroy command 🙂

Since an early version of Terraform, the `destroy` command has always
had the `-force` flag to allow an auto approval of the interactive
prompt. 0.11 introduced `-auto-approve` as default to `false` when using
the `apply` command.

The `-auto-approve` flag was introduced to reduce ambiguity of it's
function, but the `-force` flag was never updated for a destroy.

People often use wrappers when automating commands in Terraform, and the
inconsistency between `apply` and `destroy` means that additional logic
must be added to the wrappers to do similar functions. Both commands are
more or less able to run with similar syntax, and also heavily share
their code.

This commit updates the command in `destroy` to use the `-auto-approve` flag
making working with the Terraform CLI a more consistent experience.

We leave in `-force` in `destroy` for the time-being and flag it as
deprecated to ensure a safe switchover period.
@surminus
Copy link
Contributor Author

surminus commented Feb 1, 2018

@apparentlymart I've pushed a new commit with the changes - I actually had to understand the code a little bit this time rather than just a search and replace! I couldn't find a specific alias function in the flag library used for CLI options, so I added in a bit of logic that can be removed in the future instead.

@apparentlymart
Copy link
Member

Hi @surminus! Sorry for the silence here.

This is definitely on our list to review or merge, but the team is currently focused in a different area (the configuration loader) and so we've been holding off on merging this until we're able to context-switch back into "CLI land". I'm sorry we haven't been able to get to this sooner, but we'll take a look at it once we are able to take a breather from our current configuration-related projects.

@surminus
Copy link
Contributor Author

No problem, thanks for letting me know!

@Miserlou
Copy link

Miserlou commented Mar 7, 2018

Eager for this change - the asymmetry is really poor CLI design, very annoying during development.

@surminus surminus changed the title Change -auto-approve to -force when applying Change -force to -auto-approve when destroying Mar 8, 2018
@apparentlymart apparentlymart merged commit 6e1e614 into hashicorp:master Mar 9, 2018
@apparentlymart
Copy link
Member

Thanks for your patience here, @surminus!

This change looks great to me... thanks again for working on it. It'll be included in the next release.

@surminus
Copy link
Contributor Author

Great thanks @apparentlymart, pleased I could contribute!

@surminus surminus deleted the change-auto-approve-to-force branch March 12, 2018 09:20
@FernandoMiguel
Copy link

I know this may sound weird, but
Can we please not have auto approve on destroy?

Just yesterday I accidentally did a cmd+r and typed appro
And pressed enter without looking and the command there was a failed destroy --auto-approved

Had this flag existed on my current version, I would have accidentally destroyed my environment with no option to validate.

Yes, it would have been my fault for not being careful, but tooling should also prevent us from accidently executing potentially damaging actions.

Thanks

@unixorn
Copy link

unixorn commented Mar 16, 2018

It would be great if you could add something like disable-auto-approve=True in your provider stanza to disable this.

I really don't want anyone able to run something against prod with auto-approve per @FernandoMiguel 's comment above.

@paultyng
Copy link
Contributor

@FernandoMiguel @unixorn FWIW some people (including us internally) use Terraform within automation and they always need non-interactive ways of running it, so that capability is a requirement.

You could always alias terraform to a function that strips that argument off if you wanted.

@surminus
Copy link
Contributor Author

Yes, it would have been my fault for not being careful, but tooling should also prevent us from accidently executing potentially damaging actions.

The tooling does protect you from accidentally running damaging actions - you have to explicitly set the-auto-approve flag, after all.

@FernandoMiguel
Copy link

@paultyng if the flag is the same as the apply, i find it to be risky.
all i'm asking here is for sensible options that don't risk anyone job/life

I'm not opposed to automation, but I would rather have a different flag for apply and destroy, even if that is a small burden on users to remember/search

@Miserlou
Copy link

I think that would be a poor CLI API design choice.

minamijoyo added a commit to minamijoyo/terraform that referenced this pull request Mar 22, 2018
Fixes hashicorp#17671

This `auto-approve` flag was in `apply -help` on Terraform v0.11.3,
but it was deleted by mistake in hashicorp#17218.
This fix restores it.
@ghost
Copy link

ghost commented Apr 4, 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.

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

Successfully merging this pull request may close these issues.

None yet

6 participants