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

Jump to bottom

tfsdk: Check ProposedNewState for State differences before marking unknowns for Computed-only attributes in plans #184

Merged
merged 2 commits into from
Sep 29, 2021
Merged

tfsdk: Check ProposedNewState for State differences before marking unknowns for Computed-only attributes in plans #184

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d3167bd
tfsdk: Check ProposedNewState for State differences before marking un…
bflad Sep 28, 2021
124f5b7
fix comment
kmoe Sep 29, 2021
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
67 changes: 55 additions & 12 deletions tfsdk/serve.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -691,15 +691,57 @@ func (s *server) planResourceChange(ctx context.Context, req *tfprotov6.PlanReso
return
}

// first, mark any computed attributes that are null in the config as
// unknown in the plan, so providers have the choice to update them
// Execute any AttributePlanModifiers.
//
// do this first so that providers can override the unknown with a
// known value using any plan modifiers
// This pass is before any Computed-only attributes are marked as unknown
// to ensure any plan changes will trigger that behavior. These plan
// modifiers are run again after that marking to allow setting values
// and preventing extraneous plan differences.
//
// we only do this if there's a plan to modify; otherwise, it
// represents a resource being deleted and there's no point
if !plan.IsNull() {
// We only do this if there's a plan to modify; otherwise, it
// represents a resource being deleted and there's no point.
//
// TODO: Enabling this pass will generate the following test error:
//
// --- FAIL: TestServerPlanResourceChange/two_modifyplan_add_list_elem (0.00s)
// serve_test.go:3303: An unexpected error was encountered trying to read an attribute from the configuration. This is always an error in the provider. Please report the following to the provider developer:
//
// ElementKeyInt(1).AttributeName("name") still remains in the path: step cannot be applied to this value
//
// To fix this, (Config).GetAttribute() should return nil instead of the error.
// Reference: https://github.com/hashicorp/terraform-plugin-framework/issues/183
// Reference: https://github.com/hashicorp/terraform-plugin-framework/issues/150
// See also: https://github.com/hashicorp/terraform-plugin-framework/pull/167

// Execute any resource-level ModifyPlan method.
//
// This pass is before any Computed-only attributes are marked as unknown
// to ensure any plan changes will trigger that behavior. These plan
// modifiers be run again after that marking to allow setting values and
// preventing extraneous plan differences.
//
// TODO: Enabling this pass will generate the following test error:
//
// --- FAIL: TestServerPlanResourceChange/two_modifyplan_add_list_elem (0.00s)
// serve_test.go:3303: An unexpected error was encountered trying to read an attribute from the configuration. This is always an error in the provider. Please report the following to the provider developer:
//
// ElementKeyInt(1).AttributeName("name") still remains in the path: step cannot be applied to this value
//
// To fix this, (Config).GetAttribute() should return nil instead of the error.
// Reference: https://github.com/hashicorp/terraform-plugin-framework/issues/183
// Reference: https://github.com/hashicorp/terraform-plugin-framework/issues/150
// See also: https://github.com/hashicorp/terraform-plugin-framework/pull/167

// After ensuring there are proposed changes, mark any computed attributes
// that are null in the config as unknown in the plan, so providers have
// the choice to update them.
//
// Later attribute and resource plan modifier passes can override the
// unknown with a known value using any plan modifiers.
//
// We only do this if there's a plan to modify; otherwise, it
// represents a resource being deleted and there's no point.
if !plan.IsNull() && !plan.Equal(state) {
modifiedPlan, err := tftypes.Transform(plan, markComputedNilsAsUnknown(ctx, config, resourceSchema))
if err != nil {
resp.Diagnostics.AddError(
Expand All @@ -711,8 +753,8 @@ func (s *server) planResourceChange(ctx context.Context, req *tfprotov6.PlanReso
plan = modifiedPlan
}

// next, execute any AttributePlanModifiers as long as there's a plan
// to modify.
// Execute any AttributePlanModifiers again. This allows overwriting
// any unknown values.
//
// We only do this if there's a plan to modify; otherwise, it
// represents a resource being deleted and there's no point.
Expand Down Expand Up @@ -766,17 +808,18 @@ func (s *server) planResourceChange(ctx context.Context, req *tfprotov6.PlanReso
}

resourceSchema.modifyAttributePlans(ctx, modifySchemaPlanReq, &modifySchemaPlanResp)
resp.RequiresReplace = modifySchemaPlanResp.RequiresReplace
resp.RequiresReplace = append(resp.RequiresReplace, modifySchemaPlanResp.RequiresReplace...)
plan = modifySchemaPlanResp.Plan.Raw
resp.Diagnostics = modifySchemaPlanResp.Diagnostics
if resp.Diagnostics.HasError() {
return
}
}

// third, execute any resource-level ModifyPlan method
// Execute any resource-level ModifyPlan method again. This allows
// overwriting any unknown values.
//
// we do this regardless of whether the plan is null or not, because we
// We do this regardless of whether the plan is null or not, because we
// want resources to be able to return diagnostics when planning to
// delete resources, e.g. to inform practitioners that the resource
// _can't_ be deleted in the API and will just be removed from
Expand Down
14 changes: 12 additions & 2 deletions tfsdk/serve_resource_attribute_plan_modifiers_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,10 @@ func (rt testServeResourceTypeAttributePlanModifiers) GetSchema(_ context.Contex
Type: types.StringType,
PlanModifiers: []AttributePlanModifier{testAttrDefaultValueModifier{}},
},
"computed_string_no_modifiers": {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Additional attribute on this resource to allow us to further verify plan unknown marking, since this resource has attribute plan modifiers and allowed me to catch #183 for the future. It does not have to be part of this pull request though. 😄

Computed: true,
Type: types.StringType,
},
},
}, nil
}
Expand All @@ -114,6 +118,11 @@ var testServeResourceTypeAttributePlanModifiersSchema = &tfprotov6.Schema{
Version: 1,
Block: &tfprotov6.SchemaBlock{
Attributes: []*tfprotov6.SchemaAttribute{
{
Name: "computed_string_no_modifiers",
Computed: true,
Type: tftypes.String,
},
{
Name: "name",
Required: true,
Expand Down Expand Up @@ -173,8 +182,9 @@ var testServeResourceTypeAttributePlanModifiersSchema = &tfprotov6.Schema{

var testServeResourceTypeAttributePlanModifiersType = tftypes.Object{
AttributeTypes: map[string]tftypes.Type{
"name": tftypes.String,
"size": tftypes.Number,
"computed_string_no_modifiers": tftypes.String,
"name": tftypes.String,
"size": tftypes.Number,
"scratch_disk": tftypes.Object{
AttributeTypes: map[string]tftypes.Type{
"id": tftypes.String,
Expand Down
Loading