⚠️ controller-tools v0.2.0 scaffolding update #682

DirectXMan12 · 2019-05-03T01:45:19Z

This updates the v2 scaffolding to support the upcoming v0.2.0 controller-tools branch. It disables webhook-related things, since v2 scaffolding doesn't scaffold webhook Go code yet and it was breaking stuff.

It also assumes controller-gen is packaged as a binary. Since we've got Go modules, we can just GO111MODULE=on go get sigs.k8s.io/controller-tools/cmd/controller-gen@<temporary-branch-for-now>, which is what this does.

Still waiting on:

use new scaffolding structure to support admission|conversion webhooks #644 to merge
controller-tools v0.2.0
controller-runtime with modules to merge
add makefile rule to fetch controller-tools ?

k8s-ci-robot · 2019-05-03T01:45:36Z

[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-05-03T01:46:02Z

cc @mengqiy @droot

see also kubernetes-sigs/controller-tools#190

DirectXMan12 · 2019-05-03T05:39:58Z

hey, look 🎉 modules 🎉 (required some temporary branches)

droot · 2019-05-03T17:57:46Z

Skim through it, looking good overall. Have a few questions related to go-modules. Will take a detailed look soon.

It also assumes controller-gen is packaged as a binary.

We haven't made that call yet, so lets discuss.

droot

Looks good.

pkg/scaffold/project/kustomize_rbac_base.go

testdata/project_v2/config/rbac/kustomization.yaml

DirectXMan12 · 2019-05-04T07:02:50Z

We haven't made that call yet, so lets discuss.

For posterity: we discussed a bit in person, and came to the consensus that just go-getting it with modules is a fine option, and easier to manage than wrangling it as an explicit dependency.

generated_golden.sh

test.sh

mengqiy · 2019-05-06T19:45:37Z

testdata/gopath/src/project/pkg/apis/creatures/v2alpha1/doc.go

@@ -17,7 +17,7 @@ limitations under the License.
 // Package v2alpha1 contains API Schema definitions for the creatures v2alpha1 API group
 // +k8s:openapi-gen=true
 // +k8s:deepcopy-gen=package,register
-// +k8s:conversion-gen=sigs.k8s.io/kubebuilder/test/project/pkg/apis/creatures
+// +k8s:conversion-gen=project/pkg/apis/creatures
 // +k8s:defaulter-gen=TypeMeta


These markers actually can be removed.
Same comment for all the doc comments below.

these are all v1 scaffolding, which shouldn't change under this PR.

testdata/project_v2/Makefile

mengqiy · 2019-05-06T19:59:20Z

testdata/project_v2/config/crd/kustomization.yaml

@@ -2,7 +2,7 @@
 # since it depends on service name and namespace that are out of this kustomize package.
 # It should be run by config/default
 resources:
- bases/crew_firstmate.yaml
+- bases/crew.testproject.org_firstmates.yaml


It looks strange with mixture of . and _.
If I understand what's happening here correctly,

before:
group=crew
domainName=testproject.org

after:
group=crew.testproject.org

it's how controller-tools generates its. I think it's fine -- the _ separates group from resource nicely. We could switch to <resource>.<group> in CT, but I'm not sure it's worth it.

mengqiy · 2019-05-06T20:49:45Z

pkg/scaffold/v2/crd/enablewebhook_patch.go

@@ -36,7 +36,7 @@ type EnableWebhookPatch struct {
 // GetInput implements input.File
 func (p *EnableWebhookPatch) GetInput() (input.Input, error) {
 	if p.Path == "" {
-		p.Path = filepath.Join("config", "crds", "patches",
+		p.Path = filepath.Join("config", "crd", "patches",


Is there a convention to use singular or plural?

it was by default just the name of the generator in CT. Since we're overriding it anyway, it can be whatever

mengqiy · 2019-05-06T20:50:42Z

pkg/scaffold/v2/manager/kustomization.go

-    kind: Service
-    version: v1
-    name: webhook-service
+# vars:


Why comment these?

if we don't scaffold the webhook service, kustomize barfs on these variables.

This switches dependencies over to Go modules, dropping dep. This does so *only* for kubebuilder itself, not generated code.

This removes the vendor directory, since it's not needed by modules.

droot

reviewing commit by commit, first commit switch kb deps over to modules looks good.

droot

reviewed Move test project to testdata commit, looks good. minor nits about adding some comments.

test.sh

droot · 2019-05-09T23:51:34Z

test.sh

@@ -139,7 +140,7 @@ cd ${go_workspace}/src/sigs.k8s.io/kubebuilder
 go test ./cmd/... ./pkg/...

 # test project v1
-test_project project 1
+GO111MODULE=auto test_project gopath/src/project 1


Comment explaining use of auto for GO111MODULE will help why is that being done for version 1 project.

droot

Reviewed Actually move test projects to testdata commit, have some minor comments.

droot · 2019-05-09T23:52:59Z

.gitattributes

@@ -1 +1,2 @@
 test/*.tgz filter=lfs diff=lfs merge=lfs -text


may be we can take out the first line ?

droot · 2019-05-09T23:55:19Z

testdata/gopath/src/project/cmd/manager/main.go

@@ -20,14 +20,15 @@ import (
 	"flag"
 	"os"

+	"project/pkg/apis"
+	"project/pkg/controller"
+	"project/pkg/webhook"


These imports look wrong.

testdata/gopath/src/project/pkg/apis/addtoscheme_creatures_v2alpha1.go

droot · 2019-05-09T23:58:13Z

testdata/gopath/src/project/pkg/controller/firstmate/firstmate_controller_suite_test.go

@@ -23,13 +23,14 @@ import (
 	"sync"
 	"testing"

+	"project/pkg/apis"


is this import correct ? (or may be, because we set the gopath when we run test).

yes, because we're at $GOPATH/src/project

droot

Reviewed Switch v2 Scaffolding to Use Go Modules commit, have a few minor comments.

droot · 2019-05-10T00:04:26Z

.travis.yml

@@ -5,7 +5,7 @@ os:
  - osx

 go:
- "1.11"
+- "1.12"


we should ensure the version check we have is also bumped up to 1.12.

nah, it's just for building KB, I believe. Everything else works fine with Go 1.11 (in fact, building KB also would work fine, except that Travis's Go 1.11 is slightly old and has a bug).

common.sh

generated_golden.sh

droot

reviewed Scaffold v1 with controller-runtime v0.1.8, have one minor comment.

pkg/scaffold/project/gopkg.go

droot

Reviewed Fix imports.Process being confused on v1, looks good.

pkg/scaffold/v1/resource/addtoscheme.go

droot

reviewed Switch v2 scaffolding to controller-tools 0.2.0. Looks good except @mengqiy's comment about disabling some manifests.

droot

Reviewed Switch to current directory in tests, looks good.

droot

reviewed the last commit fixup! Switch kb dependencies over to modules, and looks good.

Go tooling ignores testdata, so the Go module manipulation commands (pretty much everything that Go does) will ignore the test projects. The v1 scaffolding needs to be in its own gopath, otherwise the tooling will ignore the vendor directory.

DirectXMan12 · 2019-05-10T03:58:43Z

Dep is having a conniption over v0.1.10 and a mismatch in patch versions on kubernetes libs, so I'll stick on v0.1.8 here for the v1 scaffolding.

This performs the move of the actual files that's set up in the previous commit.

This switches the v2 scaffolding to use Go modules. Currently, a custom fork of controller-runtime is being used. We'll need to merge that to controller-runtime master before merging this.

A change was introduced to the v1 scaffolding at some point that broke new scaffolding on v0.1.1, so upgrade to v0.1.8 to fix it again.

imports.Process makes some dubious assumptions about how import paths correspond to package names (not wanting to believe that a normal import path could possibly end in v1), and fails to properly double-check its work. Introduce an explicit alias to work around that.

This switches the v2 scaffolding to make use of (the upcoming) controller-tools v0.2.0, which has rewritten generators and a bundled deepcopy. It's currently using a prerelease version. controller-tools is now consumed as a binary, available via `go get` with modules turned on.

Lots of the module stuff (and more) depends on the "context" of the module, so being in the right directory when we run the imports.Process from the tests is important.

DirectXMan12 · 2019-05-10T04:08:02Z

comments should be addressed

DirectXMan12 · 2019-05-10T04:08:18Z

fixed a pluralization bug in the mean time

DirectXMan12 · 2019-05-10T20:50:48Z

The golden file unit tests are borked again. The pass locally for me, so 🤷‍♀️ . I'm half tempted to just nix them entirely and replace them with some bash that does the same thing, since the goimports library is so context-dependent. They're basically the equivalent of [[ -z $(git status --porcelain testdata/gopath/src/project ]]

They're roughly equivalent of generated_golden.sh and git status --porcelain, so we should just do that.

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/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels May 3, 2019

k8s-ci-robot requested review from mengqiy and pwittrock May 3, 2019 01:45

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

DirectXMan12 force-pushed the feature/controller-tools-0.2.0 branch from 233c626 to b681f06 Compare May 3, 2019 02:15

DirectXMan12 force-pushed the feature/controller-tools-0.2.0 branch from b094e61 to 227eded Compare May 3, 2019 06:02

droot reviewed May 3, 2019

View reviewed changes

pkg/scaffold/project/kustomize_rbac_base.go Outdated Show resolved Hide resolved

testdata/project_v2/config/rbac/kustomization.yaml Outdated Show resolved Hide resolved

DirectXMan12 force-pushed the feature/controller-tools-0.2.0 branch from 1da248f to 000587d Compare May 4, 2019 06:52

DirectXMan12 force-pushed the feature/controller-tools-0.2.0 branch from 000587d to bc7bada Compare May 4, 2019 22:54

DirectXMan12 changed the title ~~[WIP] ⚠️ controller-tools v0.2.0 scaffolding update~~ ⚠️ controller-tools v0.2.0 scaffolding update May 6, 2019

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 6, 2019

mengqiy reviewed May 6, 2019

View reviewed changes

DirectXMan12 force-pushed the feature/controller-tools-0.2.0 branch from bc7bada to cf87637 Compare May 6, 2019 23:53

mengqiy mentioned this pull request May 7, 2019

Release kubebuilder and k8s bundles separately #686

Closed

4 tasks

DirectXMan12 added 2 commits May 7, 2019 13:28

Switch kb dependencies over to modules

008660c

This switches dependencies over to Go modules, dropping dep. This does so *only* for kubebuilder itself, not generated code.

Remove vendor (no longer needed with modules)

6bf7647

This removes the vendor directory, since it's not needed by modules.

droot reviewed May 9, 2019

View reviewed changes

droot reviewed May 10, 2019

View reviewed changes

pkg/scaffold/project/gopkg.go Show resolved Hide resolved

droot reviewed May 10, 2019

View reviewed changes

pkg/scaffold/v1/resource/addtoscheme.go Show resolved Hide resolved

droot reviewed May 10, 2019

View reviewed changes

Move test project to testdata

50e68c5

Go tooling ignores testdata, so the Go module manipulation commands (pretty much everything that Go does) will ignore the test projects. The v1 scaffolding needs to be in its own gopath, otherwise the tooling will ignore the vendor directory.

DirectXMan12 added 6 commits May 9, 2019 21:07

Actually move test projects to testdata

8e34625

This performs the move of the actual files that's set up in the previous commit.

Switch v2 Scaffolding to Use Go Modules

0530fd1

This switches the v2 scaffolding to use Go modules. Currently, a custom fork of controller-runtime is being used. We'll need to merge that to controller-runtime master before merging this.

Scaffold v1 with controller-runtime v0.1.8

1d03cb4

A change was introduced to the v1 scaffolding at some point that broke new scaffolding on v0.1.1, so upgrade to v0.1.8 to fix it again.

Switch to current directory in tests

82451fb

Lots of the module stuff (and more) depends on the "context" of the module, so being in the right directory when we run the imports.Process from the tests is important.

DirectXMan12 force-pushed the feature/controller-tools-0.2.0 branch from cf87637 to 82451fb Compare May 10, 2019 04:07

Remove golden file unit tests

070467e

They're roughly equivalent of generated_golden.sh and git status --porcelain, so we should just do that.

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

k8s-ci-robot merged commit e46e06e into kubernetes-sigs:master May 10, 2019

DirectXMan12 deleted the feature/controller-tools-0.2.0 branch May 12, 2019 22:44

mengqiy mentioned this pull request May 22, 2019

✨ use Deployment for v2 scaffolding #727

Merged

@@ @@ -5,7 +5,7 @@ os: @@
                 - osx
               go:
-              - "1.11"
+              - "1.12"

⚠️ controller-tools v0.2.0 scaffolding update #682

⚠️ controller-tools v0.2.0 scaffolding update #682

Conversation

DirectXMan12 commented May 3, 2019 • edited Loading

k8s-ci-robot commented May 3, 2019

DirectXMan12 commented May 3, 2019

DirectXMan12 commented May 3, 2019

droot commented May 3, 2019

droot left a comment

Choose a reason for hiding this comment

DirectXMan12 commented May 4, 2019

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

droot left a 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

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

droot left a comment

Choose a reason for hiding this comment

droot left a comment

Choose a reason for hiding this comment

droot left a comment

Choose a reason for hiding this comment

droot left a comment

Choose a reason for hiding this comment

droot left a comment

Choose a reason for hiding this comment

DirectXMan12 commented May 10, 2019

DirectXMan12 commented May 10, 2019

DirectXMan12 commented May 10, 2019

DirectXMan12 commented May 10, 2019

DirectXMan12 commented May 3, 2019 •

edited

Loading