✨Builder: Do not require For

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.

Author: alvaroaleman

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 {
@@ -249,6 +249,9 @@ func (blder *Builder) doWatch() error {
 	}
 
 	// Do the watch requests
+	if len(blder.watchesInput) == 0 && blder.forInput.object == nil {
+		return errors.New("there are no watches configured, controller will never get triggered. Use For(), Owns() or Watches() to set them up")
+	}
 	for _, w := range blder.watchesInput {
 		allPredicates := append([]predicate.Predicate(nil), blder.globalPredicates...)
 		allPredicates = append(allPredicates, w.predicates...)
@@ -269,11 +272,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 +292,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 +316,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,
 				)
 			}

pkg/builder/controller_test.go

@@ -112,16 +112,55 @@ 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 return an error when there are no watches", func() {
+			By("creating a controller manager")
+			m, err := manager.New(cfg, manager.Options{})
+			Expect(err).NotTo(HaveOccurred())
+
+			instance, err := ControllerManagedBy(m).
+				Named("my_controller").
+				Build(noop)
+			Expect(err).To(MatchError(ContainSubstring("there are no watches configured, controller will never get triggered. Use For(), Owns() or Watches() to set them up")))
+			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() {