-
Notifications
You must be signed in to change notification settings - Fork 36
🐛 Fix builder engage options to merge instead of replace #94
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
🐛 Fix builder engage options to merge instead of replace #94
Conversation
|
|
|
Welcome @stanfordpeng! |
026f0a1 to
cbd7586
Compare
|
This looks good, please just remove the "Fixes" part from the commit message (as you can see, the bot doesn't like that). |
When multiple EngageOptions are passed to For() or Owns(), they should merge field-by-field rather than replacing the entire struct. This makes the behavior consistent with ApplyToWatches(), which already correctly merges options. Before this fix, passing both WithEngageWithLocalCluster(true) and WithEngageWithProviderClusters(false) would result in only the last option being applied, making hub-and-spoke patterns impossible. This change allows both options to be set simultaneously by merging each field independently, creating new pointer instances to avoid aliasing issues. Fixes kubernetes-sigs/issues/93
cbd7586 to
9e8b8a8
Compare
embik
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/approve
Thank you for the contribution!
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: embik, stanfordpeng The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
LGTM label has been added. Git tree hash: f696cb1ee6db96d64bed2a00724e10e3406c12fd
|
Summary
This PR fixes the
EngageOptionsbehavior inApplyToFor()andApplyToOwns()to merge options field-by-field instead of replacing the entire struct. This makes the behavior consistent withApplyToWatches(), which already correctly merges options. See #93Problem
When using the multicluster builder with multiple
EngageOptions, the last option would overwrite all previous options instead of merging them:Due to the implementation in
ApplyToFor()andApplyToOwns(), onlyengageWithProviderClusters=falsewould be applied, whileengageWithLocalClusterwould remainniland fall back to the default.This made it impossible to configure hub-and-spoke patterns where you want to watch resources in the local cluster but access remote clusters via
GetCluster().Solution
Changed
ApplyToFor()andApplyToOwns()to merge options field-by-field, matching the existing pattern inApplyToWatches():This properly merges each field independently while creating new pointer instances to avoid aliasing issues.
Testing
Verified that the hub-and-spoke pattern now works correctly by testing with the following configuration:
The controller now correctly watches only the local cluster while maintaining access to remote clusters via the manager.