New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Update sample to use kubebuilder controller library #5

Merged

pwittrock merged 1 commit into kubernetes-sigs:master from pwittrock:master

Mar 19, 2018

Contributor

pwittrock commented Mar 16, 2018 •

edited

Loading

This PR changes the for of sample-controller to use the kubebuilder controller libraries.

Note: Reduces the size of the controller by 50% (~200) lines

k8s-ci-robot added cncf-cla: yes size/L labels

droot reviewed

View reviewed changes

Contributor

droot left a comment

Took a quick look without going through the main repo and have couple of questions.
I will go through the main repo to have better context.

samples/controller/controller.go Outdated

+              		deploymentsLister: iargs.KubernetesInformers.Apps().V1().Deployments().Lister(),
+              		foosLister:        iargs.Informers.Samplecontroller().V1alpha1().Foos().Lister(),
+              		recorder:          iargs.CreateRecorder(controllerAgentName),
+              	}

Contributor

droot Mar 16, 2018

wondering if Controller struct can have iargs in it ? because everything the controller needs is coming from iargs ?

apelisse Mar 17, 2018

That'd be somewhat similar to inject cobra deep down everywhere IMHO. Objects like Controller shouldn't be coupled with iargs, or with the source of their parameters.

Contributor Author

pwittrock Mar 17, 2018

Yeah, I think that might be the way to go.

samples/controller/controller.go Outdated

-              	if shutdown {
-              		return false
-              	}
+              	genericController.WatchAndMapToControllerIf(&appsv1.Deployment{}, handlefunctions.ResourceVersionChanged, MapToDeploymentToTypes...)

Contributor

droot Mar 16, 2018

Still trying to understand the mapping stuff here. Looked the code handleObject(..) and if it is replacing that functionality then we can potentially improve the API name name here "WatchOwnersIf(...)"

also thinking if we can make the predicates more obvious like if it read "predicates.ResourceVersionChanged(...)" ?

Contributor Author

pwittrock Mar 17, 2018

+1 to both

apelisse reviewed

View reviewed changes

apelisse left a comment

It does look much simpler.

samples/controller/controller.go Outdated

+              		deploymentsLister: iargs.KubernetesInformers.Apps().V1().Deployments().Lister(),
+              		foosLister:        iargs.Informers.Samplecontroller().V1alpha1().Foos().Lister(),
+              		recorder:          iargs.CreateRecorder(controllerAgentName),
+              	}

apelisse Mar 17, 2018

That'd be somewhat similar to inject cobra deep down everywhere IMHO. Objects like Controller shouldn't be coupled with iargs, or with the source of their parameters.

samples/controller/controller.go Outdated

-              	// Get the Foo resource with this namespace/name
+              func (c *Controller) syncHandler(key types.ReconcileKey) error {
+              	namespace, name := key.Namespace, key.Name

apelisse Mar 17, 2018

major nit: why?! =))

Contributor Author

pwittrock Mar 17, 2018

To try and keep it as close to the original as possible. If I change stuff like this then there are a bunch of lines changed that are orthogonal.

natronq approved these changes

View reviewed changes

samples/controller/controller.go Outdated

-              		return false
-              	}
+              	genericController.WatchAndMapToControllerIf(&appsv1.Deployment{}, handlefunctions.ResourceVersionChanged,
+              		metav1.GroupVersionKind{Group: "apps", Version: "v1", Kind: "Deployment"})

Contributor

natronq Mar 16, 2018

The WatchAndMapToControllerIf function compares the owner of the watched object to the given GroupVersionKind. So shouldn't the GroupVersionKind here refer to sample1alphva1.Foo instead of appsv1.Deployment?

Contributor Author

pwittrock Mar 17, 2018

Yes good catch. Updated.


          Update sample to use kubebuilder controller library

c5e577f

droot approved these changes

View reviewed changes

Contributor

droot left a comment

/lgtm

Contributor

k8s-ci-robot commented Mar 19, 2018

@droot: changing LGTM is restricted to assignees, and only kubernetes-sigs org members may be assigned issues.

In response to this:

/lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

pwittrock merged commit 966df30 into kubernetes-sigs:master

jkevlin pushed a commit to jkevlin/kubebuilder that referenced this pull request


          # This is a combination of 15 commits.

9f1c319

# This is the 1st commit message:

Document adding an external type

# This is the commit message kubernetes-sigs#2:

Fix typo in book: flag is --owner, not --owners

# This is the commit message kubernetes-sigs#3:

gitbook: fixed installation instructions

# This is the commit message kubernetes-sigs#4:

Fix domain resource issues in controller

- fix in controller-tools and vendor in kubebuilder
- bind resource for controller based on whether resource is created.

# This is the commit message kubernetes-sigs#5:

improve contributing guide

# This is the commit message kubernetes-sigs#6:

Fix URL for the Gitbook

The URL is .io, not .com
# This is the commit message kubernetes-sigs#7:

Check go version

Fixes kubernetes-sigs#353

# This is the commit message kubernetes-sigs#8:

Validate create api flags

Fixes kubernetes-sigs#321

# This is the commit message kubernetes-sigs#9:

Update stable version in book

Update newest stable version to "1.0.1".

# This is the commit message kubernetes-sigs#10:

thirdparty tools: Dockerfile uses k8s 1.11

# This is the commit message kubernetes-sigs#11:

build: update cloudbuild config for building tools to use k8s 1.11

# This is the commit message kubernetes-sigs#12:

add document for connecting to existing kind

# This is the commit message kubernetes-sigs#13:

add document for connecting to existing kind

# This is the commit message kubernetes-sigs#14:

removed company from doc

# This is the commit message kubernetes-sigs#15:

fixed typo

3ks pushed a commit to 3ks/kubebuilder that referenced this pull request


          Merge pull request kubernetes-sigs#5 from zhangguanzhang/zh

0bc5bbc

fix icon and img of the index

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cncf-cla: yes size/L