Merge pull request #2091 from alvaroaleman/no-for

✨Builder: Do not require For

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() {