Skip to content
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.

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

Upgrade to Kubebuilder v2 #1723

Merged
merged 85 commits into from
Sep 23, 2019
Merged

Upgrade to Kubebuilder v2 #1723

merged 85 commits into from
Sep 23, 2019

Conversation

anyasabo
Copy link
Contributor

@anyasabo anyasabo commented Sep 12, 2019

Summary

Related to #1188 and #1604

  • Migrate to kubebuilder v2 API layout -- essentially the boilerplate is contained to groupversion_info.go rather than spread out over multiple files.
  • Upgrade to golang v1.13
  • Disables check-fmt during CI since controller-gen generates deepcopy files that do not match some newer versions, and enforcing it as part of CI is discouraged by the go team. I'll open a PR to fix it in controller-tools.
  • Removes the webhook server. It will be reintroduced in a future PR once we nail down details
  • Adds InjectMapper() to DynamicEnqueueRequest because it was introduced upstream in https://github.com/kubernetes-sigs/controller-runtime/pull/274/files
  • Removes outdated controller-gen markers and updates to current markers (see docs). This is primarily +genclient, +k8s:openapi-gen=true, and +k8s:deepcopy-gen
  • The k8s client CRUD options were refactored to take ...ListOptions etc, which required...a bit of refactoring in our calls rather than passing around a Selector everywhere. There were also some tests that were passing that should have been failing (and are now failing) because selectors were not applied to fake clients in the past.
  • Revs golangci-lint to 1.18 to support golang 1.13 -- also disables funlen because that throws a lot of errors that we can fix later if we want
  • Disables local changes as part of CI check. It was warning about go.mod modifications, even though if I run make ci locally nothing modifies my gomod files.
  • Adds CI test output files to gitignore

Differences from stock kubebuilder v2

  • Not using distroless
  • Not exposing election command line opts
  • There are now controller builders in controller-runtime to make the watches a little bit easier to set up. For our basic use cases it may be worth migrating to be more idiomatic.
  • Normally there's not a cmd pkg, but since we parse a lot of command line opts this is idiomatic for cobra

Outstanding tasks

  • Update CI to work with go modules
  • Update manifest generation -- see run/install/deploy/manifests make targets in new kubebuilder defaults vs our generate-crds and generate-all-in-one
  • Decide how we rework webhook functionality
    • Fix outstanding license validation todo
  • Add new rbac markers. Example here
  • Re-enable API reference documentation. Once we tag a release of our fork this can be started.

Notes

To compare to a base project I like to start up a new project to see how they scaffold it out

kubebuilder init --domain tutorial.kubebuilder.io
kubebuilder create api --group batch --version v1 --kind CronJob

Migration guide: https://book.kubebuilder.io/migration/guide.html
Marker docs: https://book.kubebuilder.io/reference/markers.html

@pebrc
Copy link
Collaborator

pebrc commented Sep 19, 2019

I'd generally be in favor of merging this quickly

👍 I think so too

pkg/utils/k8s/client.go Outdated Show resolved Hide resolved
Copy link
Contributor

@thbkrkr thbkrkr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!
Left some nits and questions but nothing to prevent a merge.

test/e2e/test/k8s_client.go Outdated Show resolved Hide resolved
test/e2e/kb/association_test.go Outdated Show resolved Hide resolved
pkg/controller/common/watches/handler.go Outdated Show resolved Hide resolved
cmd/manager/main.go Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
@anyasabo
Copy link
Contributor Author

Created follow up issues for all the ones I know about, I'm sure we'll find more

Copy link
Contributor

@sebgl sebgl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

Copy link
Collaborator

@pebrc pebrc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@@ -55,22 +54,22 @@ var (
// Add creates a new ApmServerElasticsearchAssociation Controller and adds it to the Manager with default RBAC. The Manager will set fields on the Controller
// and Start it when the Manager is Started.
func Add(mgr manager.Manager, params operator.Parameters) error {
r := newReconciler(mgr, params)
r := NewReconciler(mgr, params)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any particular reason these need to be exported?

Copy link
Contributor Author

@anyasabo anyasabo Sep 23, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant to leave a comment about that or fix it -- that was part of the change to using the new controller builder funcs that I later rolled back. Now there's not a good reason so I'll switch it back.

@anyasabo
Copy link
Contributor Author

anyasabo commented Sep 23, 2019

Jenkins test this please

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>enhancement Enhancement of existing functionality v1.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants