Conversation
WalkthroughAdds several new public fields (e.g., ServerMaintenancePolicy, ServerSelector, BMCUserStatus fields), many CRD shortNames, small CRD schema adjustments (descriptions, a few required/validation entries), and widespread comment/description standardization across API types and CRD manifests. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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: 3
🤖 Fix all issues with AI agents
In `@api/v1alpha1/biossettingsset_types.go`:
- Line 38: Two CRDs share the same kubebuilder shortName "bmcs" (BMCSettings and
BMCSecret), causing a collision; pick one (either the BMCSettings type in
bmcsettings_types.go / type BMCSettings or the BMCSecret type in
bmcsecret_types.go / type BMCSecret) and change its kubebuilder:resource
shortName to a unique value (e.g., "bmcsec" or similar) so it no longer
conflicts, updating the +kubebuilder:resource:scope=... annotation for that type
and regenerating CRD manifests so the new shortName is applied consistently
(note BiossettingsSet already uses "bss" and can remain unchanged).
In `@api/v1alpha1/bmcsettings_types.go`:
- Line 78: The CRD shortName collision between BMCSettings and BMCSecret (both
using shortName=bmcs) will break CRD registration; update the
kubebuilder:resource annotations to use distinct short names (e.g., change the
annotation on the BMCSettings type declaration in
api/v1alpha1/bmcsettings_types.go to shortName=bmcset and change the annotation
on the BMCSecret type declaration in api/v1alpha1/bmcsecret_types.go to
shortName=bmcsec), then regenerate CRDs/manifests so the new unique short names
take effect.
In `@api/v1alpha1/bmcversion_types.go`:
- Line 74: The printcolumn annotation references a non-existent field
`.spec.updateType`; update it to reference the actual spec field by changing the
kubebuilder printcolumn JSONPath to `.spec.updatePolicy` to match the
BMCVersionSpec's UpdatePolicy (JSON tag `updatePolicy`) so the column shows the
correct value; locate the annotation line with the comment `//
+kubebuilder:printcolumn:name="updateType"...` and update the JSONPath/name as
appropriate to reflect `updatePolicy`.
afritzler
force-pushed
the
chore/shortnames
branch
from
c660531 to
4db1bf3
Compare
github-actions
bot
added
the
documentation
afritzler
force-pushed
the
chore/shortnames
branch
from
4db1bf3 to
8d71133
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
config/crd/bases/metal.ironcore.dev_bmcsettingssets.yaml (1)
1-175:⚠️ Potential issue | 🟡 MinorAdd
shortNamesto BMCSettingsSet and other BMC-related CRDs for consistency.BMCSettingsSet lacks a
shortNamesfield, inconsistent with BIOSSettings, BIOSSettingsSet, ServerMaintenance, and other CRDs in the codebase. Additionally, BMCSettings and BMCs CRDs also lackshortNames. Consider adding short name aliases across all BMC-related resources (e.g.,bmcssfor BMCSettingsSet,bmcsfor BMCSettings, etc.) to match the established pattern throughout the API group.
🧹 Nitpick comments (1)
config/crd/bases/metal.ironcore.dev_biosversions.yaml (1)
100-103: Add enum validation forserverMaintenancePolicyin the Go types.This field accepts exactly two stable values (
OwnerApprovalandEnforced), which are already defined as constants inapi/v1alpha1/servermaintenance_types.go. Adding a+kubebuilder:validation:Enummarker on theServerMaintenancePolicytype definition would enforce valid values at admission time and align with similar patterns already used in the codebase (e.g.,ProtocolNamein bmc_types.go). This would catch invalid policy values early rather than allowing arbitrary strings.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
api/v1alpha1/bmcsettingsset_types.go (1)
37-39:⚠️ Potential issue | 🟠 MajorAdd missing
shortNameforBMCSettingsSet.The PR's objective is to add short names to metal resources. The sibling
BIOSSettingsSethasshortName=bss, andBMCSettingsSetshould have a corresponding short name. Note thatBMCSettingsis also missing a short name and should be addressed similarly.Suggested fix
-// +kubebuilder:resource:scope=Cluster +// +kubebuilder:resource:scope=Cluster,shortName=bmcssconfig/crd/bases/metal.ironcore.dev_bmcsettings.yaml (1)
122-125:⚠️ Potential issue | 🟡 MinorAdding
versiontorequiredis a validation tightening — verify no existing resources omit it.Previously only
BMCRefwas required. Addingversionto the required list means existingBMCSettingsresources that lack aversionfield will now fail validation on create/update. While this is acceptable for an alpha API, confirm that all existing consumers already set this field.#!/bin/bash # Check if version was previously required or if this is a new addition git diff HEAD~1 -- config/crd/bases/metal.ironcore.dev_bmcsettings.yaml | grep -A5 -B5 'required'
🧹 Nitpick comments (1)
api/v1alpha1/bmcsettingsset_types.go (1)
13-14: Pre-existing:+requiredmarker contradictsomitemptyJSON tag.
BMCSettingsTemplateis marked+requiredbut the JSON tag includesomitempty, which tells the serializer to drop the field when zero-valued. For required fields the convention is to omitomitempty. This is pre-existing and not introduced by this PR, but worth cleaning up.
afritzler
force-pushed
the
chore/shortnames
branch
from
8d71133 to
f4db668
Compare
|
@coderabbitai summary |
✅ Actions performedSummary regeneration triggered. |
3 participants
Summary by CodeRabbit
New Features
Documentation
Schema