Use codegen to produce CRDs#28
Conversation
I tried to keep the difference in descriptions of fields to the minimum. I advise to review the change to CRD file with (-w, ignore whitespace) option enabled. Signed-off-by: Ihar Hrachyshka <ihrachyshka@nvidia.com> Assisted-By: Claude Code; opus (claude-opus-4-5-20251101)
Summary of ChangesHello @booxter, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the Custom Resource Definition (CRD) generation process by integrating Highlights
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces code generation for the CRD schema using controller-gen, which is a significant improvement for maintainability. The changes include adding a generation script, updating Go type definitions with kubebuilder markers and detailed comments, and updating the README.md. My review focuses on making the code generation process more robust and ensuring the CRD versioning is correctly configured. I've identified a critical issue with the storage version configuration that could impact cluster operations and data persistence. Additionally, I've suggested improvements to the generation script to use pinned tool versions for reproducible builds, and enhanced some code comments for better clarity.
| # Determine GOBIN | ||
| GOBIN="${GOBIN:-$(go env GOPATH)/bin}" | ||
|
|
||
| # Install controller-gen if not present | ||
| if ! command -v controller-gen &> /dev/null && [ ! -f "${GOBIN}/controller-gen" ]; then | ||
| echo "Installing controller-gen..." | ||
| go install sigs.k8s.io/controller-tools/cmd/controller-gen@latest | ||
| fi | ||
|
|
||
| # Use controller-gen from GOBIN if not in PATH | ||
| CONTROLLER_GEN="${GOBIN}/controller-gen" | ||
| if command -v controller-gen &> /dev/null; then | ||
| CONTROLLER_GEN="controller-gen" | ||
| fi | ||
|
|
||
| cd "${SCRIPT_ROOT}" | ||
|
|
||
| # Generate CRD manifests | ||
| echo "Generating CRD manifests..." | ||
| "${CONTROLLER_GEN}" crd \ | ||
| paths="./pkg/apis/k8s.cni.cncf.io/..." \ | ||
| output:crd:artifacts:config=. |
There was a problem hiding this comment.
To ensure reproducible builds, it's better to use the vendored version of controller-gen via go run instead of installing the @latest version. This avoids unexpected changes in the generated code due to tool updates. This change assumes controller-gen has been added to tools.go as suggested in a separate comment.
| # Determine GOBIN | |
| GOBIN="${GOBIN:-$(go env GOPATH)/bin}" | |
| # Install controller-gen if not present | |
| if ! command -v controller-gen &> /dev/null && [ ! -f "${GOBIN}/controller-gen" ]; then | |
| echo "Installing controller-gen..." | |
| go install sigs.k8s.io/controller-tools/cmd/controller-gen@latest | |
| fi | |
| # Use controller-gen from GOBIN if not in PATH | |
| CONTROLLER_GEN="${GOBIN}/controller-gen" | |
| if command -v controller-gen &> /dev/null; then | |
| CONTROLLER_GEN="controller-gen" | |
| fi | |
| cd "${SCRIPT_ROOT}" | |
| # Generate CRD manifests | |
| echo "Generating CRD manifests..." | |
| "${CONTROLLER_GEN}" crd \ | |
| paths="./pkg/apis/k8s.cni.cncf.io/..." \ | |
| output:crd:artifacts:config=. | |
| cd "${SCRIPT_ROOT}" | |
| # Generate CRD manifests | |
| echo "Generating CRD manifests..." | |
| go run sigs.k8s.io/controller-tools/cmd/controller-gen crd \ | |
| paths="./pkg/apis/k8s.cni.cncf.io/..." \ | |
| output:crd:artifacts:config=. |
| _ "k8s.io/code-generator" | ||
| _ "k8s.io/code-generator/cmd/client-gen" | ||
| _ "k8s.io/code-generator/cmd/deepcopy-gen" | ||
| _ "k8s.io/code-generator/cmd/defaulter-gen" | ||
| _ "k8s.io/code-generator/cmd/informer-gen" | ||
| _ "k8s.io/code-generator/cmd/lister-gen" | ||
| _ "k8s.io/kube-openapi/cmd/openapi-gen" |
There was a problem hiding this comment.
For reproducible builds, it's best to vendor the controller-gen tool and pin its version in go.mod. Please add it to your tool dependencies. This will allow using go run in the generation script, avoiding the need to install it globally or fetch @latest.
_ "k8s.io/code-generator"
_ "k8s.io/code-generator/cmd/client-gen"
_ "k8s.io/code-generator/cmd/deepcopy-gen"
_ "k8s.io/code-generator/cmd/defaulter-gen"
_ "k8s.io/code-generator/cmd/informer-gen"
_ "k8s.io/code-generator/cmd/lister-gen"
_ "k8s.io/kube-openapi/cmd/openapi-gen"
_ "sigs.k8s.io/controller-tools/cmd/controller-gen"
zeeke
left a comment
zeeke
left a comment
There was a problem hiding this comment.
LGTM
I prefer to have another opinion here, as the project is used by multiple implementations (iptables, nftables, ovnk, ...)
@dougbtv @s1061123 @bpickard22
2 participants
I tried to keep the difference in descriptions of fields to the minimum.
I advise to review the change to CRD file with (-w, ignore whitespace)
option enabled.