-
Notifications
You must be signed in to change notification settings - Fork 37
🐛 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
base: main
Are you sure you want to change the base?
🐛 Fix builder engage options to merge instead of replace #94
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: stanfordpeng The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
|
|
Welcome @stanfordpeng! |
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#93
026f0a1 to
cbd7586
Compare
|
Keywords which can automatically close issues and at(@) or hashtag(#) mentions are not allowed in commit messages. The list of commits with invalid commit messages:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Summary
Fixes #93
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.Problem
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.