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.

Sign up for GitHub

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

terraform: Plans can be "complete" and "applyable" #34642

Merged
merged 1 commit into from
Feb 9, 2024
Merged

terraform: Plans can be "complete" and "applyable" #34642

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion internal/backend/local/backend_apply.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ func (b *Local) opApply(
return
}

trivialPlan := !plan.CanApply()
trivialPlan := !plan.Applyable
hasUI := op.UIOut != nil && op.UIIn != nil
mustConfirm := hasUI && !op.AutoApprove && !trivialPlan
op.View.Plan(plan, schemas)
Expand Down
2 changes: 1 addition & 1 deletion internal/backend/local/backend_plan.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ func (b *Local) opPlan(
}

// Record whether this plan includes any side-effects that could be applied.
runningOp.PlanEmpty = !plan.CanApply()
runningOp.PlanEmpty = !plan.Applyable

// Save the plan to disk
if path := op.PlanOutPath; path != "" {
Expand Down
6 changes: 6 additions & 0 deletions internal/command/command_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,12 @@ func testPlan(t *testing.T) *plans.Plan {
Config: backendConfigRaw,
},
Changes: plans.NewChanges(),

// We'll default to the fake plan being both applyable and complete,
// since that's what most tests expect. Tests can override these
// back to false again afterwards if they need to.
Applyable: true,
Complete: true,
}
}

Expand Down
4 changes: 4 additions & 0 deletions internal/command/jsonplan/plan.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,8 @@ type plan struct {
RelevantAttributes []ResourceAttr `json:"relevant_attributes,omitempty"`
Checks json.RawMessage `json:"checks,omitempty"`
Timestamp string `json:"timestamp,omitempty"`
Applyable bool `json:"applyable"`
Complete bool `json:"complete"`
Errored bool `json:"errored"`
}

