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 children #455

Closed
wants to merge 1 commit into from

Conversation

scothis
Copy link
Contributor

@scothis scothis commented Nov 6, 2023

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.

@scothis scothis requested a review from squeedee November 6, 2023 19:27
@scothis scothis changed the title Allow ChildReconciler to opt-in to dangerous chilren Allow ChildReconciler to opt-in to dangerous children Nov 6, 2023
Copy link

codecov bot commented Nov 6, 2023

Codecov Report

Attention: 16 lines in your changes are missing coverage. Please review.

Comparison is base (e22ffba) 60.67% compared to head (ce3c333) 60.43%.

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     #455      +/-   ##
==========================================
- Coverage   60.67%   60.43%   -0.24%     
==========================================
  Files          26       26              
  Lines        2543     2563      +20     
==========================================
+ Hits         1543     1549       +6     
- Misses        911      924      +13     
- Partials       89       90       +1     

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

@squeedee
Copy link
Collaborator

Still want to review this, just so you know, we haven't forgotten it :D

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 <andrewssc@vmware.com>
@squeedee
Copy link
Collaborator

I'm leaving this pending until I have time to evaluate it with something real world. I like this solution a lot, for my very specific usecase, but adding something dangerous only to satisfy my use case might be selfish.

@scothis
Copy link
Contributor Author

scothis commented Mar 30, 2024

Migrated to reconcilerio/runtime#506

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants