From 2d261401070f4eb8bff8b7687ad33669d669e77e Mon Sep 17 00:00:00 2001 From: Alvaro Aleman Date: Sun, 11 Dec 2022 12:04:04 -0500 Subject: [PATCH] =?UTF-8?q?=E2=9C=A8Builder:=20Do=20not=20require=20For?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Requiring For does not make sense for all controllers as it is possible that their primary event triggering is not an object in the current cluster but for example an object in a different cluster or a source.Channel. --- pkg/builder/controller.go | 47 +++++++++++++++++++++++----------- pkg/builder/controller_test.go | 31 ++++++++++++++++++++-- 2 files changed, 61 insertions(+), 17 deletions(-) diff --git a/pkg/builder/controller.go b/pkg/builder/controller.go index efaf069205..c7ec4d2122 100644 --- a/pkg/builder/controller.go +++ b/pkg/builder/controller.go @@ -17,6 +17,7 @@ limitations under the License. package builder import ( + "errors" "fmt" "strings" @@ -182,10 +183,6 @@ func (blder *Builder) Build(r reconcile.Reconciler) (controller.Controller, erro if blder.forInput.err != nil { return nil, blder.forInput.err } - // Checking the reconcile type exist or not - if blder.forInput.object == nil { - return nil, fmt.Errorf("must provide an object for reconciliation") - } // Set the ControllerManagedBy if err := blder.doController(r); err != nil { @@ -231,6 +228,9 @@ func (blder *Builder) doWatch() error { } // Watches the managed types + if len(blder.ownsInput) > 0 && blder.forInput.object == nil { + return errors.New("Owns() can only be used together with For()") + } for _, own := range blder.ownsInput { typeForSrc, err := blder.project(own.object, own.objectProjection) if err != nil { @@ -269,11 +269,14 @@ func (blder *Builder) doWatch() error { return nil } -func (blder *Builder) getControllerName(gvk schema.GroupVersionKind) string { +func (blder *Builder) getControllerName(gvk schema.GroupVersionKind, hasGVK bool) (string, error) { if blder.name != "" { - return blder.name + return blder.name, nil + } + if !hasGVK { + return "", errors.New("one of For() or Named() must be called") } - return strings.ToLower(gvk.Kind) + return strings.ToLower(gvk.Kind), nil } func (blder *Builder) doController(r reconcile.Reconciler) error { @@ -286,13 +289,18 @@ func (blder *Builder) doController(r reconcile.Reconciler) error { // Retrieve the GVK from the object we're reconciling // to prepopulate logger information, and to optionally generate a default name. - gvk, err := getGvk(blder.forInput.object, blder.mgr.GetScheme()) - if err != nil { - return err + var gvk schema.GroupVersionKind + hasGVK := blder.forInput.object != nil + if hasGVK { + var err error + gvk, err = getGvk(blder.forInput.object, blder.mgr.GetScheme()) + if err != nil { + return err + } } // Setup concurrency. - if ctrlOptions.MaxConcurrentReconciles == 0 { + if ctrlOptions.MaxConcurrentReconciles == 0 && hasGVK { groupKind := gvk.GroupKind().String() if concurrency, ok := globalOpts.GroupKindConcurrency[groupKind]; ok && concurrency > 0 { @@ -305,21 +313,30 @@ func (blder *Builder) doController(r reconcile.Reconciler) error { ctrlOptions.CacheSyncTimeout = *globalOpts.CacheSyncTimeout } - controllerName := blder.getControllerName(gvk) + controllerName, err := blder.getControllerName(gvk, hasGVK) + if err != nil { + return err + } // Setup the logger. if ctrlOptions.LogConstructor == nil { log := blder.mgr.GetLogger().WithValues( "controller", controllerName, - "controllerGroup", gvk.Group, - "controllerKind", gvk.Kind, ) + if hasGVK { + log = log.WithValues( + "controllerGroup", gvk.Group, + "controllerKind", gvk.Kind, + ) + } ctrlOptions.LogConstructor = func(req *reconcile.Request) logr.Logger { log := log if req != nil { + if hasGVK { + log = log.WithValues(gvk.Kind, klog.KRef(req.Namespace, req.Name)) + } log = log.WithValues( - gvk.Kind, klog.KRef(req.Namespace, req.Name), "namespace", req.Namespace, "name", req.Name, ) } diff --git a/pkg/builder/controller_test.go b/pkg/builder/controller_test.go index 4b694dcf45..d8ffc7d0eb 100644 --- a/pkg/builder/controller_test.go +++ b/pkg/builder/controller_test.go @@ -112,16 +112,43 @@ var _ = Describe("application", func() { Expect(instance).To(BeNil()) }) - It("should return an error if For function is not called", func() { + It("should return an error if For and Named function are not called", func() { By("creating a controller manager") m, err := manager.New(cfg, manager.Options{}) Expect(err).NotTo(HaveOccurred()) instance, err := ControllerManagedBy(m). + Watches(&source.Kind{Type: &appsv1.ReplicaSet{}}, &handler.EnqueueRequestForObject{}). + Build(noop) + Expect(err).To(MatchError(ContainSubstring("one of For() or Named() must be called"))) + Expect(instance).To(BeNil()) + }) + + It("should return an error when using Owns without For", func() { + By("creating a controller manager") + m, err := manager.New(cfg, manager.Options{}) + Expect(err).NotTo(HaveOccurred()) + + instance, err := ControllerManagedBy(m). + Named("my_controller"). Owns(&appsv1.ReplicaSet{}). Build(noop) - Expect(err).To(MatchError(ContainSubstring("must provide an object for reconciliation"))) + Expect(err).To(MatchError(ContainSubstring("Owns() can only be used together with For()"))) Expect(instance).To(BeNil()) + + }) + + It("should allow creating a controllerw without calling For", func() { + By("creating a controller manager") + m, err := manager.New(cfg, manager.Options{}) + Expect(err).NotTo(HaveOccurred()) + + instance, err := ControllerManagedBy(m). + Named("my_controller"). + Watches(&source.Kind{Type: &appsv1.ReplicaSet{}}, &handler.EnqueueRequestForObject{}). + Build(noop) + Expect(err).NotTo(HaveOccurred()) + Expect(instance).NotTo(BeNil()) }) It("should return an error if there is no GVK for an object, and thus we can't default the controller name", func() {