-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Evaluate using generics for source/predicate/handler chain #2214
Comments
/assign |
The Kubernetes project currently lacks enough contributors to adequately respond to all issues. This bot triages un-triaged issues according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
/assign |
The Kubernetes project currently lacks enough contributors to adequately respond to all issues. This bot triages un-triaged issues according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues. This bot triages un-triaged issues according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle rotten |
I did look into this. It works great for just the source/predicate/handler chain, but due to golang/go#49085 it makes the UX of the |
I can give it another try if you'd like |
Its not about giving it a try. I don't think it is possible to do this without that golang issue being solved or accepting that the UX of the builder will become quite a bit worse. Let me try to find the branch I did this in (its been a couple of months) so it can be shown |
Hi, I added a PR which propagates type information to the handler - #2698 Also faced with the same issue as @alvaroaleman mentioned. One way to tackle this is to pass prepared source.Kind to all builder methods instead of object ( Another option is to wrap |
Yeah a wrapper around generic source/predicate/handler is the only way to make this work in the builder. Just to be clear on this, we will not be merging any code unless we are sure with worsening the builder UX like that. |
I guess to extend to the prior message, I would really love to see us using generics there somehow because that would mean that you can build arbitrary controllers that might not even act on kube resources. But not at the price of significantly worsening the experience when building kube controllers, as that will remain the 98% use case of controller-runtime. |
I think the builder pattern can be preserved. Here is the flow I came up with: ctrl.
NewControllerManagedBy(manager). // Create the Controller
With(ctrl.Object(&appsv1.ReplicaSet{})). // ReplicaSet is the Application API
Own(ctrl.Object(&corev1.Pod{})). // ReplicaSet owns Pods created by it
Watch(ctrl.Object(&corev1.ConfigMap{})). // ConfigMaps are being watched
Complete(&ReplicaSetReconciler{Client: manager.GetClient()}) where // WatchObject is an interface on an object wrapper with preserved type information
type WatchObject interface {
GetObject() client.Object
SetSource(cache.Cache) source.SyncingSource
}
type watchObject[T client.Object] struct {
object T
}
// SetSource returns inner client.Object
func (w watchObject[T]) GetObject() client.Object {
return w.object
}
// SetSource sets and returns the source.SyncingSource on the object
func (w watchObject[T]) SetSource(cache cache.Cache) source.SyncingSource {
return source.ObjectKind(cache, w.object)
}
// Object constructs a wrapper on a generic object with stored type information
func Object[T client.Object](obj T) WatchObject {
return watchObject[T]{object: obj}
} and both |
I looked into this a bit further… The biggest problem with integrating generics is an enormous amount of code duplication, to preserve current API definitions without changes introduced by generics. Essentially everything: predicates, TLDR: I would personally just prefer having generic predicates and eventHandler, and ensuring the type safety for the objects coming from cache (for starters), and focus on that. WDYT? The problem I’m facing is again, the builder, or rather the options for each watch input. I’m rotating this pseudocode design in mind at it’s most raw form: // internal source.Handler:
func NewEventHandler[T any](ctx context.Context, queue workqueue.RateLimitingInterface,
handler handler.EventHandler[T], predicates []predicate.Predicate[T]) *EventHandler[T] // handler:
func EnqueueRequestsFromMapFunc[T any](fn MapFunc[T]) EventHandler[T] // Predicate:
func NewPredicateFuncs[T any](filter func(object T) bool) Funcs[T] // Builder:
func (blder *Builder) Add(…) *Builder
func Watch[T any](object T, eventHandler handler.EventHandler[T], opts ...Option[T]) // Usage:
predicate := predicate.NewPredicateFuncs(func(pod *corev1.Pod) bool { return true }))
mapHandler := handler.EnqueueRequestsFromMapFunc(func(ctx context.Context, pod *corev1.Pod) []reconcile.Request …
b.Add(Watch(&corev1.Pod{}, mapHandler, predicate)) With this approach the first occurrence of But the options do not fit. If I specify type in it, then everything storing the struct to which the option should be applied have to inherit this type, and the I think that if the current UX brings enough improvement and it makes sense to add in some (similar or other) for, it would be good to add these changes piece by piece. Refactoring whole source/predicate/handler chain causes changes in the whole project. If we agree on that, we can split it in smaller parts and implement incrementally. |
Exactly, that is the unsolved problem. I do not want to force everyone to use a wrapper before calling |
Okay so one approach I can think of that would not require changes to exported parts of the builder is to change the |
I tried that, that works, have a very much WIP implementation for this in this branch. It is simpler than the original form I was trying to pursue, thanks for the pointer. I agree, it seems to me that the 2 approaches should co-exist, as forcing the majority to move their code to generics may cause confusion. |
@Danil-Grigorev would you be up to write a small doc that describes the changes in public interfaces needed for this and the motivation? |
I created #2783 for this which I think is the most compatible way we can do this, let me know what you think. |
Predicates currently take a
client.Object
and return abool
. With generics, we might be able to have typed generics, which would let ppl avoid a type assertion inside their predicate when they want to access anything other than metadata.We currently type to
client.Object
, but this is both too much in the case of asource.Channel
that could take literally everything that might not actually be aclient.Object
and too little in the case of everything else, as we lose the type information.The text was updated successfully, but these errors were encountered: