✨ Improve builder UX #264

pwittrock · 2018-12-20T05:04:12Z

No description provided.

DirectXMan12

few nits inline, generally looks good

needs a better PR description ;-)

DirectXMan12 · 2018-12-20T19:21:10Z

example_test.go

+	}
+
+	_, err = controllers.
+		NewControllerBuilder(manager). // Create the Controller


🚲 🏠 : maybe just NewControllerIn? Reads a bit more fluently

DirectXMan12 · 2018-12-20T19:21:51Z

example_test.go

+		NewControllerBuilder(manager). // Create the Controller
+		ForType(&appsv1.ReplicaSet{}). // ReplicaSet is the Application API
+		Owns(&corev1.Pod{}).           // ReplicaSet owns Pods created by it
+		Build(&ReplicaSetReconciler{Client: manager.GetClient()})


what's up with the return of explicit wiring?

I think this works for this case. It is pretty simple.

DirectXMan12 · 2018-12-20T19:24:16Z

pkg/builder/build.go

+
+// Watch exposes the lower-level Controller Watch functions through the builder.  Consider using
+// Owns or ForType instead of Watch directly.
+func (b *Builder) Watch(src source.Source, eventhandler handler.EventHandler) *Builder {


nit: follow the same phrasing as ForType or Owns here -- Watches or something similar

DirectXMan12 · 2018-12-20T19:26:17Z

pkg/builder/build.go

 // WithConfig sets the Config to use for configuring clients.  Defaults to the in-cluster config or to ~/.kube/config.
+// Deprecated - set through the Manager instead.


nit: we should prob follow the standard library convention: https://rakyll.org/deprecated/

DirectXMan12 · 2018-12-20T19:28:08Z

pkg/builder/build.go

@@ -114,12 +131,10 @@ func (b *Builder) Build(r reconcile.Reconciler) (manager.Manager, error) {
 		return nil, err
 	}

-	a := &application{mgr: b.mgr, ctrl: b.ctrl}
-
 	// Reconcile type
 	s := &source.Kind{Type: b.apiType}


blech, we should fix these single-letter variables in a follow-up

DirectXMan12 · 2018-12-20T19:30:00Z

pkg/builder/build_test.go

+	Describe("Start with Controller", func() {
+		depName := "deploy-name-2"
+		It("should Reconcile objects", func(done Done) {
+			By("Creating the application")


these tests effectively do the same thing after setting up the controller. We should prob refactor out into a help above for easier maintenance

or invert and do something like

func DescribeAController(setupTheController func() manager.Manager) { It("should reconcile objects", func(done Done) { mgr := setupTheController() // ... }) }

DirectXMan12 · 2018-12-20T19:32:43Z

pkg/builder/example_test.go

@@ -19,39 +19,41 @@ package builder_test
 import (
 	"context"
 	"fmt"
-	"os"
+	"log"


why're we reverting back to std log?

droot

few minor nits.

droot · 2018-12-20T19:42:53Z

alias.go

+*/
+
+package controller_runtime
+


We should add a comment explaining motivation for this file alias.go.

droot · 2018-12-20T19:44:05Z

alias.go

+
+func NewManager(config *rest.Config, options Options) (manager.Manager, error) {
+	return manager.New(config, options)
+}


nit: some fns/vars have comments and some don't, so reading the file feels inconsistent.

Also this will impact the godocs as well.

Done. Copied all comments

droot · 2018-12-20T19:50:14Z

example_test.go

+
+	_, err = controllers.
+		NewControllerBuilder(manager). // Create the Controller
+		ForType(&appsv1.ReplicaSet{}). // ReplicaSet is the Application API


wondering if we should drop Type ?

pwittrock · 2018-12-20T23:49:27Z

PTAL

DirectXMan12

couple of nits inline. Otherwise lgtm

DirectXMan12 · 2018-12-21T01:42:55Z

example_test.go

+	corev1 "k8s.io/api/core/v1"
+	controllers "sigs.k8s.io/controller-runtime"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+	logf "sigs.k8s.io/controller-runtime/pkg/runtime/log"


can just use controllers.Log, right?

DirectXMan12 · 2018-12-21T01:45:11Z

pkg/controller/controller.go

 		MaxConcurrentReconciles: options.MaxConcurrentReconciles,
-		Name:                    name,
+		Name: name,


did these get unformatted?

These are formatted with go fmt ./pkg/... and should be fine.

DirectXMan12 · 2018-12-21T01:45:32Z

pkg/source/source.go

@@ -241,7 +241,7 @@ func (cs *Channel) syncLoop() {
 	}
 }

-// Informer is used to provide a source of events originating inside the cluster from Watches (e.g. Pod Create)
+// Informer is used to provide a source of events originating inside the cluster from Watch (e.g. Pod Create)


Done by the IDE when refactoring the naming change requested in your previous comment.

DirectXMan12 · 2018-12-21T01:45:52Z

alias.go

+
+	// Log is the base logger used by kubebuilder.  It delegates
+	// to another logr.Logger.  You *must* call SetLogger to
+	// get any actual logging.


might want to note that SetLogger isn't here ;-)

pwittrock · 2018-12-21T22:23:10Z

PTAL. I am going on break. Can we merge this as is and follow up on any minor / formatting issues?

- Rename Builder functions - Encourage providing the manager to the Builder factory - Deprecate discouraged functions - Add alias.go with short cuts for common functions to reduce import line bloat - Fix issue with GetConfigOrDie not exiting - Update examples - Add example at package root

pwittrock · 2019-01-03T20:29:46Z

ping

droot

Looks good to me. would wait for @DirectXMan12 to take a final look.

droot · 2019-01-03T21:19:26Z

hack/verify.sh

@@ -52,6 +51,7 @@ gometalinter.v2 --disable-all \
    --skip=atomic \
    ./pkg/...
 # TODO: Enable these as we fix them to make them pass
+#    --enable=goimports \


It gave me trouble in the past and reinstalling (updating) goimports fixed the problem. I am assuming this will be enabled before the merge ?

droot · 2019-01-03T22:16:58Z

pkg/builder/build_test.go

+				For(&appsv1.Deployment{}).
+				Watches( // Equivalent of Owns
+					&source.Kind{Type: &appsv1.ReplicaSet{}},
+					&handler.EnqueueRequestForOwner{OwnerType: &appsv1.Deployment{}, IsController: true})


Looking at this example, builder for controller appears to be a very thin improvement over current controller abstraction because users have to understand manager, watches and handlers to use these. Am I missing something ?

this is just an example of using watches directly. We need to test the "I want .Owns and one more complicated thing" -- the test just happens to use the Owner enqueuer for convienience

droot · 2019-01-03T22:18:47Z

pkg/builder/example_test.go

+		ControllerManagedBy(mgr).  // Create the ControllerManagedBy
+		For(&appsv1.ReplicaSet{}). // ReplicaSet is the Application API
+		Owns(&corev1.Pod{}).       // ReplicaSet owns Pods created by it
+		Build(&ReplicaSetReconciler{})


I see it now (ignore my previous comment). Simple use-case is looking simple now :)

DirectXMan12

minor nits. Otherwise looks good

DirectXMan12 · 2019-01-03T23:47:02Z

pkg/builder/build.go

 // Supporting mocking out functions for testing
 var getConfig = config.GetConfig
 var newController = controller.New
 var newManager = manager.New
 var getGvk = apiutil.GVKForObject

-// Builder builds an Application Controller (e.g. Operator) and returns a manager.Manager to start it.
+// Builder builds an Application ControllerManagedBy (e.g. Operator) and returns a manager.Manager to start it.


find-replace issue

DirectXMan12 · 2019-01-03T23:50:05Z

pkg/builder/build_test.go

+				For(&appsv1.Deployment{}).
+				Watches( // Equivalent of Owns
+					&source.Kind{Type: &appsv1.ReplicaSet{}},
+					&handler.EnqueueRequestForOwner{OwnerType: &appsv1.Deployment{}, IsController: true})


this is just an example of using watches directly. We need to test the "I want .Owns and one more complicated thing" -- the test just happens to use the Owner enqueuer for convienience

…hat haven't changed their imports)

pwittrock · 2019-01-04T22:08:56Z

PTAL

DirectXMan12 · 2019-01-04T22:21:13Z

/lgtm
/approve

k8s-ci-robot · 2019-01-04T22:21:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: DirectXMan12, pwittrock

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [DirectXMan12,pwittrock]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 20, 2018

k8s-ci-robot requested review from DirectXMan12 and droot December 20, 2018 05:04

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Dec 20, 2018

DirectXMan12 suggested changes Dec 20, 2018

View reviewed changes

droot reviewed Dec 20, 2018

View reviewed changes

pwittrock force-pushed the simple branch 2 times, most recently from ad518da to 8f7b8af Compare December 20, 2018 23:18

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 20, 2018

pwittrock force-pushed the simple branch 6 times, most recently from 41d6293 to 42c1184 Compare December 21, 2018 00:48

DirectXMan12 suggested changes Dec 21, 2018

View reviewed changes

pwittrock force-pushed the simple branch 3 times, most recently from 1a65375 to 609e516 Compare December 21, 2018 22:22

pwittrock force-pushed the simple branch from 609e516 to 8409bdd Compare December 21, 2018 23:28

droot reviewed Jan 3, 2019

View reviewed changes

DirectXMan12 suggested changes Jan 3, 2019

View reviewed changes

disable metalinter goimports because it is broken (failing on files t…

58c9cd9

…hat haven't changed their imports)

pwittrock force-pushed the simple branch from dbb3b7c to 58c9cd9 Compare January 4, 2019 22:07

DirectXMan12 approved these changes Jan 4, 2019

View reviewed changes

k8s-ci-robot assigned DirectXMan12 Jan 4, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 4, 2019

k8s-ci-robot merged commit 5aed6d6 into kubernetes-sigs:master Jan 4, 2019

		// WithConfig sets the Config to use for configuring clients. Defaults to the in-cluster config or to ~/.kube/config.
		// Deprecated - set through the Manager instead.

✨ Improve builder UX #264

✨ Improve builder UX #264

Conversation

pwittrock commented Dec 20, 2018

DirectXMan12 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

droot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pwittrock Dec 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pwittrock commented Dec 20, 2018

DirectXMan12 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pwittrock Dec 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pwittrock commented Dec 21, 2018 • edited Loading

pwittrock commented Jan 3, 2019

droot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DirectXMan12 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pwittrock commented Jan 4, 2019

DirectXMan12 commented Jan 4, 2019

k8s-ci-robot commented Jan 4, 2019

pwittrock Dec 20, 2018 •

edited

Loading

pwittrock Dec 21, 2018 •

edited

Loading

pwittrock commented Dec 21, 2018 •

edited

Loading