First stab at attr.TypeWithModifyPlan. #162
Conversation
paddycarver
added
the
types
paddycarver
force-pushed
the
paddy_attr_type_modifyplan
branch
from
466b911 to
0d33ed0
Compare
paddycarver
force-pushed
the
paddy_attr_type_modifyplan
branch
2 times, most recently
from
765b493 to
5fdd181
Compare
|
I think this is ready for review, but is rebased on top of #231 and so depends on that being reviewed/merged first. |
paddycarver
force-pushed
the
paddy_attr_type_modifyplan
branch
from
7dc5310 to
9ff49d4
Compare
|
I'll review once #231 is sorted and this change set is clearer. 👍 |
Move out of serve and into a helper function. Simplify some of the logic.
paddycarver
force-pushed
the
paddy_attr_type_modifyplan
branch
from
9ff49d4 to
69053e4
Compare
|
Rebased after merging #231. |
| "github.com/hashicorp/terraform-plugin-go/tftypes" | ||
| ) | ||
|
|
||
| func runTypePlanModifiers(ctx context.Context, state, plan tftypes.Value, schema Schema, resp *planResourceChangeResponse) (tftypes.Value, bool) { |
There was a problem hiding this comment.
It seems like the bool return value could be dropped here (or return its own diag.Diagnostics) in preference of the caller checking resp.Diagnostics.HasError() similar to other existing logic.
| // TODO: this isn't ideal | ||
| // but should it be a pathed diagnostic? | ||
| // Terraform is horribly broken at this point | ||
| // there may be nothing in the config to point to |
There was a problem hiding this comment.
Agree on the sentiment, but I still feel like we should set the expected attribute path and let Terraform CLI handle the UI details.
| return plan, false | ||
| } | ||
| } | ||
| for attrName, a := range schema.Attributes { |
There was a problem hiding this comment.
In addition to walking schema.Attributes, schema.Blocks should also be walked for their nested Blocks and Attributes to ensure those type plan modifiers are executed.
| // modifying the plan for each element in a set isn't | ||
| // supported at the moment, because there's no way to | ||
| // correlate the new value in the plan with the old | ||
| // value in the state; sets can only be reliably | ||
| // compared by the identity of the elements, and the | ||
| // identity of the element changed. | ||
| // | ||
| // as such, sets must have plan modification applied at | ||
| // the set level, because anything else doesn't really | ||
| // make much sense. | ||
| // | ||
| // providers unhappy with this can always implement the | ||
| // logic to call each element's ModifyPlan inside their | ||
| // set type's ModifyPlan, but I can't see a way to do | ||
| // it that's not rife with weird behaviors. |
There was a problem hiding this comment.
I'm worried that there are no Go documentation, diagnostics, logging, etc. breadcrumbs around this explicit omission in handling. Here's some potential ideas (not necessarily mutually exclusive):
- Add Go documentation to
attr.TypeWithPlanModification - Add a line item/comment to Consider InternalValidate equivalent #113
- Deeply continuing to traverse the set to find any potential
attr.TypeWithPlanModification, skip plan modification and output a warning log (I'm guessing preferable over warning diagnostics since it could be very noisy) - Save the current plan value for each element, perform the traversal with plan modifications, compare the two values and remove the current (now old) value if modified
| func attributeTypeModifyPlan(ctx context.Context, typ attr.Type, state, plan tftypes.Value, path *tftypes.AttributePath) (attr.Value, diag.Diagnostics) { | ||
| var diags diag.Diagnostics | ||
| if plan.IsKnown() && !plan.IsNull() { | ||
| if typ.TerraformType(ctx).Is(tftypes.Object{}) { |
There was a problem hiding this comment.
Nit: Seems this logic could be simplified via:
tfType := typ.TerraformType(ctx)
switch {
case tfType.Is(tftypes.Object{}):
plan, diags = attributeTypeModifyPlanObject(ctx, typ, state, plan, path)
case tfType.Is(tftypes.Map{}):
plan, diags = attributeTypeModifyPlanMap(ctx, typ, state, plan, path)
// ...
}
if diags.HasError() {
return plan, diags
}Unless there's some explicit reason to return a nil value instead of the prior plan value during errors.
| // ModifyPlan returns the Value that should be used in the plan. It is | ||
| // generally used to suppress diffs that do not correspond to semantic | ||
| // differences. In these cases, the `state` Value should be returned. | ||
| ModifyPlan(ctx context.Context, state, plan Value, path *tftypes.AttributePath) (Value, diag.Diagnostics) |
There was a problem hiding this comment.
Open question: Should these also have access to the configuration value?
|
|
|
@paddycarver I think we're finally at a place where we can work on this after a lot of refactoring. Do you have interest in the implementation still? |
bflad
added
waiting-response
|
There have been a lot of changes to the framework affecting this particular code, so I'm going to close this. |
|
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. |
Addresses #161.