Return controller from builder.Build

Return controller from builder.Build to allow developers to use it after
it's been created by the builder; e.g., to add watches dynamically from
a reconciler.

Remove deprecated SimpleController() and Builder.WithManager().

Signed-off-by: Andy Goldstein <[email protected]>

Author: ncdc

pkg/builder/controller.go

@@ -23,7 +23,6 @@ import (
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/client-go/rest"
 	"sigs.k8s.io/controller-runtime/pkg/client/apiutil"
-	"sigs.k8s.io/controller-runtime/pkg/client/config"
 	"sigs.k8s.io/controller-runtime/pkg/controller"
 	"sigs.k8s.io/controller-runtime/pkg/handler"
 	"sigs.k8s.io/controller-runtime/pkg/manager"
@@ -33,9 +32,7 @@ import (
 )
 
 // Supporting mocking out functions for testing
-var getConfig = config.GetConfig
 var newController = controller.New
-var newManager = manager.New
 var getGvk = apiutil.GVKForObject
 
 // Builder builds a Controller.
@@ -50,16 +47,9 @@ type Builder struct {
 	name           string
 }
 
-// SimpleController returns a new Builder.
-//
-// Deprecated: Use ControllerManagedBy(Manager) instead.
-func SimpleController() *Builder {
-	return &Builder{}
-}
-
 // ControllerManagedBy returns a new controller builder that will be started by the provided Manager
 func ControllerManagedBy(m manager.Manager) *Builder {
-	return SimpleController().WithManager(m)
+	return &Builder{mgr: m}
 }
 
 // ForType defines the type of Object being *reconciled*, and configures the ControllerManagedBy to respond to create / delete /
@@ -109,14 +99,6 @@ func (blder *Builder) WithConfig(config *rest.Config) *Builder {
 	return blder
 }
 
-// WithManager sets the Manager to use for registering the ControllerManagedBy.  Defaults to a new manager.Manager.
-//
-// Deprecated: Use ControllerManagedBy(Manager) and this isn't needed.
-func (blder *Builder) WithManager(m manager.Manager) *Builder {
-	blder.mgr = m
-	return blder
-}
-
 // WithEventFilter sets the event filters, to filter which create/update/delete/generic events eventually
 // trigger reconciliations.  For example, filtering on whether the resource version has changed.
 // Defaults to the empty list.
@@ -141,23 +123,17 @@ func (blder *Builder) Complete(r reconcile.Reconciler) error {
 	return err
 }
 
-// Build builds the Application ControllerManagedBy and returns the Manager used to start it.
-//
-// Deprecated: Use Complete
-func (blder *Builder) Build(r reconcile.Reconciler) (manager.Manager, error) {
+// Build builds the Application ControllerManagedBy and returns the Controller it created.
+func (blder *Builder) Build(r reconcile.Reconciler) (controller.Controller, error) {
 	if r == nil {
 		return nil, fmt.Errorf("must provide a non-nil Reconciler")
 	}
-
-	// Set the Config
-	if err := blder.loadRestConfig(); err != nil {
-		return nil, err
+	if blder.mgr == nil {
+		return nil, fmt.Errorf("must provide a non-nil Manager")
 	}
 
-	// Set the Manager
-	if err := blder.doManager(); err != nil {
-		return nil, err
-	}
+	// Set the Config
+	blder.loadRestConfig()
 
 	// Set the ControllerManagedBy
 	if err := blder.doController(r); err != nil {
@@ -169,7 +145,7 @@ func (blder *Builder) Build(r reconcile.Reconciler) (manager.Manager, error) {
 		return nil, err
 	}
 
-	return blder.mgr, nil
+	return blder.ctrl, nil
 }
 
 func (blder *Builder) doWatch() error {
@@ -203,26 +179,10 @@ func (blder *Builder) doWatch() error {
 	return nil
 }
 
-func (blder *Builder) loadRestConfig() error {
-	if blder.config != nil {
-		return nil
-	}
-	if blder.mgr != nil {
+func (blder *Builder) loadRestConfig() {
+	if blder.config == nil {
 		blder.config = blder.mgr.GetConfig()
-		return nil
 	}
-	var err error
-	blder.config, err = getConfig()
-	return err
-}
-
-func (blder *Builder) doManager() error {
-	if blder.mgr != nil {
-		return nil
-	}
-	var err error
-	blder.mgr, err = newManager(blder.config, manager.Options{})
-	return err
 }
 
 func (blder *Builder) getControllerName() (string, error) {

pkg/builder/controller_test.go

@@ -30,7 +30,6 @@ import (
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/types"
-	"k8s.io/client-go/rest"
 	"sigs.k8s.io/controller-runtime/pkg/controller"
 	"sigs.k8s.io/controller-runtime/pkg/handler"
 	"sigs.k8s.io/controller-runtime/pkg/manager"
@@ -44,9 +43,7 @@ var _ = Describe("application", func() {
 
 	BeforeEach(func() {
 		stop = make(chan struct{})
-		getConfig = func() (*rest.Config, error) { return cfg, nil }
 		newController = controller.New
-		newManager = manager.New
 	})
 
 	AfterEach(func() {
@@ -57,35 +54,32 @@ var _ = Describe("application", func() {
 
 	Describe("New", func() {
 		It("should return success if given valid objects", func() {
-			instance, err := SimpleController().
-				For(&appsv1.ReplicaSet{}).
-				Owns(&appsv1.ReplicaSet{}).
-				Build(noop)
+			By("creating a controller manager")
+			m, err := manager.New(cfg, manager.Options{})
 			Expect(err).NotTo(HaveOccurred())
-			Expect(instance).NotTo(BeNil())
-		})
 
-		It("should return an error if the Config is invalid", func() {
-			getConfig = func() (*rest.Config, error) { return cfg, fmt.Errorf("expected error") }
-			instance, err := SimpleController().
+			instance, err := ControllerManagedBy(m).
 				For(&appsv1.ReplicaSet{}).
 				Owns(&appsv1.ReplicaSet{}).
 				Build(noop)
-			Expect(err).To(HaveOccurred())
-			Expect(err.Error()).To(ContainSubstring("expected error"))
-			Expect(instance).To(BeNil())
+			Expect(err).NotTo(HaveOccurred())
+			Expect(instance).NotTo(BeNil())
 		})
 
 		It("should return an error if there is no GVK for an object", func() {
-			instance, err := SimpleController().
+			By("creating a controller manager")
+			m, err := manager.New(cfg, manager.Options{})
+			Expect(err).NotTo(HaveOccurred())
+
+			instance, err := ControllerManagedBy(m).
 				For(&fakeType{}).
 				Owns(&appsv1.ReplicaSet{}).
 				Build(noop)
 			Expect(err).To(HaveOccurred())
 			Expect(err.Error()).To(ContainSubstring("no kind is registered for the type builder.fakeType"))
 			Expect(instance).To(BeNil())
 
-			instance, err = SimpleController().
+			instance, err = ControllerManagedBy(m).
 				For(&appsv1.ReplicaSet{}).
 				Owns(&fakeType{}).
 				Build(noop)
@@ -94,25 +88,17 @@ var _ = Describe("application", func() {
 			Expect(instance).To(BeNil())
 		})
 
-		It("should return an error if it cannot create the manager", func() {
-			newManager = func(config *rest.Config, options manager.Options) (manager.Manager, error) {
-				return nil, fmt.Errorf("expected error")
-			}
-			instance, err := SimpleController().
-				For(&appsv1.ReplicaSet{}).
-				Owns(&appsv1.ReplicaSet{}).
-				Build(noop)
-			Expect(err).To(HaveOccurred())
-			Expect(err.Error()).To(ContainSubstring("expected error"))
-			Expect(instance).To(BeNil())
-		})
-
 		It("should return an error if it cannot create the controller", func() {
 			newController = func(name string, mgr manager.Manager, options controller.Options) (
 				controller.Controller, error) {
 				return nil, fmt.Errorf("expected error")
 			}
-			instance, err := SimpleController().
+
+			By("creating a controller manager")
+			m, err := manager.New(cfg, manager.Options{})
+			Expect(err).NotTo(HaveOccurred())
+
+			instance, err := ControllerManagedBy(m).
 				For(&appsv1.ReplicaSet{}).
 				Owns(&appsv1.ReplicaSet{}).
 				Build(noop)
@@ -150,30 +136,6 @@ var _ = Describe("application", func() {
 		})
 	})
 
-	Describe("Start with SimpleController", func() {
-		It("should Reconcile Owns objects", func(done Done) {
-			bldr := SimpleController().
-				ForType(&appsv1.Deployment{}).
-				WithConfig(cfg).
-				Owns(&appsv1.ReplicaSet{})
-			doReconcileTest("1", stop, bldr, nil, false)
-
-			close(done)
-		}, 10)
-
-		It("should Reconcile Owns objects with a Manager", func(done Done) {
-			m, err := manager.New(cfg, manager.Options{})
-			Expect(err).NotTo(HaveOccurred())
-
-			bldr := SimpleController().
-				WithManager(m).
-				For(&appsv1.Deployment{}).
-				Owns(&appsv1.ReplicaSet{})
-			doReconcileTest("2", stop, bldr, m, false)
-			close(done)
-		}, 10)
-	})
-
 	Describe("Start with ControllerManagedBy", func() {
 		It("should Reconcile Owns objects", func(done Done) {
 			m, err := manager.New(cfg, manager.Options{})
@@ -217,25 +179,21 @@ func doReconcileTest(nameSuffix string, stop chan struct{}, blder *Builder, mgr
 		return reconcile.Result{}, nil
 	})
 
-	instance := mgr
 	if complete {
 		err := blder.Complete(fn)
 		Expect(err).NotTo(HaveOccurred())
 	} else {
 		var err error
-		instance, err = blder.Build(fn)
+		var c controller.Controller
+		c, err = blder.Build(fn)
 		Expect(err).NotTo(HaveOccurred())
-	}
-
-	// Manager should match
-	if mgr != nil {
-		Expect(instance).To(Equal(mgr))
+		Expect(c).NotTo(BeNil())
 	}
 
 	By("Starting the application")
 	go func() {
 		defer GinkgoRecover()
-		Expect(instance.Start(stop)).NotTo(HaveOccurred())
+		Expect(mgr.Start(stop)).NotTo(HaveOccurred())
 		By("Stopping the application")
 	}()
 
@@ -263,7 +221,7 @@ func doReconcileTest(nameSuffix string, stop chan struct{}, blder *Builder, mgr
 			},
 		},
 	}
-	err := instance.GetClient().Create(context.TODO(), dep)
+	err := mgr.GetClient().Create(context.TODO(), dep)
 	Expect(err).NotTo(HaveOccurred())
 
 	By("Waiting for the Deployment Reconcile")
@@ -293,7 +251,7 @@ func doReconcileTest(nameSuffix string, stop chan struct{}, blder *Builder, mgr
 			Template: dep.Spec.Template,
 		},
 	}
-	err = instance.GetClient().Create(context.TODO(), rs)
+	err = mgr.GetClient().Create(context.TODO(), rs)
 	Expect(err).NotTo(HaveOccurred())
 
 	By("Waiting for the ReplicaSet Reconcile")

pkg/builder/webhook.go

@@ -55,25 +55,16 @@ func (blder *WebhookBuilder) For(apiType runtime.Object) *WebhookBuilder {
 // Complete builds the webhook.
 func (blder *WebhookBuilder) Complete() error {
 	// Set the Config
-	if err := blder.loadRestConfig(); err != nil {
-		return err
-	}
+	blder.loadRestConfig()
 
 	// Set the Webhook if needed
 	return blder.registerWebhooks()
 }
 
-func (blder *WebhookBuilder) loadRestConfig() error {
-	if blder.config != nil {
-		return nil
-	}
-	if blder.mgr != nil {
+func (blder *WebhookBuilder) loadRestConfig() {
+	if blder.config == nil {
 		blder.config = blder.mgr.GetConfig()
-		return nil
 	}
-	var err error
-	blder.config, err = getConfig()
-	return err
 }
 
 func (blder *WebhookBuilder) registerWebhooks() error {

pkg/builder/webhook_test.go

@@ -28,7 +28,6 @@ import (
 	. "github.com/onsi/gomega"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
-	"k8s.io/client-go/rest"
 	"sigs.k8s.io/controller-runtime/pkg/controller"
 	"sigs.k8s.io/controller-runtime/pkg/manager"
 	"sigs.k8s.io/controller-runtime/pkg/scheme"
@@ -40,9 +39,7 @@ var _ = Describe("application", func() {
 
 	BeforeEach(func() {
 		stop = make(chan struct{})
-		getConfig = func() (*rest.Config, error) { return cfg, nil }
 		newController = controller.New
-		newManager = manager.New
 	})
 
 	AfterEach(func() {

pkg/patterns/application/doc.go

@@ -20,7 +20,7 @@ limitations under the License.
 // An application is a Controller and Resource that together implement the operational logic for an application.
 // They are often used to take off-the-shelf OSS applications, and make them Kubernetes native.
 //
-// A typical application Controller may use a new builder.SimpleController() to create a Controller
+// A typical application Controller may use builder.ControllerManagedBy() to create a Controller
 // for a single API type that manages other objects it creates.
 //
 // Application Controllers are most useful for stateful applications such as Cassandra, Etcd and MySQL