Add support for creating core type controllers #132

Liujingfang1 · 2018-05-07T17:02:45Z

Add flag controller-only to init
Add command create controller with flag --core-type
Update controller template and inject template
Add test with create controller --core-type in test.sh
This is to fix (L) Support for creating controllers for core types #41

Liujingfang1 · 2018-05-07T23:01:54Z

Add parseDomain from file directly and remove a lot of if statements. PTAL

droot

Took a quick look, have couple of comments.

droot · 2018-05-08T21:00:22Z

cmd/internal/codegen/parse/parser.go

+				glog.Fatal(err)
+			}
+
+			comments := Comments(lines)


May be, we can move this logic in the scanner loop itself itself. Ignore the line if its not a comment, else invoke Comments(line) and check for domain ?

I updated this function to ignore the lines not a comment.
Comments(line) is left after all comments are get since Comments is the same type of []string.

droot · 2018-05-08T21:04:27Z

cmd/kubebuilder/create/resource/controller.go

@@ -42,11 +42,17 @@ import (
    "github.com/kubernetes-sigs/kubebuilder/pkg/controller"
    "github.com/kubernetes-sigs/kubebuilder/pkg/controller/types"
    "k8s.io/client-go/tools/record"
-
+    {{if .CoreType}}


What does CoreType means here ?

Should be ControllerOnly, will fix it.

I changed it back to CoreType in create controller command. It makes sense now compared with ControllerOnly.

droot · 2018-05-08T21:09:22Z

cmd/kubebuilder/initproject/init.go


 func AddInit(cmd *cobra.Command) {
 	cmd.AddCommand(repoCmd)
 	repoCmd.Flags().StringVar(&domain, "domain", "", "domain for the API groups")
 	repoCmd.Flags().StringVar(&copyright, "copyright", filepath.Join("hack", "boilerplate.go.txt"), "Location of copyright boilerplate file.")
 	repoCmd.Flags().BoolVar(&bazel, "bazel", false, "if true, setup Bazel workspace artifacts")
+	repoCmd.Flags().BoolVar(&controllerOnly, "controller-only", false, "if true, setup controller only")


Can I add API resource to a project which has been initialized as "controller-only" ?

If yes, then does this flag makes sense ?

It won't work since the pkg/inject won't build

Hmm.... it will be great if we can remove this restriction.
Ideal workflow is - to be able to not to take this flag as input for init command and then be able to add resources for non-core type and controllers for core-types.

Lets discuss today to explore if we can make this happen.

I agree with you, it's nice to remove the restriction.
The divergence comes from the different dependencies for core type controller and CRD behaviors. Having a flag in init command, we can easily distinguish the different dependencies.
Let's discuss to see if there's better solution.

As discussed offline, we keep this flag for now. Later when the dynamic client is added in the kube builder library, we will try to remove this flag.

droot · 2018-05-08T21:51:37Z

cmd/kubebuilder/create/resource/run.go

@@ -61,6 +62,7 @@ func AddCreateResource(cmd *cobra.Command) {
 	createResourceCmd.Flags().BoolVar(&nonNamespacedKind, "non-namespaced", false, "if set, the API kind will be non namespaced")
 	createResourceCmd.Flags().BoolVar(&controller, "controller", true, "if true, generate the controller code for the resource")
 	createResourceCmd.Flags().BoolVar(&generate, "generate", true, "generate source code")
+	createResourceCmd.Flags().BoolVar(&controllerOnly, "controller-only", false, "skip resources, generate controller only")


hmm.... add a resource with --controller-only looks a bit confusing. "Add controller" should address this use case right ?

I didn't realize that it might cause confusion. Do you suggest to add a new command like add controller or create controller? This new command will have almost the same flags and need to call the same functions as current create resource. I'm not sure if it worth to do it.

yeah I think create controller makes sense. We don't actually want to create a resource in this case.

Liujingfang1 · 2018-05-09T17:48:17Z

Add a subcommand create controller. Now we can create core type controller by
kubebuilder create controller --group <group> --version <version> --kind <kind> --core-type

droot

Looks good to me. Was wondering if we can improve the test coverage ?

droot · 2018-05-10T17:28:14Z

@Liujingfang1 travis tests are failing on this one.

Liujingfang1 · 2018-05-10T17:32:27Z

I updated the test script in travis-ci. Seems it's not properly added. I'll fix it.

Liujingfang1 · 2018-05-10T17:48:51Z

Change the command in test.sh as

kubebuilder create controller --group insect --version v1beta1 --kind Wasp
kubebuilder create controller --group ant --version v1beta1 --kind Ant

Liujingfang1 · 2018-05-11T16:50:22Z

Add two different workflows in test.sh

Create a coretype controller after creating a CRD resource

  kubebuilder create resource --group ant --version v1beta1 --kind Ant
  kubebuilder create controller --group apps --version v1beta2 --kind Deployment --core-type

Create coretype controller only

    kubebuilder init repo --domain sample.kubernetes.io --controller-only
    kubebuilder create controller --group apps --version v1beta2 --kind Deployment --core-type

Note that for the type Deployment, the object of this type must have some fields nonempty. For those fields, I add a post step to update the controller test file.

Liujingfang1 · 2018-05-11T17:22:41Z

@droot @pwittrock All tests passed, PTAL

droot

Thanks for working on the tests for this one.
Have some more comments.

droot · 2018-05-11T17:00:46Z

cmd/kubebuilder-gen/internal/controllergen/inject.go

+	    	"k8s.io/client-go/kubernetes/scheme",
+	    	"rscheme " + "\"" + repo + "/pkg/client/clientset/versioned/scheme\""}...
+	    )
+    }


Keeping track of these imports feels like a lot of hand-written magic :)
Another possible approach could be ...we add all the imports and then run goimports to remove extra imports.

As discussed offline, Let goimports add imports could be risky. We may only allow goimports to remove extra imports. It can be done in a different PR.

droot · 2018-05-11T17:08:37Z

cmd/kubebuilder/create/controller/controllertest.go

+    {{if .CoreType}}
+    "{{ .Repo }}/pkg/inject"
+    "{{ .Repo }}/pkg/inject/args"
+    {{else}}
    "{{ .Repo }}/pkg/client/clientset/versioned"


may be just have the clienset/versioned guarded ?

droot · 2018-05-11T17:22:32Z

cmd/kubebuilder/create/controller/run.go

+var nonNamespacedKind bool
+var generate bool
+var CoreType bool
+


We should move away from this pattern of defining pkg level globals for command options. Currently all commands are implemented like this but its an anti-pattern where push down cobra.command and these globals too deep in to the business logic.

Idea is to define a separate struct for command args:
type controllerArgs struct { namespaced bool coreType bool generate bool }
Bind above struct field to the command-args. So filling of struct is delegated to cobra with some basic validation.

Then have validate() method on controllerArgs

And then execution should NOT take cobra.cmd or args as input like we do today.

Not a deal breaker, but wanted to bring it out (can be handled in separate PR but will be good if we can do it in current PR because then we can point people to the pattern here to refactor commands):

I can do it in this PR.

droot · 2018-05-11T17:29:26Z

