kubernetes-sigs
Builder's `For`, `Owns` and `Watches` methods overwrite multiple options
The Builder struct's For, Owns and Watches methods all follow the same pattern of having a variable number of modifying Option structs as the final parameter to the method. Here is the code for the For method:
// For defines the type of Object being *reconciled*, and configures the ControllerManagedBy to respond to create / delete /
// update events by *reconciling the object*.
// This is the equivalent of calling
// Watches(&source.Kind{Type: apiType}, &handler.EnqueueRequestForObject{})
func (blder *Builder) For(object runtime.Object, opts ...ForOption) *Builder {
input := ForInput{object: object}
for _, opt := range opts {
opt.ApplyToFor(&input)
}
blder.forInput = input
return blder
}
The problem with the above is if you call For with more than one ForOption, the builder's forInput member ends up containing only the predicates contained in the last ForOption. The reason is because the ForOption.ApplyToFor assigns the ForOption.predicates member to the supplied ForInput.predicates member (note: ForOption is an interface that is implemented by the Predicates struct):
// ApplyToFor applies this configuration to the given ForInput options.
func (w Predicates) ApplyToFor(opts *ForInput) {
opts.predicates = w.predicates
}
The solution is to append w.predicates to the supplied opt.predicates instead of assigning/overwriting the value of opts.predicates:
// ApplyToFor applies this configuration to the given ForInput options.
func (w Predicates) ApplyToFor(opts *ForInput) {
opts.predicates = append(opts.predicates, w.predicates...)
}
Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
If this issue is safe to close now please do so with /close.
Send feedback to sig-testing, kubernetes/test-infra and/or fejta. /lifecycle stale
Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.
If this issue is safe to close now please do so with /close.
Send feedback to sig-testing, kubernetes/test-infra and/or fejta. /lifecycle rotten
@vincepri @sbueringer @alvaroaleman Is the intent to only have 1 For option but make it not required when doing this? ~~If we need to adjust to allow multiple, I can have a PR that appends the values to allow multiple ForOptions to be applied to the forInput~~
I see the discussion on the previous PR. Do we want to do the nil check to close on this issue?
