Skip to content

Commit

Permalink
Complicate our RequiresReplace(If) logic.
Browse files Browse the repository at this point in the history
Fixes #187, supercedes #203.

This changes the logic on our RequiresReplace function in the following
ways:

* if the attribute being modified has not changed, RequiresReplace isn't
  changed. This prevents the resource from being destroyed and recreated
  when _any_ attribute changes, limiting it only to the attribute with
  RequiresReplace set on it.
* if the attribute being modified is computed and is null in the config,
  RequiresReplace isn't changed. This prevents the resource from being
  destroyed and recreated when the provider returns an unknown value in
  the plan, or even a new, provider-controlled value, because it's
  unlikely the practitioners expect their resource to be destroyed and
  recreated when an attribute out of their control changes. This has the
  unfortunate side-effect that practitioners removing an
  Optional+Computed field from their config will not prompt the default
  RequiresReplace behavior, but providers can specify their own, special
  plan modifier that has a better understanding of practitioner intent
  in that case.
* if the resource is being created or destroyed, RequiresReplace isn't
  changed. This prevents the resource from being marked for destruction
  and recreation when it's just now being created or destroyed, which
  would be nonsensical.

RequiresReplaceIf gets all these changes and an additional change that
it will only set RequiresReplace to true, never to false, as its Modify
method's GoDoc indicated. This means that RequiresReplaceIf functions
can be chained, and that RequiresReplaceIf won't overwrite previous plan
modifiers' determinations on whether or not the resource should be
destroyed and recreated.

Tests were added for RequiresReplace and RequiresReplaceIf to test these
invariants.
  • Loading branch information
paddycarver committed Oct 28, 2021
1 parent 69ccc65 commit 486e1dc
Show file tree
Hide file tree
Showing 4 changed files with 795 additions and 14 deletions.
101 changes: 100 additions & 1 deletion tfsdk/attribute_plan_modification.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package tfsdk

import (
"context"
"fmt"

"github.com/hashicorp/terraform-plugin-framework/attr"
"github.com/hashicorp/terraform-plugin-framework/diag"
Expand Down Expand Up @@ -64,6 +65,52 @@ type RequiresReplaceModifier struct{}

// Modify sets RequiresReplace on the response to true.
func (r RequiresReplaceModifier) Modify(ctx context.Context, req ModifyAttributePlanRequest, resp *ModifyAttributePlanResponse) {
if req.AttributeConfig == nil || req.AttributePlan == nil || req.AttributeState == nil {
// shouldn't happen, but let's not panic if it does
return
}

if req.State.Raw.IsNull() {
// if we're creating the resource, no need to delete and
// recreate it
return
}

if req.Plan.Raw.IsNull() {
// if we're deleting the resource, no need to delete and
// recreate it
return
}

attrSchema, err := req.State.Schema.AttributeAtPath(req.AttributePath)
if err != nil {
resp.Diagnostics.AddAttributeError(req.AttributePath,
"Error finding attribute schema",
fmt.Sprintf("An unexpected error was encountered retrieving the schema for this attribute. This is always a bug in the provider.\n\nError: %s", err),
)
return
}

configRaw, err := req.AttributeConfig.ToTerraformValue(ctx)
if err != nil {
resp.Diagnostics.AddAttributeError(req.AttributePath,
"Error converting config value",
fmt.Sprintf("An unexpected error was encountered converting a %s to its equivalent Terraform representation. This is always a bug in the provider.\n\nError: %s", req.AttributeConfig.Type(ctx), err),
)
return
}
if configRaw == nil && attrSchema.Computed {
// if the config is null and the attribute is computed, this
// could be an out of band change, don't require replace
return
}

if req.AttributePlan.Equal(req.AttributeState) {
// if the plan and the state are in agreement, this attribute
// isn't changing, don't require replace
return
}

resp.RequiresReplace = true
}

Expand Down Expand Up @@ -103,9 +150,61 @@ type RequiresReplaceIfModifier struct {
// Modify sets RequiresReplace on the response to true if the conditional
// RequiresReplaceIfFunc returns true.
func (r RequiresReplaceIfModifier) Modify(ctx context.Context, req ModifyAttributePlanRequest, resp *ModifyAttributePlanResponse) {
if req.AttributeConfig == nil || req.AttributePlan == nil || req.AttributeState == nil {
// shouldn't happen, but let's not panic if it does
return
}

if req.State.Raw.IsNull() {
// if we're creating the resource, no need to delete and
// recreate it
return
}

if req.Plan.Raw.IsNull() {
// if we're deleting the resource, no need to delete and
// recreate it
return
}

attrSchema, err := req.State.Schema.AttributeAtPath(req.AttributePath)
if err != nil {
resp.Diagnostics.AddAttributeError(req.AttributePath,
"Error finding attribute schema",
fmt.Sprintf("An unexpected error was encountered retrieving the schema for this attribute. This is always a bug in the provider.\n\nError: %s", err),
)
return
}

configRaw, err := req.AttributeConfig.ToTerraformValue(ctx)
if err != nil {
resp.Diagnostics.AddAttributeError(req.AttributePath,
"Error converting config value",
fmt.Sprintf("An unexpected error was encountered converting a %s to its equivalent Terraform representation. This is always a bug in the provider.\n\nError: %s", req.AttributeConfig.Type(ctx), err),
)
return
}
if configRaw == nil && attrSchema.Computed {
// if the config is null and the attribute is computed, this
// could be an out of band change, don't require replace
return
}

if req.AttributePlan.Equal(req.AttributeState) {
// if the plan and the state are in agreement, this attribute
// isn't changing, don't require replace
return
}

res, diags := r.f(ctx, req.AttributeState, req.AttributeConfig, req.AttributePath)
resp.Diagnostics.Append(diags...)
resp.RequiresReplace = res

// if the function says to require replacing, we require replacing
// if the function says not to, we don't change the value that other
// plan modifiers may have set
if res {
resp.RequiresReplace = true
}
}

// Description returns a human-readable description of the plan modifier.
Expand Down
Loading

0 comments on commit 486e1dc

Please sign in to comment.