Kubebuilder update from v4.11.1 to v4.12.0#686
Merged
Conversation
github-actions
bot
added
size/XXL
documentation
Contributor
|
No actionable comments were generated in the recent review. 🎉 WalkthroughKubebuilder/project tooling bumped to v4.12.0; DevContainer and post-install tooling added; golangci-lint configured to build a custom binary with a logcheck plugin; large refactor removing explicit Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/main.go (1)
575-597:⚠️ Potential issue | 🟠 Major
ctrl.SetupSignalHandler()is called twice — use the existing context.Line 575 creates
ctxfromctrl.SetupSignalHandler(), then line 596 callsctrl.SetupSignalHandler()again. While controller-runtime usessync.Onceinternally (so it won't panic), this is confusing and redundant. Reusectxfrom line 575.Proposed fix
setupLog.Info("Starting manager") - if err := mgr.Start(ctrl.SetupSignalHandler()); err != nil { + if err := mgr.Start(ctx); err != nil { setupLog.Error(err, "Failed to run manager")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/main.go` around lines 575 - 597, The code calls ctrl.SetupSignalHandler() twice creating two contexts; reuse the earlier ctx instead of creating a new one. Update the mgr.Start(...) call to pass the existing ctx variable (created at the top) rather than calling ctrl.SetupSignalHandler() again so the manager and the registry RunnableFunc use the same cancellation context (symbols: ctx, mgr.Start, manager.RunnableFunc, registry.NewServer, registryServer.Start).
🧹 Nitpick comments (3)
cmd/main.go (1)
514-563: Consolidate repeatedENABLE_WEBHOOKSchecks into a single block.Seven identical
if os.Getenv("ENABLE_WEBHOOKS") != "false"checks (each with// nolint:goconst) can be consolidated into one. This is likely scaffolded code, but since the project already customizes cmd/main.go, collapsing them would improve readability.Proposed refactor
+ if os.Getenv("ENABLE_WEBHOOKS") != "false" { // nolint:goconst + if err := webhookv1alpha1.SetupEndpointWebhookWithManager(mgr); err != nil { + setupLog.Error(err, "Failed to create webhook", "webhook", "Endpoint") + os.Exit(1) + } + if err := webhookv1alpha1.SetupBMCSecretWebhookWithManager(mgr); err != nil { + setupLog.Error(err, "Failed to create webhook", "webhook", "BMCSecret") + os.Exit(1) + } + if err := webhookv1alpha1.SetupServerWebhookWithManager(mgr); err != nil { + setupLog.Error(err, "Failed to create webhook", "webhook", "Server") + os.Exit(1) + } + if err := webhookv1alpha1.SetupBIOSSettingsWebhookWithManager(mgr); err != nil { + setupLog.Error(err, "Failed to create webhook", "webhook", "BIOSSettings") + os.Exit(1) + } + if err := webhookv1alpha1.SetupBIOSVersionWebhookWithManager(mgr); err != nil { + setupLog.Error(err, "Failed to create webhook", "webhook", "BIOSVersion") + os.Exit(1) + } + if err := webhookv1alpha1.SetupBMCSettingsWebhookWithManager(mgr); err != nil { + setupLog.Error(err, "Failed to create webhook", "webhook", "BMCSettings") + os.Exit(1) + } + if err := webhookv1alpha1.SetupBMCVersionWebhookWithManager(mgr); err != nil { + setupLog.Error(err, "Failed to create webhook", "webhook", "BMCVersion") + os.Exit(1) + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/main.go` around lines 514 - 563, The repeated os.Getenv("ENABLE_WEBHOOKS") != "false" checks should be collapsed into a single check: read the env once (e.g., enableWebhooks := os.Getenv("ENABLE_WEBHOOKS")), then if enableWebhooks != "false" call each webhook setup in sequence (webhookv1alpha1.SetupEndpointWebhookWithManager, SetupBMCSecretWebhookWithManager, SetupServerWebhookWithManager, SetupBIOSSettingsWebhookWithManager, SetupBIOSVersionWebhookWithManager, SetupBMCSettingsWebhookWithManager, SetupBMCVersionWebhookWithManager) using the existing mgr and error handling (setupLog.Error(...) and os.Exit(1)) for each call; remove the duplicated // nolint:goconst comments after consolidation and keep the same contextual log keys ("webhook", "<Name>") for each error..devcontainer/post-install.sh (1)
50-73: Unpinned tool versions (latest) reduce build reproducibility.Both
kindandkubebuilderare installed fromlatestURLs. While acceptable for a dev container, this means builds may break silently when upstream releases introduce breaking changes. Consider pinning to specific versions (possibly derived from theMakefileorPROJECTfile) for consistency with the rest of the toolchain.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.devcontainer/post-install.sh around lines 50 - 73, The script installs kind and kubebuilder using unpinned "latest" URLs which hurts reproducibility; update the post-install.sh logic to use explicit version variables (e.g., KIND_VERSION and KUBEBUILDER_VERSION) instead of "latest" when building the download URLs for curl, sourcing those values from the repo canonical source (Makefile, PROJECT, or a new .versions file) and fall back to the existing ARCH variable if needed; update the download lines that reference "https://kind.sigs.k8s.io/dl/latest/kind-linux-${ARCH}" and "https://go.kubebuilder.io/dl/latest/linux/${ARCH}" to use the version variables (and update any echo messages to reflect the chosen version) so CI/devcontainers use pinned tool versions consistent with the toolchain..custom-gcl.yml (1)
11-11: Consider pinning the logcheck plugin version instead oflatest.Using
version: latestmeans the plugin version resolved duringmake lintcan change without any change to this file, potentially causing non-reproducible lint results or unexpected breakage. Pinning to a specific release tag (e.g.,v0.9.1) would improve reproducibility.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.custom-gcl.yml at line 11, The manifest uses an unstable selector "version: latest" in .custom-gcl.yml which makes lint plugin resolution non-reproducible; change that value to a specific released tag (for example replace version: latest with version: v0.9.1 or the current tested release) so the logcheck plugin version is pinned and future make lint runs are deterministic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.devcontainer/devcontainer.json:
- Around line 19-21: Replace the deprecated terminal.integrated.shell.linux
setting by adding a terminal.integrated.profiles.linux entry that defines a
profile (e.g., "bash") pointing to /bin/bash and then set
terminal.integrated.defaultProfile.linux to that profile name; update or remove
terminal.integrated.shell.linux and ensure the new profile key and
defaultProfile reference match the profile identifier used so VS Code uses
Terminal Profiles instead of the deprecated setting.
In @.devcontainer/post-install.sh:
- Around line 116-127: The Docker readiness loop currently only prints "WARNING:
Docker not ready after 30s" when the retry limit is reached, which can lead to
confusing failures later; update the for-loop block that checks docker info to
exit non-zero on failure by adding an explicit exit 1 immediately after the
warning so the script terminates cleanly instead of continuing to commands like
docker network create.
In `@AGENTS.md`:
- Line 234: The markdown line has missing spaces around the inline code token
`--force`; update the sentence so the inline code is surrounded by spaces like:
When `--force` is used — ensure the text around the inline code (`--force`)
includes a leading and trailing space so it renders correctly.
- Line 6: Add explicit language specifiers to the fenced code blocks in
AGENTS.md to satisfy MD040: replace plain triple-backtick blocks with
language-tagged ones (e.g., use ```text for directory/listing blocks and ```bash
or ```shell for command blocks). Update the specific code fences flagged
(including the block at the shown location and also those at the other flagged
occurrences) so each opening ``` has an appropriate language identifier to
ensure static analysis passes.
- Line 230: Update the external Kubernetes community links that reference the
branch name "master" by replacing them with permalink URLs that include a
concrete commit SHA (e.g., change
"https://github.com/kubernetes/community/blob/master/..." to
"https://github.com/kubernetes/community/blob/<commit-sha>/...") so CI
inclusive-language checks stop flagging them; alternatively, if you prefer not
to pin to a SHA, add an exception rule in the CI inclusive-language check for
external URLs or specifically allow these kubernetes/community links. Ensure you
update every occurrence mentioned (the URL in AGENTS.md and the other
occurrences referenced in the comment) so all CI warnings are silenced.
In `@cmd/main.go`:
- Around line 486-506: Duplicate registration of BIOSSettingsSetReconciler
causes startup failure; remove the repeated controller registration block so
BIOSSettingsSetReconciler is only registered once. Locate the two identical
SetupWithManager calls that construct
&controller.BIOSSettingsSetReconciler{Client: mgr.GetClient(), Scheme:
mgr.GetScheme()} and delete one of them (keep the BMCVersionSetReconciler
registration intact), ensuring only a single call to
BIOSSettingsSetReconciler.SetupWithManager(mgr) remains and related
setupLog.Error/os.Exit handling is preserved for the remaining block.
In `@Makefile`:
- Line 285: Update the KUBEBUILDER_VERSION variable to match the scaffolding
version used: change the KUBEBUILDER_VERSION value from v4.11.1 to v4.12.0 (the
Makefile variable KUBEBUILDER_VERSION should be set to v4.12.0 so `make
kubebuilder` installs the same Kubebuilder version used to regenerate the
project).
In `@test/e2e/e2e_test.go`:
- Around line 230-231: Replace the hard-coded time.Sleep(5 * time.Second) after
the "waiting additional time for webhook server to stabilize" step with a
deterministic polling/Eventually check: remove the sleep and instead use an
Eventually-based loop (or a small retry helper) that repeatedly calls the
webhook readiness/health endpoint or sends a probe request until it returns the
expected success response (HTTP 200 or specific JSON) within a reasonable
timeout and interval; update the test in test/e2e/e2e_test.go to call that probe
(or implement a helper like waitForWebhookReady) so the test no longer relies on
a fixed wait and becomes deterministic.
---
Outside diff comments:
In `@cmd/main.go`:
- Around line 575-597: The code calls ctrl.SetupSignalHandler() twice creating
two contexts; reuse the earlier ctx instead of creating a new one. Update the
mgr.Start(...) call to pass the existing ctx variable (created at the top)
rather than calling ctrl.SetupSignalHandler() again so the manager and the
registry RunnableFunc use the same cancellation context (symbols: ctx,
mgr.Start, manager.RunnableFunc, registry.NewServer, registryServer.Start).
---
Nitpick comments:
In @.custom-gcl.yml:
- Line 11: The manifest uses an unstable selector "version: latest" in
.custom-gcl.yml which makes lint plugin resolution non-reproducible; change that
value to a specific released tag (for example replace version: latest with
version: v0.9.1 or the current tested release) so the logcheck plugin version is
pinned and future make lint runs are deterministic.
In @.devcontainer/post-install.sh:
- Around line 50-73: The script installs kind and kubebuilder using unpinned
"latest" URLs which hurts reproducibility; update the post-install.sh logic to
use explicit version variables (e.g., KIND_VERSION and KUBEBUILDER_VERSION)
instead of "latest" when building the download URLs for curl, sourcing those
values from the repo canonical source (Makefile, PROJECT, or a new .versions
file) and fall back to the existing ARCH variable if needed; update the download
lines that reference "https://kind.sigs.k8s.io/dl/latest/kind-linux-${ARCH}" and
"https://go.kubebuilder.io/dl/latest/linux/${ARCH}" to use the version variables
(and update any echo messages to reflect the chosen version) so CI/devcontainers
use pinned tool versions consistent with the toolchain.
In `@cmd/main.go`:
- Around line 514-563: The repeated os.Getenv("ENABLE_WEBHOOKS") != "false"
checks should be collapsed into a single check: read the env once (e.g.,
enableWebhooks := os.Getenv("ENABLE_WEBHOOKS")), then if enableWebhooks !=
"false" call each webhook setup in sequence
(webhookv1alpha1.SetupEndpointWebhookWithManager,
SetupBMCSecretWebhookWithManager, SetupServerWebhookWithManager,
SetupBIOSSettingsWebhookWithManager, SetupBIOSVersionWebhookWithManager,
SetupBMCSettingsWebhookWithManager, SetupBMCVersionWebhookWithManager) using the
existing mgr and error handling (setupLog.Error(...) and os.Exit(1)) for each
call; remove the duplicated // nolint:goconst comments after consolidation and
keep the same contextual log keys ("webhook", "<Name>") for each error.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/main.go (1)
522-571: Consider consolidating the repeatedENABLE_WEBHOOKSguard into a single block.The same
os.Getenv("ENABLE_WEBHOOKS") != "false"check is repeated 7 times, each suppressed with// nolint:goconst. This can be collapsed into one conditional containing all webhook registrations, which eliminates the need for the nolint directives entirely.That said, I understand this may be the Kubebuilder scaffold pattern (each webhook gets its own guarded block for the
// +kubebuilder:scaffold:buildermarker). If you plan to keep accepting scaffold output, feel free to leave as-is.♻️ Suggested consolidation
- // nolint:goconst - if os.Getenv("ENABLE_WEBHOOKS") != "false" { - if err := webhookv1alpha1.SetupEndpointWebhookWithManager(mgr); err != nil { - setupLog.Error(err, "Failed to create webhook", "webhook", "Endpoint") - os.Exit(1) - } - } - // nolint:goconst - if os.Getenv("ENABLE_WEBHOOKS") != "false" { - if err := webhookv1alpha1.SetupBMCSecretWebhookWithManager(mgr); err != nil { - setupLog.Error(err, "Failed to create webhook", "webhook", "BMCSecret") - os.Exit(1) - } - } - // nolint:goconst - if os.Getenv("ENABLE_WEBHOOKS") != "false" { - if err := webhookv1alpha1.SetupServerWebhookWithManager(mgr); err != nil { - setupLog.Error(err, "Failed to create webhook", "webhook", "Server") - os.Exit(1) - } - } - // nolint:goconst - if os.Getenv("ENABLE_WEBHOOKS") != "false" { - if err := webhookv1alpha1.SetupBIOSSettingsWebhookWithManager(mgr); err != nil { - setupLog.Error(err, "Failed to create webhook", "webhook", "BIOSSettings") - os.Exit(1) - } - } - // nolint:goconst - if os.Getenv("ENABLE_WEBHOOKS") != "false" { - if err := webhookv1alpha1.SetupBIOSVersionWebhookWithManager(mgr); err != nil { - setupLog.Error(err, "Failed to create webhook", "webhook", "BIOSVersion") - os.Exit(1) - } - } - // nolint:goconst - if os.Getenv("ENABLE_WEBHOOKS") != "false" { - if err := webhookv1alpha1.SetupBMCSettingsWebhookWithManager(mgr); err != nil { - setupLog.Error(err, "Failed to create webhook", "webhook", "BMCSettings") - os.Exit(1) - } - } - // nolint:goconst - if os.Getenv("ENABLE_WEBHOOKS") != "false" { - if err := webhookv1alpha1.SetupBMCVersionWebhookWithManager(mgr); err != nil { - setupLog.Error(err, "Failed to create webhook", "webhook", "BMCVersion") - os.Exit(1) - } - } + if os.Getenv("ENABLE_WEBHOOKS") != "false" { + webhookSetups := []struct { + name string + fn func(manager.Manager) error + }{ + {"Endpoint", webhookv1alpha1.SetupEndpointWebhookWithManager}, + {"BMCSecret", webhookv1alpha1.SetupBMCSecretWebhookWithManager}, + {"Server", webhookv1alpha1.SetupServerWebhookWithManager}, + {"BIOSSettings", webhookv1alpha1.SetupBIOSSettingsWebhookWithManager}, + {"BIOSVersion", webhookv1alpha1.SetupBIOSVersionWebhookWithManager}, + {"BMCSettings", webhookv1alpha1.SetupBMCSettingsWebhookWithManager}, + {"BMCVersion", webhookv1alpha1.SetupBMCVersionWebhookWithManager}, + } + for _, ws := range webhookSetups { + if err := ws.fn(mgr); err != nil { + setupLog.Error(err, "Failed to create webhook", "webhook", ws.name) + os.Exit(1) + } + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/main.go` around lines 522 - 571, The repeated os.Getenv("ENABLE_WEBHOOKS") != "false" guard is duplicated around each webhook registration; consolidate by evaluating ENABLE_WEBHOOKS once and, if true, call webhookv1alpha1.SetupEndpointWebhookWithManager, SetupBMCSecretWebhookWithManager, SetupServerWebhookWithManager, SetupBIOSSettingsWebhookWithManager, SetupBIOSVersionWebhookWithManager, SetupBMCSettingsWebhookWithManager and SetupBMCVersionWebhookWithManager inside a single if block so you remove the repeated nolint:goconst comments and avoid redundant checks; if Kubebuilder scaffold markers require separate blocks, instead move a single boolean like enableWebhooks := os.Getenv("ENABLE_WEBHOOKS") != "false" above and use that variable in each existing if to eliminate repeated environment lookups while preserving scaffold placement.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmd/main.go`:
- Around line 522-571: The repeated os.Getenv("ENABLE_WEBHOOKS") != "false"
guard is duplicated around each webhook registration; consolidate by evaluating
ENABLE_WEBHOOKS once and, if true, call
webhookv1alpha1.SetupEndpointWebhookWithManager,
SetupBMCSecretWebhookWithManager, SetupServerWebhookWithManager,
SetupBIOSSettingsWebhookWithManager, SetupBIOSVersionWebhookWithManager,
SetupBMCSettingsWebhookWithManager and SetupBMCVersionWebhookWithManager inside
a single if block so you remove the repeated nolint:goconst comments and avoid
redundant checks; if Kubebuilder scaffold markers require separate blocks,
instead move a single boolean like enableWebhooks :=
os.Getenv("ENABLE_WEBHOOKS") != "false" above and use that variable in each
existing if to eliminate repeated environment lookups while preserving scaffold
placement.
asergeant01
approved these changes
Feb 17, 2026
| } | ||
|
|
||
| // nolint:goconst | ||
| if os.Getenv("ENABLE_WEBHOOKS") != "false" { |
Contributor
There was a problem hiding this comment.
Do we need all the os.Getenv("ENABLE_WEBHOOKS") != "false" blocks. We could have just one right?
Member
Author
There was a problem hiding this comment.
If we drop that, we will run into merge conflicts in case we add new webhooks as this will violate the kubebuilder template.
This was referenced Feb 18, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Chores
Refactor
Tests