🏃 Design for Simplified Scaffolding #620

DirectXMan12 · 2019-03-04T23:46:12Z

This introduces a design for a simplified scaffolding that addresses
both persistent pedagogical issues and dependency injection issues with
our current scaffolding.

k8s-ci-robot · 2019-03-04T23:46:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: DirectXMan12

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]

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

DirectXMan12 · 2019-03-04T23:48:34Z

see also #603. Holding off on a full-on prototype for a bit, but this looks quite a bit similar to how I structure the controllers when I'm developing or prototyping with CR.

DirectXMan12 · 2019-03-04T23:48:52Z

cc @droot @mengqiy

mengqiy · 2019-03-05T21:29:24Z

designs/simplified-scaffolding.md

+#### Use a single types.go file
+
+This works fine when you have a single "major" Kind, but quickly grows
+unweildy when you have multiple major kinds and end up with


unweildy

Typo?

mengqiy · 2019-03-05T22:26:24Z

designs/simplified-scaffolding.md

+│   ├── mykind_controller_test.go
+│   └── controllers_suite_test.go
+├── api
+│   └── v1


It sounds like this is the proposed structure if we have more than one versions.

├── api │ ├── v1 │ │ └── foo_types.go │ │ └── groupversion_info.go │ └── v2 │ └── foo_types.go │ └── groupversion_info.go

It sounds like the info about group will be duplicated across versions.
Given we assume only one group in this structure, does it make sense to have structure like

├── api │ ├── group_info.go │ ├── v1 │ │ └── foo_types.go │ │ └── version_info.go │ └── v2 │ └── foo_types.go │ └── version_info.go

hmm... the group info is the same, but the version is not, and you're adding different types to each. The amount shared between groups is relatively minimal (just group name), so I'm not sure it makes sense to have the extra file.

IMO, repeating the group-info in this case is reasonable. Adding extra files just for that will lead to the similar problems that we have today.

mengqiy · 2019-03-05T22:30:37Z

designs/simplified-scaffolding.md

+3. Tell users to move things, and scaffold out with the new structure.
+   This is fairly messy for the user.
+
+Since growing to multiple API groups seems to be fairly uncommon, it's


fairly uncommon

IIUC you mentioned that you have some data to back this when we chat offline. If you put the number here it's more convincing to make the decision :)

swear I had this somewhere, but can't seem to find the data now. I'll see what I can put together

hmm... reproducing what I had before is proving difficult (the query I was using before is now returning wildly truncated results), but with a random sampling of smaller projects on github using kubebuilder (presence of manager.yaml) and larger projects that I know of that make use of controller runtime or just CRDs in general (e.g. kubeflow, knative, cluster-api, cert-manager, app-crd, prometheus operator, etcd operator) most seem to follow the pattern of 1 API group per repo. The main exception was a couple of the knative repositories (but not all of them) and openshift, which puts all of its types in a single repo anyway, so isn't relevant to us.

This makes some amount of sense -- most operators or controllers logically operate on a single set of resources (the CRD defining the operator, a collection of related resources that work together) which would generally be grouped into a single API group.

hmm...another crazy idea :)

What if we removed the api and introduced the group hierarchy.

./test/project - groupa/v1/.... - groupb/v2/...

droot

Proposal looks reasonable to me. Have a few comments.

droot · 2019-03-06T20:47:49Z

designs/simplified-scaffolding.md

+([GoDoc](https://godoc.org/sigs.k8s.io/controller-runtime/pkg/builder#ControllerManagedBy))
+as a way to simplify construction of controllers and reduce boilerplate
+for the common cases of controller construction.  Informal feedback from
+this has been positive, and it enables fairly rapid, clear, and consice


s/consice/concise

droot · 2019-03-06T20:48:29Z

designs/simplified-scaffolding.md

+
+This proliferation of files makes it difficult for users to understand how
+their code relates to the library, posing some barier for initial adoption
+and moving beyond a basic knowelege of functionality to actual


typo: knowledge

droot · 2019-03-06T20:55:52Z

designs/simplified-scaffolding.md

+│   ├── mykind_controller_test.go
+│   └── controllers_suite_test.go
+├── api
+│   └── v1


IMO, repeating the group-info in this case is reasonable. Adding extra files just for that will lead to the similar problems that we have today.

droot · 2019-03-06T21:02:23Z

designs/simplified-scaffolding.md

+(`main.go` is the defacto standard entrypoint for many go programs), and
+simplifies the levels of hierarchy.  Furthermore, since `main.go` actually
+instantiates an instance of the reconciler, users are able to add custom
+logic having to do with flags.


droot · 2019-03-06T21:13:11Z

designs/simplified-scaffolding.md

+   the current structure.
+
+Option 1 should be fairly simple, since the logic is already needed for
+registering types to the scheme.


In case where user has removed that particular code comment (or we are unable to wire second controller for some reason), we can always fallback to option 2 where we just emit the snippet which user can copy-paste in their main.go.

droot · 2019-03-06T21:22:16Z

designs/simplified-scaffolding.md

+3. Tell users to move things, and scaffold out with the new structure.
+   This is fairly messy for the user.
+
+Since growing to multiple API groups seems to be fairly uncommon, it's


hmm...another crazy idea :)

What if we removed the api and introduced the group hierarchy.

./test/project - groupa/v1/.... - groupb/v2/...

DirectXMan12 · 2019-03-06T23:32:45Z

What if we removed the api and introduced the group hierarchy.

hmm... in this case, would controllers go under this hierarchy as well? I'm not certain this would be clear, depending on the group name -- e.g. with a group name of apps or dependencies, it's not clear that it's an API group and not something else.

This introduces a design for a simplified scaffolding that addresses both persistent pedagogical issues and dependency injection issues with our current scaffolding.

droot · 2019-03-07T19:35:51Z

hmm... in this case, would controllers go under this hierarchy as well? I'm not certain this would be clear, depending on the group name -- e.g. with a group name of apps or dependencies, it's not clear that it's an API group and not something else.

Yeah, removing api and introducing groups directly can get confusing. Not a good idea, I think.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 4, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 4, 2019

k8s-ci-robot requested review from droot and mengqiy March 4, 2019 23:46

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Mar 4, 2019

DirectXMan12 force-pushed the design/simplified-scaffolding branch 2 times, most recently from e382092 to 35897e9 Compare March 4, 2019 23:52

mengqiy reviewed Mar 5, 2019

View reviewed changes

DirectXMan12 mentioned this pull request Mar 6, 2019

Umbrella: Refactor for simpler building blocks, composability kubernetes-sigs/controller-tools#162

Closed

droot reviewed Mar 6, 2019

View reviewed changes

🏃 Design for Simplified Scaffolding

9d5d401

This introduces a design for a simplified scaffolding that addresses both persistent pedagogical issues and dependency injection issues with our current scaffolding.

DirectXMan12 force-pushed the design/simplified-scaffolding branch from 35897e9 to 9d5d401 Compare March 6, 2019 23:33

droot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 8, 2019

k8s-ci-robot merged commit 5ca2489 into kubernetes-sigs:master Mar 8, 2019

droot mentioned this pull request Mar 12, 2019

Umbrella Issue: Support builder pattern based scaffolding #630

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🏃 Design for Simplified Scaffolding #620

🏃 Design for Simplified Scaffolding #620

DirectXMan12 commented Mar 4, 2019

k8s-ci-robot commented Mar 4, 2019

DirectXMan12 commented Mar 4, 2019

DirectXMan12 commented Mar 4, 2019

mengqiy Mar 5, 2019

mengqiy Mar 5, 2019

DirectXMan12 Mar 6, 2019 •

edited

Loading

droot Mar 6, 2019

mengqiy Mar 5, 2019

DirectXMan12 Mar 6, 2019

DirectXMan12 Mar 6, 2019

droot Mar 6, 2019

droot left a comment

droot Mar 6, 2019

droot Mar 6, 2019

droot Mar 6, 2019

droot Mar 6, 2019

droot Mar 6, 2019

droot Mar 6, 2019

DirectXMan12 commented Mar 6, 2019

droot commented Mar 7, 2019 •

edited

Loading

🏃 Design for Simplified Scaffolding #620

🏃 Design for Simplified Scaffolding #620

Conversation

DirectXMan12 commented Mar 4, 2019

k8s-ci-robot commented Mar 4, 2019

DirectXMan12 commented Mar 4, 2019

DirectXMan12 commented Mar 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DirectXMan12 Mar 6, 2019 • 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DirectXMan12 commented Mar 6, 2019

droot commented Mar 7, 2019 • edited Loading

DirectXMan12 Mar 6, 2019 •

edited

Loading

droot commented Mar 7, 2019 •

edited

Loading