chore: Uses NewUnknownReplacements meant to replace schemafunc.CopyUnknowns logic and schemafunc.NewAttributeChanges
#3192
Conversation
…nknowns` logic and `schemafunc.NewAttributeChanges`
…g the new `ReplaceUnknown` signature
EspenAlbert
changed the title
schemafunc.CopyUnknowns logic and schemafunc.NewAttributeChangesschemafunc.CopyUnknowns logic and schemafunc.NewAttributeChanges
| } | ||
|
|
||
| // ModifyPlan is the only method overridden in this test. | ||
| func (r *rs) ModifyPlan(ctx context.Context, req resource.ModifyPlanRequest, resp *resource.ModifyPlanResponse) { |
There was a problem hiding this comment.
as discussed offline, not sure about this test as it's very coupled to the implementation
There was a problem hiding this comment.
Curious to hear what others think. Please react with 👍 if you find it useful or 👎 if it is just noisy/confusing
… in AncestorPathWithIndex
| } | ||
|
|
||
| // AttributePath similar to the internal function in TPF, but simpler interface as argument and less logging | ||
| func AttributePath(ctx context.Context, tfType *tftypes.AttributePath, schema TPFSchema) (path.Path, diag.Diagnostics) { |
There was a problem hiding this comment.
There was a problem hiding this comment.
I haven't gone deep, but it smells a lot we're copying internal methods. Also, what is the impact on the policy?
There was a problem hiding this comment.
Did we get any additional information here?
There was a problem hiding this comment.
Good point. Waiting on reply.
|
|
||
| func readSrcStructValue[T any](ctx context.Context, src conversion.TPFSrc, p path.Path) *T { | ||
| var obj types.Object | ||
| if localDiags := src.GetAttribute(ctx, p, &obj); localDiags.HasError() { |
There was a problem hiding this comment.
localDiags not used/passed anywhere? Why is it handled differently than in readSrcStructValues?
There was a problem hiding this comment.
Thanks for spotting this.
I'm not 100% sure all the cases where this might happen.
I think it is used in readSrcStructValues to use the conversion.TFModelList method, but I agree we should be consistent.
I'll investigate and update
There was a problem hiding this comment.
Updated, choosing to remove the diags argument.
These errors should be spotted before production, but just to be sure, we log them instead of adding to "root" diagnostics. This avoids any risk of the user having a "stuck" resource due to plan modifier errors.
…d of global failure
…cestor path behavior
|
This PR has gone 7 days without any activity and meets the project’s definition of "stale". This will be auto-closed if there is no new activity over the next 7 days. If the issue is still relevant and active, you can simply comment with a "bump" to keep it open, or add the label "not_stale". Thanks for keeping our repository healthy! |
Description
Adds NewUnknownReplacements meant to replace
schemafunc.CopyUnknownslogic andschemafunc.NewAttributeChanges.The new common logic replaces the need for reflect and instead uses Terraform's
req.State.Raw.Diff(req.Config.Raw)to find changes.It also comes with a simplified API where each individual field can be defined instead of having to go top-down.
Note: fields can be defined at any level, for example electable_specs or instance_size are both valid.
See #3218, #3223, #3236 for how the usage is for
advanced_clusterSee also guide on adding a new attribute here
Link to any related issue(s): CLOUDP-307851
Type of change:
Required Checklist:
Further comments