Conversation
WalkthroughTooling and dependency versions bumped, CRD metadata annotations regenerated, webhook validators migrated from runtime.Object to typed parameters, controllers switched some resource updates from Patch to Apply, probe functions simplified, a validating webhook for BMCSecret added, and test binary versions bumped to 1.35.0. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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 🧪 Unit Test Generation v2 is now available!We have significantly improved our unit test generation capabilities. To enable: Add this to your reviews:
finishing_touches:
unit_tests:
enabled: trueTry it out by using the Have feedback? Share your thoughts on our Discord thread! 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}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@internal/controller/server_controller.go`:
- Around line 625-671: The OwnerReference construction using
bootConfig.APIVersion and bootConfig.Kind must be replaced with hardcoded values
to avoid SSA validation failures: create a single ownerRef (replace both
duplicated metav1apply.OwnerReference() blocks) that sets APIVersion to
"metal.ironcore.dev/v1alpha1" and Kind to "ServerBootConfiguration", keep the
WithName/WithUID/WithController/WithBlockOwnerDeletion calls as-is, and reuse
that ownerRef for both sshSecretApply and ignitionSecretApply to remove the
duplicated blocks.
In `@internal/webhook/v1alpha1/biossettings_webhook.go`:
- Around line 91-95: The error string mistakenly says "BMC" instead of "Server";
update the fmt.Errorf call that builds the duplicate reference message (the one
using settings.Spec.ServerRef.Name, settings.Name, bs.Spec.ServerRef.Name,
bs.Name) to refer to "Server" or "ServerRef" (e.g., "Server (%s) referred in %s
is duplicate of Server (%s) referred in %s") so the message correctly reflects
BIOSSettings' ServerRef usage.
🧹 Nitpick comments (2)
internal/webhook/v1alpha1/endpoint_webhook.go (1)
96-96: Inconsistent field path casing between create and update validations.Line 96 uses
"MACAddress"while Line 113 uses"macAddress". Consider aligning both to use consistent casing (typicallymacAddressto match JSON field naming conventions).🔧 Suggested fix
for _, e := range endpoints.Items { if e.Spec.MACAddress == spec.MACAddress { - allErrs = append(allErrs, field.Duplicate(field.NewPath("spec").Child("MACAddress"), e.Spec.MACAddress)) + allErrs = append(allErrs, field.Duplicate(field.NewPath("spec").Child("macAddress"), e.Spec.MACAddress)) } }Also applies to: 113-113
internal/webhook/v1alpha1/bmcversion_webhook.go (1)
75-92: RedundantGetcall - the object is already provided as parameter.The
ValidateDeletemethod fetches theBMCVersionviav.Client.Get(), but the admission request already provides the complete object inobj. This is unnecessary and could introduce race conditions if the object changes between the admission request and theGetcall.Other webhook files in this PR (e.g.,
biosversion_webhook.go,biossettings_webhook.go) directly useobj.Status.Statewithout re-fetching.♻️ Suggested simplification
func (v *BMCVersionCustomValidator) ValidateDelete(ctx context.Context, obj *metalv1alpha1.BMCVersion) (admission.Warnings, error) { bmcversionlog.Info("Validation for BMCVersion upon deletion", "name", obj.GetName()) - bv := &metalv1alpha1.BMCVersion{} - err := v.Client.Get(ctx, client.ObjectKey{ - Name: obj.GetName(), - Namespace: obj.GetNamespace(), - }, bv) - if err != nil { - return nil, fmt.Errorf("failed to get BMCVersion: %w", err) - } - - if bv.Status.State == metalv1alpha1.BMCVersionStateInProgress && !ShouldAllowForceDeleteInProgress(obj) { + if obj.Status.State == metalv1alpha1.BMCVersionStateInProgress && !ShouldAllowForceDeleteInProgress(obj) { return nil, apierrors.NewBadRequest("Unable to delete BMCVersion as it is in progress") } return nil, nil }
afritzler
force-pushed
the
enh/k8s-1.35
branch
from
e8a0270 to
f11b9e6
Compare
github-actions
bot
added
the
documentation
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/webhook/v1alpha1/endpoint_webhook.go (1)
86-114: Keep MAC address error paths consistent across create/update.The
ValidateMACAddressCreatefunction reportsspec.MACAddress(uppercase), whileValidateMACAddressUpdatereportsspec.macAddress(lowercase). Additionally, both functions ignore thepathparameter passed to them and create a new path instead. Usepath.Child("macAddress")in both functions to maintain consistency and properly utilize the path context.🔧 Proposed fix
- allErrs = append(allErrs, field.Duplicate(field.NewPath("spec").Child("MACAddress"), e.Spec.MACAddress)) + allErrs = append(allErrs, field.Duplicate(path.Child("macAddress"), e.Spec.MACAddress))- allErrs = append(allErrs, field.Duplicate(field.NewPath("spec").Child("macAddress"), e.Spec.MACAddress)) + allErrs = append(allErrs, field.Duplicate(path.Child("macAddress"), e.Spec.MACAddress))
🤖 Fix all issues with AI agents
In `@docs/api-reference/api.md`:
- Around line 452-455: The table contains bare URLs (e.g., the RFC link in the
`data` row and the Kubernetes links in the `type` row) that trigger markdownlint
MD034; update those bare URLs to use Markdown autolinks (wrap each URL in angle
brackets, e.g. <https://...>) so they are recognized as links by the linter
while preserving the existing text and links for the `data` and `type` table
rows.
In `@internal/webhook/v1alpha1/bmcsettings_webhook.go`:
- Around line 64-66: The apierrors.NewInvalid call constructs schema.GroupKind
using mixed sources (newObj.GroupVersionKind().Group and oldObj.Kind); change it
so both Group and Kind come from the same object (use newObj). Replace the Kind
argument (oldObj.Kind) with newObj.GroupVersionKind().Kind (or newObj's
equivalent) in the schema.GroupKind constructor inside the return statement in
bmcsettings_webhook.go where NewInvalid is called.
🧹 Nitpick comments (1)
internal/webhook/v1alpha1/bmcversion_webhook.go (1)
78-89: Redundant API fetch in ValidateDelete.The
objparameter already contains the BMCVersion being deleted. Re-fetching it from the API server (lines 78-85) is unnecessary, adds latency, and introduces a potential race condition if the object state changes between the admission request and the re-fetch.Compare with
bmcsettings_webhook.gowhich correctly usesobjdirectly:if obj.Status.State == metalv1alpha1.BMCSettingsStateInProgress && !ShouldAllowForceDeleteInProgress(obj) {Proposed fix to use obj directly
func (v *BMCVersionCustomValidator) ValidateDelete(ctx context.Context, obj *metalv1alpha1.BMCVersion) (admission.Warnings, error) { bmcversionlog.Info("Validation for BMCVersion upon deletion", "name", obj.GetName()) - bv := &metalv1alpha1.BMCVersion{} - err := v.Client.Get(ctx, client.ObjectKey{ - Name: obj.GetName(), - Namespace: obj.GetNamespace(), - }, bv) - if err != nil { - return nil, fmt.Errorf("failed to get BMCVersion: %w", err) - } - - if bv.Status.State == metalv1alpha1.BMCVersionStateInProgress && !ShouldAllowForceDeleteInProgress(obj) { + if obj.Status.State == metalv1alpha1.BMCVersionStateInProgress && !ShouldAllowForceDeleteInProgress(obj) { return nil, apierrors.NewBadRequest("Unable to delete BMCVersion as it is in progress") } return nil, nil }
afritzler
force-pushed
the
enh/k8s-1.35
branch
from
f11b9e6 to
1ecd99a
Compare
afritzler
force-pushed
the
enh/k8s-1.35
branch
from
5824705 to
c890911
Compare
xkonni
left a comment
xkonni
left a comment
There was a problem hiding this comment.
tried it, for once even the tests passed. looks good to me.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@Makefile`:
- Around line 274-279: The Makefile sets GOLANGCI_LINT_VERSION ?= v2.8 which may
be built with Go 1.24 and incompatible with the repo's Go 1.25; update
GOLANGCI_LINT_VERSION in the Makefile to a golangci-lint release built with Go
1.25 (or a known compatible version) or add a comment/verification step in the
Makefile/CI that confirms v2.8 is compatible; edit the GOLANGCI_LINT_VERSION
variable and any CI/install targets that consume it (search for
GOLANGCI_LINT_VERSION) to pin a compatible release or to perform an explicit
compatibility check.
🧹 Nitpick comments (3)
internal/webhook/v1alpha1/bmcversion_webhook.go (1)
75-92: RedundantGetcall inValidateDelete.The
objparameter already contains the completeBMCVersionobject being deleted, including itsStatus. TheGetcall on lines 78-85 re-fetches the same object, adding unnecessary latency. This also introduces a subtle inconsistency:bv.Status.Statecomes from the re-fetched object whileShouldAllowForceDeleteInProgress(obj)checks annotations on the originalobj.Compare with
biosversion_webhook.go(lines 76-83) andbiossettings_webhook.go(lines 75-83), which useobj.Statusdirectly without re-fetching.Proposed fix
func (v *BMCVersionCustomValidator) ValidateDelete(ctx context.Context, obj *metalv1alpha1.BMCVersion) (admission.Warnings, error) { bmcversionlog.Info("Validation for BMCVersion upon deletion", "name", obj.GetName()) - bv := &metalv1alpha1.BMCVersion{} - err := v.Client.Get(ctx, client.ObjectKey{ - Name: obj.GetName(), - Namespace: obj.GetNamespace(), - }, bv) - if err != nil { - return nil, fmt.Errorf("failed to get BMCVersion: %w", err) - } - - if bv.Status.State == metalv1alpha1.BMCVersionStateInProgress && !ShouldAllowForceDeleteInProgress(obj) { + if obj.Status.State == metalv1alpha1.BMCVersionStateInProgress && !ShouldAllowForceDeleteInProgress(obj) { return nil, apierrors.NewBadRequest("Unable to delete BMCVersion as it is in progress") } return nil, nil }internal/webhook/v1alpha1/endpoint_webhook.go (1)
94-98: Inconsistent field path casing betweenValidateMACAddressCreateandValidateMACAddressUpdate.
ValidateMACAddressCreateuses"MACAddress"(PascalCase) on line 96, whileValidateMACAddressUpdateuses"macAddress"(camelCase) on line 113. For consistency and alignment with Kubernetes JSON field naming conventions (typically camelCase), both should use the same casing.Proposed fix
for _, e := range endpoints.Items { if e.Spec.MACAddress == spec.MACAddress { - allErrs = append(allErrs, field.Duplicate(field.NewPath("spec").Child("MACAddress"), e.Spec.MACAddress)) + allErrs = append(allErrs, field.Duplicate(field.NewPath("spec").Child("macAddress"), e.Spec.MACAddress)) } }Also applies to: 111-114
internal/webhook/v1alpha1/bmcsecret_webhook.go (1)
34-36: UnusedClientfield in validator struct.The
Clientfield is initialized viamgr.GetClient()but never used in any of the validation methods. If this is intentional for future expansion, consider adding a brief comment. Otherwise, consider removing it to avoid dead code.
3 participants
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.