Expand Down Expand Up @@ -225,6 +227,8 @@ func Marshal(
output := newPlan()
output.TerraformVersion = version.String()
output.Timestamp = p.Timestamp.Format(time.RFC3339)
output.Applyable = p.Applyable
output.Complete = p.Complete
output.Errored = p.Errored

err := output.marshalPlanVariables(p.VariableValues, config.Module.Variables)
Expand Down
2 changes: 2 additions & 0 deletions internal/command/show_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1207,6 +1207,8 @@ type plan struct {
OutputChanges map[string]interface{} `json:"output_changes,omitempty"`
PriorState priorState `json:"prior_state,omitempty"`
Config map[string]interface{} `json:"configuration,omitempty"`
Applyable bool `json:"applyable"`
Complete bool `json:"complete"`
Errored bool `json:"errored"`
}

Expand Down
2 changes: 2 additions & 0 deletions internal/command/testdata/show-json-sensitive/output.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
{
"format_version": "1.0",
"applyable": true,
"complete": true,
"variables": {
"test_var": {
"value": "bar"
Expand Down
2 changes: 2 additions & 0 deletions internal/command/testdata/show-json/basic-create/output.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
{
"format_version": "1.0",
"applyable": true,
"complete": true,
"variables": {
"test_var": {
"value": "bar"
Expand Down
2 changes: 2 additions & 0 deletions internal/command/testdata/show-json/basic-delete/output.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
{
"format_version": "1.0",
"applyable": true,
"complete": true,
"variables": {
"test_var": {
"value": "bar"
Expand Down
2 changes: 2 additions & 0 deletions internal/command/testdata/show-json/basic-update/output.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
{
"format_version": "1.0",
"applyable": false,
"complete": true,
"variables": {
"test_var": {
"value": "bar"
Expand Down
2 changes: 2 additions & 0 deletions internal/command/testdata/show-json/conditions/output-refresh-only.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
{
"format_version": "1.2",
"terraform_version": "1.8.0-dev",
"applyable": false,
"complete": true,
"variables": {
"ami": {
"value": "bad-ami"
Expand Down
2 changes: 2 additions & 0 deletions internal/command/testdata/show-json/conditions/output.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
{
"format_version": "1.1",
"terraform_version": "1.2.0-dev",
"applyable": true,
"complete": true,
"variables": {
"ami": {
"value": "ami-test"
Expand Down
2 changes: 2 additions & 0 deletions internal/command/testdata/show-json/drift/output.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
{
"format_version": "1.0",
"applyable": true,
"complete": true,
"planned_values": {
"root_module": {
"resources": [
Expand Down
2 changes: 2 additions & 0 deletions internal/command/testdata/show-json/module-depends-on/output.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
{
"format_version": "1.0",
"terraform_version": "0.13.1-dev",
"applyable": true,
"complete": true,
"planned_values": {
"root_module": {
"resources": [
Expand Down
2 changes: 2 additions & 0 deletions internal/command/testdata/show-json/modules/output.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
{
"format_version": "1.0",
"applyable": true,
"complete": true,
"planned_values": {
"outputs": {
"test": {
Expand Down
2 changes: 2 additions & 0 deletions internal/command/testdata/show-json/moved-drift/output.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
{
"format_version": "1.0",
"applyable": true,
"complete": true,
"planned_values": {
"root_module": {
"resources": [
Expand Down
2 changes: 2 additions & 0 deletions internal/command/testdata/show-json/moved/output.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
{
"format_version": "1.0",
"applyable": true,
"complete": true,
"planned_values": {
"root_module": {
"resources": [
Expand Down
2 changes: 2 additions & 0 deletions internal/command/testdata/show-json/multi-resource-update/output.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
{
"format_version": "1.0",
"terraform_version": "0.13.0",
"applyable": true,
"complete": true,
"variables": {
"test_var": {
"value": "bar"
Expand Down
2 changes: 2 additions & 0 deletions internal/command/testdata/show-json/nested-modules/output.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
{
"format_version": "1.0",
"applyable": true,
"complete": true,
"planned_values": {
"root_module": {
"child_modules": [
Expand Down
2 changes: 2 additions & 0 deletions internal/command/testdata/show-json/plan-error/output.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
{
"format_version": "1.2",
"applyable": false,
"complete": false,
"planned_values": {
"root_module": {}
},
Expand Down
2 changes: 2 additions & 0 deletions internal/command/testdata/show-json/provider-aliasing-conflict/output.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
{
"format_version": "1.0",
"terraform_version": "1.1.0-dev",
"applyable": true,
"complete": true,
"planned_values": {
"root_module": {
"resources": [
Expand Down
2 changes: 2 additions & 0 deletions internal/command/testdata/show-json/provider-aliasing-default/output.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
{
"format_version": "1.0",
"terraform_version": "1.1.0-dev",
"applyable": true,
"complete": true,
"planned_values": {
"root_module": {
"resources": [
Expand Down
2 changes: 2 additions & 0 deletions internal/command/testdata/show-json/provider-aliasing/output.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
{
"format_version": "1.0",
"terraform_version": "1.1.0-dev",
"applyable": true,
"complete": true,
"planned_values": {
"root_module": {
"resources": [
Expand Down
2 changes: 2 additions & 0 deletions internal/command/testdata/show-json/provider-version-no-config/output.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
{
"format_version": "1.0",
"applyable": true,
"complete": true,
"variables": {
"test_var": {
"value": "bar"
Expand Down
2 changes: 2 additions & 0 deletions internal/command/testdata/show-json/provider-version/output.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
{
"format_version": "1.0",
"applyable": true,
"complete": true,
"variables": {
"test_var": {
"value": "bar"
Expand Down
2 changes: 2 additions & 0 deletions internal/command/testdata/show-json/requires-replace/output.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
{
"format_version": "1.0",
"applyable": true,
"complete": true,
"planned_values": {
"root_module": {
"resources": [
Expand Down
2 changes: 2 additions & 0 deletions internal/command/testdata/show-json/sensitive-values/output.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
{
"format_version": "1.0",
"applyable": true,
"complete": true,
"variables": {
"test_var": {
"value": "boop"
Expand Down
2 changes: 2 additions & 0 deletions internal/command/testdata/show-json/unknown-output/output.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
{
"format_version": "1.1",
"terraform_version": "1.3.0-dev",
"applyable": true,
"complete": true,
"planned_values": {
"outputs": {
"bar": {
Expand Down
10 changes: 7 additions & 3 deletions internal/command/views/operation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,11 +116,15 @@ func (v *OperationHuman) Plan(plan *plans.Plan, schemas *terraform.Schemas) {

// Side load some data that we can't extract from the JSON plan.
var opts []plans.Quality
if !plan.CanApply() {
opts = append(opts, plans.NoChanges)
}
if plan.Errored {
opts = append(opts, plans.Errored)
} else if !plan.Applyable {
// FIXME: There might be other reasons for "non-applyable" in future,
// so maybe we should check plan.Changes.IsEmpty here and use a more
// generic fallback message if the plan doesn't seem to be empty.
// There are currently no other cases though, so we'll keep it
// simple for now.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for my edification: aside from errors (handled above), what are some other potential reasons that a non-empty plan could be non-applyable?

Copy link
Contributor Author

@apparentlymart apparentlymart Feb 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the moment, there are none, which is why I didn't go to the trouble of complicating this. But since the idea of this was to give us the flexibility to adjust the definition of "applyable" in future -- for reasons we have not yet imagined -- this comment is intended as an aid to the hypothetical future person who is trying to do that and noticing that Terraform CLI is returning a confusing error message in whatever new situation they've introduced.

I'm making a tradeoff here that we've made before but I wasn't very explicit about: we typically assume it's okay for stuff in this codebase to make possibly-short-sighted assumptions about Terraform Core's abstractions because we can always change both e.g. Terraform CLI and Terraform Core together in a single commit if we need to. Whereas subsystems outside of this codebase, like Terraform Cloud, ought to follow the abstraction more closely since we typically not guarantee that they'll upgrade in lockstep with Terraform CLI/Core.

So when Terraform Cloud is relying on this, I would expect it to:

  • Use this flag alone to decide whether or not to show the "approve plan" button, or the automated equivalents thereof.
  • If the plan is also marked as errored then show the errors as an explanation for why the plan is not applyable.
  • If the plan is not errored, and all of the changes are no-op changes, then show "no changes" as the explanation for why the plan is not applyable.
  • If neither of the above conditions applies, show a generic message saying the plan is not applyable, which then serve as a stopgap for any situation where Terraform Core upgrades sooner than Terraform Cloud does, such as when a customer is running an older release of Terraform Enterprise.

The above logic is what the comment you were replying to is gesturing at. If I were writing Terraform CLI fresh today I'd have made it use a similar decision tree as the above, but because it lives in the same codebase as Terraform Core we can more reasonably assume that we'll update Terraform CLI at the same time as adding any hypothetical third case for a plan not being applyable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to merge this now just because it seems like if we want to do anything in response to this comment I could do so in a separate PR, and I have some other commits building on this one that I'd like to try to finish up today. Please let me know if there's something you'd like to change about this, and I'd be happy to do it in a followup PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, not at all! Thanks for the clear explanation.

opts = append(opts, plans.NoChanges)
}

renderer.RenderHumanPlan(jplan, plan.UIMode, opts...)
Expand Down
4 changes: 3 additions & 1 deletion internal/command/views/plan_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,9 @@ func testPlan(t *testing.T) *plans.Plan {
})

return &plans.Plan{
Changes: changes,
Changes: changes,
Applyable: true,
Complete: true,
}
}

Expand Down
5 changes: 2 additions & 3 deletions internal/command/views/show.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,11 +88,10 @@ func (v *ShowHuman) Display(config *configs.Config, plan *plans.Plan, planJSON *
}

var opts []plans.Quality
if !plan.CanApply() {
opts = append(opts, plans.NoChanges)
}
if plan.Errored {
opts = append(opts, plans.Errored)
} else if !plan.Applyable {
opts = append(opts, plans.NoChanges)
}

renderer.RenderHumanPlan(jplan, plan.UIMode, opts...)
Expand Down
5 changes: 2 additions & 3 deletions internal/command/views/test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,11 +230,10 @@ func (t *TestHuman) Run(run *moduletest.Run, file *moduletest.File, progress mod
}

var opts []plans.Quality
if !run.Verbose.Plan.CanApply() {
opts = append(opts, plans.NoChanges)
}
if run.Verbose.Plan.Errored {
opts = append(opts, plans.Errored)
} else if !run.Verbose.Plan.Applyable {
opts = append(opts, plans.NoChanges)
}

renderer.RenderHumanPlan(plan, run.Verbose.Plan.UIMode, opts...)
Expand Down
72 changes: 25 additions & 47 deletions internal/plans/plan.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,31 @@ type Plan struct {
ForceReplaceAddrs []addrs.AbsResourceInstance
Backend Backend

// Complete is true if Terraform considers this to be a "complete" plan,
// which is to say that it includes a planned action (even if no-op)
// for every resource instance object that was mentioned across both
// the desired state and prior state.
//
// If Complete is false then the plan might still be applyable (check
// [Plan.Applyable]) but after applying it the operator should be reminded
// to plan and apply again to hopefully make more progress towards
// convergence.
//
// For an incomplete plan, other fields of this type may give more context
// about why the plan is incomplete, which a UI layer could present to
// the user as part of a warning that the plan is incomplete.
Complete bool

// Applyable is true if both Terraform was able to create a plan
// successfully and if the plan calls for making some sort of meaningful
// change.
//
// If [Plan.Errored] is also set then that means the plan is non-applyable
// due to an error. If not then the plan was created successfully but found
// no material differences between desired and prior state, and so
// applying this plan would achieve nothing.
Applyable bool

// Errored is true if the Changes information is incomplete because
// the planning operation failed. An errored plan cannot be applied,
// but can be cautiously inspected for debugging purposes.
Expand Down Expand Up @@ -131,53 +156,6 @@ type Plan struct {
ProviderFunctionResults []providers.FunctionHash
}

// CanApply returns true if and only if the recieving plan includes content
// that would make sense to apply. If it returns false, the plan operation
// should indicate that there's nothing to do and Terraform should exit
// without prompting the user to confirm the changes.
//
// This function represents our main business logic for making the decision
// about whether a given plan represents meaningful "changes", and so its
// exact definition may change over time; the intent is just to centralize the
// rules for that rather than duplicating different versions of it at various
// locations in the UI code.
func (p *Plan) CanApply() bool {
switch {
case p.Errored:
// An errored plan can never be applied, because it is incomplete.
// Such a plan is only useful for describing the subset of actions
// planned so far in case they are useful for understanding the
// causes of the errors.
return false

case !p.Changes.Empty():
// "Empty" means that everything in the changes is a "NoOp", so if
// not empty then there's at least one non-NoOp change.
return true

case !p.PriorState.ManagedResourcesEqual(p.PrevRunState):
// If there are no changes planned but we detected some
// outside-Terraform changes while refreshing then we consider
// that applyable in isolation only if this was a refresh-only
// plan where we expect updating the state to include these
// changes was the intended goal.
//
// (We don't treat a "refresh only" plan as applyable in normal
// planning mode because historically the refresh result wasn't
// considered part of a plan at all, and so it would be
// a disruptive breaking change if refreshing alone suddenly
// became applyable in the normal case and an existing configuration
// was relying on ignore_changes in order to be convergent in spite
// of intentional out-of-band operations.)
return p.UIMode == RefreshOnlyMode

default:
// Otherwise, there are either no changes to apply or they are changes
// our cases above don't consider as worthy of applying in isolation.
return false
}
}

// ProviderAddrs returns a list of all of the provider configuration addresses
// referenced throughout the receiving plan.
//
Expand Down
Loading
Loading