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

Include additional proposed change information for certain kinds of planning errors #34312

Merged
merged 2 commits into from Nov 29, 2023

Conversation

apparentlymart
Copy link
Member

@apparentlymart apparentlymart commented Nov 28, 2023

Some time ago we ruled that Terraform Core would make a best effort to return a partial plan describing the subset of actions that were successfully proposed before encountering an error, which Terraform CLI and Terraform Cloud then rely on to present some additional context to support the associated error messages.

This change aims to improve that effort by making a distinction between the failure of the planning operation itself vs. something else in the configuration ruling that the successfully-created plan is unacceptable for some separate reason.

In that case, it's helpful to still include that proposed change in the plan, because the planning step itself succeeded and these problems are in a sense "between" the planning operations, blocking any downstream work from starting.

In particular, this change arranges for a failed prevent_destroy check or a failed postcondition check to still include the planning result that it was checked against, which then allows the UI the option of displaying that planned action alongside the error describing why it was unacceptable.


The CLI layer already supports this from our earlier work in this area, and so no changes are required there. The following is an example result from a prevent_destroy violation with these changes in place:

Terraform used the selected providers to generate the following execution plan.
Resource actions are indicated with the following symbols:
-/+ destroy and then create replacement

Terraform planned the following actions, but then encountered a problem:

  # null_resource.b must be replaced
-/+ resource "null_resource" "b" {
      ~ id       = "4627279270654855411" -> (known after apply)
      ~ triggers = { # forces replacement
          ~ "foo" = "bar" -> "baz"
        }
    }

Plan: 1 to add, 0 to change, 1 to destroy.
╷
│ Error: Instance cannot be destroyed
│ 
│   on prevent-destroy-error.tf line 7:
│    7: resource "null_resource" "b" {
│ 
│ Resource null_resource.b has lifecycle.prevent_destroy set, but the plan
│ calls for this resource to be destroyed. To avoid this error and continue
│ with the plan, either disable lifecycle.prevent_destroy or reduce the scope
│ of the plan using the -target option.
╵

Combining the error message with the proposed change first explains why this resource instance was proposed to be replaced, and then explains that it's invalid to do so. This presentation could potentially be improved in future to connect these two parts of the output more strongly, but that'll be for a future PR (focused at the CLI layer, rather than the Core layer) if so.


Closes #34305 and closes #30271.

…stics

This method was returning diagnostics wrapped inside an error, and all
of its callers were then immediately unwrapping it and using the
diagnostics inside anyway.

To make that clearer, we'll just return diagnostics directly.

(It's likely that this was originally written as a function that returned
an error before we even _had_ the concept of diagnostics, and over time
we've gradually migrated all of the callers to be ready to accept
diagnostics but neglected to actually update the function itself.)
Some time ago we ruled that Terraform Core would make a best effort to
return a partial plan describing the subset of actions that were
successfully proposed before encountering an error, which Terraform CLI
and Terraform Cloud then rely on to present some additional context to
support the associated error messages.

This change aims to improve that effort by making a distinction between
the failure of the planning operation itself vs. something else in the
configuration ruling that the successfully-created plan is unacceptable
for some separate reason.

In that case, it's helpful to still include that proposed change in the
plan, because the planning step itself succeeded and these problems are in
a sense "between" the planning operations, blocking any downstream work
from starting.

In particular, this change arranges for a failed prevent_destroy check or
a failed postcondition check to still include the planning result that
they were checked against, which then allows the UI the option of
displaying that planned action alongside the error describing why it was
unacceptable.

This doesn't include any Terraform CLI UI changes, but the UI layer is
already built to show partial plans returned alongside errors and so as
of this change the additional planned changes are already included. It'll
be up to future maintainers of Terraform CLI to decide whether and how
to refine that output, but the existing behavior is sufficient for now.
@apparentlymart apparentlymart merged commit 94b3242 into main Nov 29, 2023
6 checks passed
@apparentlymart apparentlymart deleted the f-prevent-destroy-better-error branch November 29, 2023 18:08
Copy link

Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch.

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 contributions.
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 Dec 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.