Skip to content

Commit

Permalink
terraform: Plans can be "complete" and "applyable"
Browse files Browse the repository at this point in the history
These ideas are both already implied by some logic elsewhere in the system,
but until now we didn't have the decision logic centralized in a single
place that could therefore evolve over time without necessarily always
updating every caller together.

We'll now have the modules runtime produce its own boolean ruling about
each characteristic, which callers can rely on for the mechanical
decision-making of whether to offer the user an "approve" prompt, and
whether to remind the user after apply that it was an incomplete plan
that will probably therefore need at least one more plan/apply round to
converge.

The "Applyable" flag directly replaces the previous method Plan.CanApply,
with equivalent logic. Making this a field instead of a method means that
we can freeze it as part of a saved plan, rather than recalculating it
when we reload the plan, and we can export the field value in our export
formats like JSON while ensuring it'll always be consistent with what
Terraform is using internally.

Callers can (and should) still use other context in the plan to return
more tailored messages for specific situations they already know about
that might be useful to users, but with these flags as a baseline callers
can now just fall back to a generic presentation when encountering a
situation they don't yet understand, rather than making the wrong decision
and causing something strange to happen. That is: a lack of awareness of
a new rule will now cause just a generic message in the UI, rather than
incorrect behavior.

This commit mostly just deals with populating the flags, and then all of
the direct consequences of that on our various tests. Further changes to
actually make use of these flags elsewhere in the system will follow in
later commits, both in this repository and in other repositories.
  • Loading branch information
apparentlymart committed Feb 9, 2024
1 parent c01b4a8 commit 884e1fb
Show file tree
Hide file tree
Showing 52 changed files with 1,434 additions and 1,033 deletions.
2 changes: 1 addition & 1 deletion internal/backend/local/backend_apply.go
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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.
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
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
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
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
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

0 comments on commit 884e1fb

Please sign in to comment.