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.

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

move RequiresReplace check of plan to Update time instead of Observe & suppress diffs with only computed attributes #341

Conversation

erhancagirici
Copy link
Collaborator

@erhancagirici erhancagirici commented Feb 9, 2024

Description of your changes

Moves the RequiresReplace check of the Plan to start of the Update, instead of Observe.

Currently plans are checked if they return RequiresReplace at diff calculation time at Observe calls, and the plan is refused if they require resource replace with an error due to XRM compliance.
However, this prevents late initialization of MR fields with RequiresReplace during initial creation, due to first observe after creation calculates a diff with RequiresReplace that gets rejected, since those fields are not late-initialized yet. Then, the diff planning exists early, not letting it reach to late initialization section.

Therefore the enforcement of the RequiresReplace plans are moved to Update time, to let fields late initialize first and enforce it after update decision is made.

Also, this change suppresses diffs with only computed attributes. The current diff detection mechanism compares the plan response and the current state. Plans can contain unknown values for Computed attributes, which is shown as a "false-positive" diff in the current diff logic.

This change suppreses diff elements that has unknown plan value. Note that it does NOT modify the plan itself in any way, just influences the logic of whether there is a diff or not.

I have:

  • Read and followed Upjet's contribution process.
  • Run make reviewable to ensure this PR is ready for review.
  • Added backport release-x.y labels to auto-backport this PR if necessary.

How has this code been tested

Tested with new opensearchserverless resources in PR crossplane-contrib/provider-upjet-aws#1130 and all existing Terraform Plugin Framework resources

… diffs with only computed values

Signed-off-by: Erhan Cagirici <[email protected]>
@erhancagirici erhancagirici force-pushed the plugin-fw-requires-replace-at-update-time branch from 2d9df14 to 86997cd Compare February 15, 2024 08:35
Copy link
Member

@sergenyalcin sergenyalcin left a comment

Choose a reason for hiding this comment

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

Thanks @erhancagirici left a comment

Copy link
Member

@sergenyalcin sergenyalcin left a comment

Choose a reason for hiding this comment

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

Thanks @erhancagirici LGTM!

@sergenyalcin sergenyalcin merged commit af53160 into crossplane:main Feb 15, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants