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

Allow ChildReconciler to opt-in to dangerous chilren #506

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

scothis
Copy link
Contributor

@scothis scothis commented Mar 30, 2024

Duck types represent a partial view of a resource. It is dangerous to perform operation that assume the client has the full resource when it only has a portion of the resource. Calling update in this case can result in data loss as fields that are not part of the duck type will be dropped on the server. Likewise, it is difficult to create a resource with a partial object as required/essential fields may not be available.

This change allows users to explicitly override these concerns and use the ChildReconciler and ChildSetReconciler to manage a child via a duck type. This should only be done when the user is certain the duck typed object contains all fields represented on the resource.

Duck types represent a partial view of a resource. It is dangerous to
perform operation that assume the client has the full resource when it
only has a portion of the resource. Calling update in this case can
result in data loss as fields that are not part of the duck type will be
dropped on the server. Likewise, it is difficult to create a resource
with a partial object as required/essential fields may not be available.

This change allows users to explicitly override these concerns and use
the ChildReconciler and ChildSetReconciler to manage a child via a duck
type.  This should only be done when the user is certain the duck typed
object contains all fields represented on the resource.

Signed-off-by: Scott Andrews <[email protected]>
@scothis
Copy link
Contributor Author

scothis commented Mar 30, 2024

Originally vmware-labs/reconciler-runtime#455. Rebased and package imports updated.

Copy link

codecov bot commented Mar 30, 2024

Codecov Report

Attention: Patch coverage is 27.27273% with 16 lines in your changes are missing coverage. Please review.

Project coverage is 60.89%. Comparing base (ce74140) to head (3cb7e95).

Files Patch % Lines
reconcilers/child.go 23.07% 9 Missing and 1 partial ⚠️
reconcilers/config.go 0.00% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #506      +/-   ##
==========================================
- Coverage   61.13%   60.89%   -0.24%     
==========================================
  Files          28       28              
  Lines        2583     2603      +20     
==========================================
+ Hits         1579     1585       +6     
- Misses        917      930      +13     
- Partials       87       88       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mamachanko
Copy link
Contributor

Do you plan to hold off of this change until #509?

@scothis
Copy link
Contributor Author

scothis commented Apr 9, 2024

Was originally holding off until @squeedee was able to verify the behavior. Now it's worth holding until we can verify server side apply.

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