cmd/kubebuilder/initproject/init.go


 func AddInit(cmd *cobra.Command) {
 	cmd.AddCommand(repoCmd)
 	repoCmd.Flags().StringVar(&domain, "domain", "", "domain for the API groups")
 	repoCmd.Flags().StringVar(&copyright, "copyright", filepath.Join("hack", "boilerplate.go.txt"), "Location of copyright boilerplate file.")
 	repoCmd.Flags().BoolVar(&bazel, "bazel", false, "if true, setup Bazel workspace artifacts")
+	repoCmd.Flags().BoolVar(&controllerOnly, "controller-only", false, "if true, setup controller only")


Hmm.... it will be great if we can remove this restriction.
Ideal workflow is - to be able to not to take this flag as input for init command and then be able to add resources for non-core type and controllers for core-types.

Lets discuss today to explore if we can make this happen.

droot · 2018-05-11T17:33:13Z

test.sh

+  export PATH=/tmp/kubebuilder/bin/:$PATH
+  export TEST_ASSET_KUBECTL=/tmp/kubebuilder/bin/kubectl
+  export TEST_ASSET_KUBE_APISERVER=/tmp/kubebuilder/bin/kube-apiserver
+  export TEST_ASSET_ETCD=/tmp/kubebuilder/bin/etcd


we can set these env once ?

droot · 2018-05-11T17:34:51Z

test.sh

+}
+
+function update_controller_test {
+  # Update import


Its not very clear what are we doing here ?

Explained offline. Also added a comment for explanation.

droot

LGTM.
Lets check with @pwittrock if he has any ideas to workaround the restriction of "not being able to add API resource to a project created using --controller-only flag during init"

…rence-markers-rbac zh-translation:/reference/markers/rbac.md

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 7, 2018

Liujingfang1 mentioned this pull request May 7, 2018

add support for creating coretype controller #117

Closed

Liujingfang1 force-pushed the tmpcore branch 2 times, most recently from b6db00e to 17a7a2a Compare May 7, 2018 17:46

Liujingfang1 changed the title ~~[WIP] Add support for creating core type controllers~~ Add support for creating core type controllers May 7, 2018

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 7, 2018

Liujingfang1 requested review from pwittrock and droot May 7, 2018 17:47

Liujingfang1 force-pushed the tmpcore branch 2 times, most recently from 23c8f1d to 2b25ee4 Compare May 7, 2018 22:51

droot reviewed May 8, 2018

View reviewed changes

Liujingfang1 force-pushed the tmpcore branch 6 times, most recently from b295792 to ed6ab6b Compare May 9, 2018 17:44

Liujingfang1 force-pushed the tmpcore branch from ed6ab6b to df5b9ac Compare May 10, 2018 16:50

droot reviewed May 10, 2018

View reviewed changes

Liujingfang1 force-pushed the tmpcore branch from dc514e7 to 994834c Compare May 10, 2018 17:35

Liujingfang1 force-pushed the tmpcore branch 2 times, most recently from 13a802b to 57bd34d Compare May 11, 2018 16:45

Liujingfang1 force-pushed the tmpcore branch 2 times, most recently from 190b139 to 23a42ba Compare May 11, 2018 17:13

droot reviewed May 11, 2018

View reviewed changes

Liujingfang1 force-pushed the tmpcore branch 3 times, most recently from 52c95aa to e61782f Compare May 11, 2018 20:49

Liujingfang1 added 3 commits May 14, 2018 13:43

add coretype controller

62a5b54

Add test for create controller

c545cba

Address comments

fe2d0f9

Liujingfang1 force-pushed the tmpcore branch from e61782f to fe2d0f9 Compare May 14, 2018 20:48

droot reviewed May 14, 2018

View reviewed changes

pwittrock merged commit 391d011 into kubernetes-sigs:master May 14, 2018

Liujingfang1 deleted the tmpcore branch June 1, 2018 23:15

grantr mentioned this pull request Jun 12, 2018

Generated controllers don't match samples #264

Closed

luffyao pushed a commit to luffyao/kubebuilder that referenced this pull request Aug 6, 2020

Merge pull request kubernetes-sigs#132 from helight/zh-translate-refe…

b8a7938

…rence-markers-rbac zh-translation:/reference/markers/rbac.md

Add support for creating core type controllers #132

Add support for creating core type controllers #132

Conversation

Liujingfang1 commented May 7, 2018 • edited Loading

Liujingfang1 commented May 7, 2018

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

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

Liujingfang1 commented May 9, 2018

droot left a comment

Choose a reason for hiding this comment

droot commented May 10, 2018

Liujingfang1 commented May 10, 2018

Liujingfang1 commented May 10, 2018

Liujingfang1 commented May 11, 2018

Liujingfang1 commented May 11, 2018

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

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

Liujingfang1 commented May 7, 2018 •

edited

Loading