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

Use kubebuilder controller-gen for creating CRDs #2855

Merged
merged 6 commits into from Nov 22, 2019

Conversation

pgier
Copy link
Contributor

@pgier pgier commented Nov 14, 2019

Testing switching over to kubebuilder controller-get for CRD generation

@s-urbaniak
Copy link
Contributor

looks great, thank you! 🎉

@s-urbaniak
Copy link
Contributor

Question, can we also replace openapi-gen with controller-tools? I believe support for that has been added recently.

$(PO_CRDGEN_BINARY): cmd/po-crdgen/main.go $(OPENAPI_TARGET)
@go install -mod=vendor $(GO_PKG)/cmd/po-crdgen
$(CONTROLLER_GEN_BINARY):
@go install -mod=vendor sigs.k8s.io/controller-tools/cmd/controller-gen
Copy link

Choose a reason for hiding this comment

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

don't use an arbitrary version, but download a binary. Controller tools is often lagging behind in Go and Kube version support. I would try to avoid any trouble by using a binary. We use those from https://github.com/openshift/kubernetes-sigs-controller-tools/releases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was just following what we are doing for the other build tools here which AFAIK is preferred since it avoids committing binary tools to git or downloading them at build time. It's not an arbitrary version because it's installed from the vendor directory set to v0.2.2. But I can certainly change it if that's preferred, @s-urbaniak wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

@pgier I am voting for pinning the version in the Makefile 👍

@paulfantom
Copy link
Member

Whoa, awesome! 🎉

@pgier
Copy link
Contributor Author

pgier commented Nov 15, 2019

@s-urbaniak I'm not sure if we can replace openapi-gen, but we can at least replace the deepcopy-gen the rbac stuff also.

@sttts
Copy link

sttts commented Nov 15, 2019

I'm not sure if we can replace openapi-gen

Why not?

@s-urbaniak
Copy link
Contributor

@pgier CRD schema generation was added by @sttts in kubernetes-sigs/controller-tools#183, so we should be able to leverage that feature.

Having said that I am also fine in doing this in a step-by-step manner as a follow-up.

Replaces the custom po-crdgen command
Removes dependency on deepcopy-gen command
@pgier pgier force-pushed the controller-gen-crd branch 2 times, most recently from bde01cb to 9592167 Compare November 20, 2019 22:14
Generate bindata.go from the generated crd yaml and then
unmarshal the bindata yaml into CRD structs at runtime.
This removes the need for openapi-gen.
@pgier pgier changed the title [WIP] Use kubebuilder controller-gen for creating CRDs Use kubebuilder controller-gen for creating CRDs Nov 21, 2019
@pgier
Copy link
Contributor Author

pgier commented Nov 21, 2019

Assuming the tests pass this time, I think this is ready for another review and merge if everything looks ok.

@pgier
Copy link
Contributor Author

pgier commented Nov 21, 2019

Just to add some notes about the changes here, we are now generating the CRDs as yaml files from types.go using controller-gen, then generating bindata for those yaml files, and then unmarshalling the yaml files into CRD structs at run time. This replaces the previous process of using the custom po-crdgen command to generate CRDs and openapi-gen to generate schema directly in go source. The new process also ensures that the example CRD yaml files match what we are using in prometheus-operator at deploy/run time.

controller-gen also now handles the deepcopy generation and replaces the deepcopy-gen tool.

@brancz
Copy link
Contributor

brancz commented Nov 22, 2019

Overall lgtm, nice work! Happy to get rid of some of the tech debt we've build around this.

@s-urbaniak
Copy link
Contributor

@pgier the unit tests are spot on 👌
lgtm 🎉
Thanks for the hard work here!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants