Spec migration for BIOS/BMC Settings/Version resources#957
Open
nagadeesh-nagaraja wants to merge 5 commits into
Open
Spec migration for BIOS/BMC Settings/Version resources#957nagadeesh-nagaraja wants to merge 5 commits into
nagadeesh-nagaraja wants to merge 5 commits into
Conversation
Signed-off-by: Nagadeesh Nagaraja <nagdeesh@gmail.com>
Contributor
📝 WalkthroughWalkthroughIntroduces a ChangesDriftPolicy, SettingsFlow, and Multi-Ref Migration
Sequence Diagram(s)sequenceDiagram
participant User
participant Webhook
participant Reconciler
participant K8sAPI
User->>Webhook: Create/Update BMCSettings or BIOSSettings
Webhook->>Webhook: validateDriftPolicy (reject if no owner refs)
Webhook->>K8sAPI: List existing resources
K8sAPI-->>Webhook: Existing list
Webhook->>Webhook: checkDuplicate (serverRef/bmcRef + version)
Webhook-->>User: Admit or reject
Reconciler->>K8sAPI: Fetch BMC/Server object
K8sAPI-->>Reconciler: spec.BMCSettingRef (deprecated) or spec.BMCSettingsRefs
alt deprecated single-ref found
Reconciler->>Reconciler: Migrate into BMCSettingsRefs, clear BMCSettingRef
Reconciler->>K8sAPI: Patch BMC/Server (migration)
K8sAPI-->>Reconciler: Requeue
else spec.settings (flat map) found
Reconciler->>Reconciler: Migrate into spec.settingsFlow, clear spec.settings
Reconciler->>K8sAPI: Patch BMCSettings (migration)
K8sAPI-->>Reconciler: Requeue
else already canonical
Reconciler->>Reconciler: containsRef check on BMCSettingsRefs
Reconciler->>K8sAPI: Patch ref list (add/remove)
Reconciler->>Reconciler: flattenSettingsFlow → compute diff
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
config/crd/bases/metal.ironcore.dev_biossettings.yaml (1)
122-150:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEnforce unique
settingsFlow.priorityto keep precedence deterministic.
settingsFlowis order-defining, but the schema doesn’t prevent duplicate priorities. With priority-based flatten/merge, equal priorities make effective override order ambiguous. Add a CEL uniqueness rule forpriority(optionallynametoo).Suggested schema rule
x-kubernetes-validations: - message: version is required on BIOSSettings rule: size(self.version) > 0 - message: serverRef is required rule: has(self.serverRef) + - message: settingsFlow priorities must be unique + rule: '!has(self.settingsFlow) || self.settingsFlow.all(i, self.settingsFlow.filter(j, j.priority == i.priority).size() == 1)'As per coding guidelines,
config/crd/bases/*.yamlis generated; apply this in API markers/types and regenerate withmake manifests.Also applies to: 158-162
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@config/crd/bases/metal.ironcore.dev_biossettings.yaml` around lines 122 - 150, The settingsFlow array in the BIOSSettings CRD schema allows duplicate priority values, making the order of applying settings ambiguous when priorities are equal. Since the CRD file is generated from API type definitions, locate the BIOSSettings API type definition (not in config/crd/bases) and add a CEL validation rule using kubebuilder markers to enforce that the priority field must be unique across all items in the settingsFlow array. After adding the validation marker, regenerate the CRD manifest by running make manifests to update the generated YAML file.Source: Coding guidelines
config/crd/bases/metal.ironcore.dev_bmcs.yaml (1)
103-124:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
bmcSettingsRefsListand deprecation guidance are inconsistent.The deprecated field instructs users to migrate to
bmcSettingsRefs, but the actual new schema property isbmcSettingsRefsList. This creates an API contract mismatch for clients/manifests. Pick one canonical field name and align API types, CRD schema, and migration guidance consistently.As per coding guidelines, this CRD is generated; fix the source API field/markers and regenerate manifests.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@config/crd/bases/metal.ironcore.dev_bmcs.yaml` around lines 103 - 124, The deprecation message in the bmcSettingsRef field states to use bmcSettingsRefs instead, but the actual new field in the CRD is named bmcSettingsRefsList, creating an inconsistency in the API contract. Locate the source API type definition (not this generated CRD yaml file) that defines the BMC structure and the bmcSettingsRef field with its deprecation marker. Update either the deprecation guidance comment to reference the correct field name bmcSettingsRefsList, or rename the new field from bmcSettingsRefsList to bmcSettingsRefs to match the deprecation guidance, ensuring consistency across all references. After making the change to the source API definition, regenerate the CRD manifest.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api/v1alpha1/biosversion_types.go`:
- Around line 70-73: The ServerRef field in the BiosVersion type has a
conflicting +optional marker that contradicts its required nature enforced by
the XValidation rule and the absence of omitempty in the JSON tag. Remove the
+optional marker comment from the ServerRef field definition to align the API
schema markers with the actual field requirements.
In `@api/v1alpha1/bmc_types.go`:
- Around line 59-67: The deprecation comment for the BMCSettingRef field
instructs users to use "bmcSettingsRefs" as the replacement, but the actual JSON
serialization tag for the BMCSettingsRefs field is "bmcSettingsRefsList". Update
the deprecation comment for BMCSettingRef to reference the correct serialized
field name "bmcSettingsRefsList" instead of "bmcSettingsRefs" so that YAML
authors receive accurate migration guidance.
In `@config/crd/bases/metal.ironcore.dev_bmcsettings.yaml`:
- Around line 126-130: The deprecation description for the settings field states
that settingsFlow takes precedence when both fields are set, but the CEL
validation rules currently reject any configuration where both settings and
settingsFlow are provided simultaneously. To fix this, either update the
deprecation description to accurately reflect that both fields cannot be set
together (align documentation with actual validation), or modify the CEL
validation rules to allow both fields to be present and ensure that the
settingsFlow field actually takes precedence during processing when both are
specified. Apply the same fix to all similar occurrences of this pattern in the
file (referenced at lines 302-304).
In `@internal/controller/bmc_controller.go`:
- Around line 102-110: When r.Get returns NotFound for a BMCSettings object, the
code ignores the error but proceeds to call r.Delete on the unpopulated
bmcSettings struct, which will fail because the object metadata is not
populated. Wrap the r.Delete call with client.IgnoreNotFound to handle the case
where the BMCSettings object no longer exists. Apply this fix to both the main
loop block (the for loop iterating over bmcObj.Spec.BMCSettingsRefs) and the
legacy block that handles deletion of old BMCSettings objects, ensuring that
Delete failures due to missing objects are safely ignored.
In `@internal/controller/helper.go`:
- Around line 630-648: The flattenSettingsFlow function has inverted priority
merge semantics. The comment states that higher-priority items (lower numbers)
should win on conflicts, but the current code sorts in ascending order and
applies items sequentially with maps.Copy, causing later items to overwrite
earlier ones. This means lower-priority items (higher numbers) actually win,
contradicting the documented behavior. Fix this by modifying the SortFunc
comparison to sort in descending order instead, so that higher-priority items
with lower priority numbers are applied last and thus overwrite lower-priority
items, aligning the implementation with the documented intent.
In `@internal/webhook/v1alpha1/helper.go`:
- Around line 59-61: The field path casing in the Child method call is
inconsistent with the JSON tag definition. Locate the line in helper.go that
contains field.NewPath("spec").Child("BMCRef") and change the string "BMCRef" to
"bmcRef" (lowercase) to match the JSON tag casing. This will ensure consistency
with the correct field path casing used elsewhere in the file (such as on line
102) and ensure validation error messages display the correct field names as
defined in the CRD schema.
---
Outside diff comments:
In `@config/crd/bases/metal.ironcore.dev_biossettings.yaml`:
- Around line 122-150: The settingsFlow array in the BIOSSettings CRD schema
allows duplicate priority values, making the order of applying settings
ambiguous when priorities are equal. Since the CRD file is generated from API
type definitions, locate the BIOSSettings API type definition (not in
config/crd/bases) and add a CEL validation rule using kubebuilder markers to
enforce that the priority field must be unique across all items in the
settingsFlow array. After adding the validation marker, regenerate the CRD
manifest by running make manifests to update the generated YAML file.
In `@config/crd/bases/metal.ironcore.dev_bmcs.yaml`:
- Around line 103-124: The deprecation message in the bmcSettingsRef field
states to use bmcSettingsRefs instead, but the actual new field in the CRD is
named bmcSettingsRefsList, creating an inconsistency in the API contract. Locate
the source API type definition (not this generated CRD yaml file) that defines
the BMC structure and the bmcSettingsRef field with its deprecation marker.
Update either the deprecation guidance comment to reference the correct field
name bmcSettingsRefsList, or rename the new field from bmcSettingsRefsList to
bmcSettingsRefs to match the deprecation guidance, ensuring consistency across
all references. After making the change to the source API definition, regenerate
the CRD manifest.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f4276f71-5059-4119-8823-511203006f3a
📒 Files selected for processing (44)
api/v1alpha1/applyconfiguration/api/v1alpha1/biossettingsspec.goapi/v1alpha1/applyconfiguration/api/v1alpha1/biosversionspec.goapi/v1alpha1/applyconfiguration/api/v1alpha1/bmcsettingsspec.goapi/v1alpha1/applyconfiguration/api/v1alpha1/bmcsettingstemplate.goapi/v1alpha1/applyconfiguration/api/v1alpha1/bmcspec.goapi/v1alpha1/applyconfiguration/api/v1alpha1/bmcversionspec.goapi/v1alpha1/applyconfiguration/api/v1alpha1/serverspec.goapi/v1alpha1/applyconfiguration/internal/internal.goapi/v1alpha1/biossettings_types.goapi/v1alpha1/biosversion_types.goapi/v1alpha1/bmc_types.goapi/v1alpha1/bmcsettings_types.goapi/v1alpha1/bmcversion_types.goapi/v1alpha1/common_types.goapi/v1alpha1/server_types.goapi/v1alpha1/zz_generated.deepcopy.goconfig/crd/bases/metal.ironcore.dev_biossettings.yamlconfig/crd/bases/metal.ironcore.dev_biossettingssets.yamlconfig/crd/bases/metal.ironcore.dev_biosversions.yamlconfig/crd/bases/metal.ironcore.dev_bmcs.yamlconfig/crd/bases/metal.ironcore.dev_bmcsettings.yamlconfig/crd/bases/metal.ironcore.dev_bmcsettingssets.yamlconfig/crd/bases/metal.ironcore.dev_bmcversions.yamlconfig/crd/bases/metal.ironcore.dev_servers.yamlinternal/controller/biossettings_controller.gointernal/controller/biossettings_controller_test.gointernal/controller/biossettingsset_controller.gointernal/controller/biossettingsset_controller_test.gointernal/controller/bmc_controller.gointernal/controller/bmcsettings_controller.gointernal/controller/bmcsettings_controller_test.gointernal/controller/bmcsettingsset_controller.gointernal/controller/bmcsettingsset_controller_test.gointernal/controller/helper.gointernal/controller/server_controller.gointernal/controller/server_controller_test.gointernal/webhook/v1alpha1/biossettings_webhook.gointernal/webhook/v1alpha1/biossettings_webhook_test.gointernal/webhook/v1alpha1/biosversion_webhook.gointernal/webhook/v1alpha1/bmcsettings_webhook.gointernal/webhook/v1alpha1/bmcsettings_webhook_test.gointernal/webhook/v1alpha1/bmcversion_webhook.gointernal/webhook/v1alpha1/bmcversion_webhook_test.gointernal/webhook/v1alpha1/helper.go
💤 Files with no reviewable changes (1)
- config/crd/bases/metal.ironcore.dev_biossettingssets.yaml
Signed-off-by: Nagadeesh Nagaraja <nagdeesh@gmail.com>
github-actions
Bot
added
the
documentation
| // Version specifies the software version (e.g. BIOS, BMC) these settings apply to. | ||
| // +required | ||
| Version string `json:"version"` | ||
| // +optional |
Contributor
Author
There was a problem hiding this comment.
note: this is done to allow flexibility to use this field in. different places without always being required.
the "required" parts are handled in the parent struct BIOSSettingsSpec using kubebuilder CEL
Signed-off-by: Nagadeesh Nagaraja <nagdeesh@gmail.com>
nagadeesh-nagaraja
force-pushed
the
sepcChanges
branch
from
e84e396 to
defaf7a
Compare
| // but different versions (e.g. hop stages) are permitted. | ||
| func checkDuplicateBMCSettings(list *metalv1alpha1.BMCSettingsList, obj *metalv1alpha1.BMCSettings) error { | ||
| for _, existing := range list.Items { | ||
| if existing.Name == obj.Name { |
Contributor
There was a problem hiding this comment.
think this extra check is missing:
if existing.Spec.BMCRef == nil || obj.Spec.BMCRef == nil {
continue
}
Contributor
Author
There was a problem hiding this comment.
BMCRef can not be nil.
Signed-off-by: Nagadeesh Nagaraja <nagdeesh@gmail.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api/v1alpha1/server_types.go`:
- Around line 151-159: Fix the typo "t1his" to "this" in the comment for the
BIOSSettingsRefs field. Additionally, correct the naming mismatch between the
field name BIOSSettingsRefs and its JSON tag biosSettingsRefsList by either
renaming the field to BIOSSettingsRefsList to match the JSON tag, or changing
the JSON tag to biosSettingsRefs to match the field name. After making these
changes, run make lint-fix and make test to ensure compliance with coding
guidelines.
In `@docs/api-reference/api.md`:
- Around line 1820-1821: Fix the typo in the biosSettingsRefsList field
description in the ServerSpec documentation. In the description text where it
currently reads "the BIOS configuration for t1his server", change "t1his" to
"this" so it reads "the BIOS configuration for this server".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a000ca41-f6ed-46e2-9162-3574ca25d547
⛔ Files ignored due to path filters (8)
dist/chart/templates/crd/metal.ironcore.dev_biossettings.yamlis excluded by!**/dist/**dist/chart/templates/crd/metal.ironcore.dev_biossettingssets.yamlis excluded by!**/dist/**dist/chart/templates/crd/metal.ironcore.dev_biosversions.yamlis excluded by!**/dist/**dist/chart/templates/crd/metal.ironcore.dev_bmcs.yamlis excluded by!**/dist/**dist/chart/templates/crd/metal.ironcore.dev_bmcsettings.yamlis excluded by!**/dist/**dist/chart/templates/crd/metal.ironcore.dev_bmcsettingssets.yamlis excluded by!**/dist/**dist/chart/templates/crd/metal.ironcore.dev_bmcversions.yamlis excluded by!**/dist/**dist/chart/templates/crd/metal.ironcore.dev_servers.yamlis excluded by!**/dist/**
📒 Files selected for processing (20)
api/v1alpha1/applyconfiguration/api/v1alpha1/bmcsettingstemplate.goapi/v1alpha1/applyconfiguration/api/v1alpha1/bmcspec.goapi/v1alpha1/applyconfiguration/api/v1alpha1/serverspec.goapi/v1alpha1/applyconfiguration/internal/internal.goapi/v1alpha1/biosversion_types.goapi/v1alpha1/bmc_types.goapi/v1alpha1/bmcsettings_types.goapi/v1alpha1/server_types.goconfig/crd/bases/metal.ironcore.dev_biosversions.yamlconfig/crd/bases/metal.ironcore.dev_bmcs.yamlconfig/crd/bases/metal.ironcore.dev_bmcsettings.yamlconfig/crd/bases/metal.ironcore.dev_bmcsettingssets.yamlconfig/crd/bases/metal.ironcore.dev_servers.yamldocs/api-reference/api.mdinternal/controller/biossettings_controller.gointernal/controller/biossettings_controller_test.gointernal/controller/bmc_controller.gointernal/controller/bmcsettings_controller.gointernal/controller/helper.gointernal/controller/server_controller.go
💤 Files with no reviewable changes (3)
- api/v1alpha1/biosversion_types.go
- internal/controller/bmcsettings_controller.go
- internal/controller/biossettings_controller.go
✅ Files skipped from review due to trivial changes (2)
- api/v1alpha1/applyconfiguration/api/v1alpha1/bmcspec.go
- api/v1alpha1/applyconfiguration/internal/internal.go
🚧 Files skipped from review as they are similar to previous changes (13)
- api/v1alpha1/applyconfiguration/api/v1alpha1/bmcsettingstemplate.go
- api/v1alpha1/applyconfiguration/api/v1alpha1/serverspec.go
- config/crd/bases/metal.ironcore.dev_servers.yaml
- config/crd/bases/metal.ironcore.dev_bmcs.yaml
- internal/controller/bmc_controller.go
- api/v1alpha1/bmc_types.go
- internal/controller/server_controller.go
- config/crd/bases/metal.ironcore.dev_bmcsettingssets.yaml
- config/crd/bases/metal.ironcore.dev_bmcsettings.yaml
- config/crd/bases/metal.ironcore.dev_biosversions.yaml
- api/v1alpha1/bmcsettings_types.go
- internal/controller/helper.go
- internal/controller/biossettings_controller_test.go
Comment on lines
+1820
to
+1821
| | `biosSettingsRef` _[LocalObjectReference](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.36/#localobjectreference-v1-core)_ | BIOSSettingsRef is a reference to a biossettings object that specifies<br />the BIOS configuration for this server.<br />Deprecated: use biosSettingsRefsList instead. Will be removed in next release. | | | | ||
| | `biosSettingsRefsList` _[LocalObjectReference](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.36/#localobjectreference-v1-core) array_ | BIOSSettingsRefs is a list of references to BIOSSettings objects that specify<br />the BIOS configuration for t1his server. Replaces the single biosSettingsRef to support<br />multiple simultaneous settings objects created. | | | |
Contributor
There was a problem hiding this comment.
Fix typo in ServerSpec documentation.
Line 1821 contains the same typo found in the source file: "t1his" should be "this" in the biosSettingsRefsList field description.
🔧 Proposed fix
-| `biosSettingsRefsList` _[LocalObjectReference](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.36/#localobjectreference-v1-core) array_ | BIOSSettingsRefs is a list of references to BIOSSettings objects that specify<br />the BIOS configuration for t1his server. Replaces the single biosSettingsRef to support<br />multiple simultaneous settings objects created. | | |
+| `biosSettingsRefsList` _[LocalObjectReference](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.36/#localobjectreference-v1-core) array_ | BIOSSettingsRefs is a list of references to BIOSSettings objects that specify<br />the BIOS configuration for this server. Replaces the single biosSettingsRef to support<br />multiple simultaneous settings objects created. | | |📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| | `biosSettingsRef` _[LocalObjectReference](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.36/#localobjectreference-v1-core)_ | BIOSSettingsRef is a reference to a biossettings object that specifies<br />the BIOS configuration for this server.<br />Deprecated: use biosSettingsRefsList instead. Will be removed in next release. | | | | |
| | `biosSettingsRefsList` _[LocalObjectReference](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.36/#localobjectreference-v1-core) array_ | BIOSSettingsRefs is a list of references to BIOSSettings objects that specify<br />the BIOS configuration for t1his server. Replaces the single biosSettingsRef to support<br />multiple simultaneous settings objects created. | | | | |
| | `biosSettingsRefsList` _[LocalObjectReference](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.36/#localobjectreference-v1-core) array_ | BIOSSettingsRefs is a list of references to BIOSSettings objects that specify<br />the BIOS configuration for this server. Replaces the single biosSettingsRef to support<br />multiple simultaneous settings objects created. | | | |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/api-reference/api.md` around lines 1820 - 1821, Fix the typo in the
biosSettingsRefsList field description in the ServerSpec documentation. In the
description text where it currently reads "the BIOS configuration for t1his
server", change "t1his" to "this" so it reads "the BIOS configuration for this
server".
xkonni
requested changes
Jun 19, 2026
xkonni
left a comment
xkonni
left a comment
Contributor
There was a problem hiding this comment.
-
Stale biosSettingsRef after deletion — cleanupReferences replaced the old BIOSSettingsRef.Name == settings.Name check with containsRef(BIOSSettingsRefs, ...). For any Server that hasn't been migrated yet, deletion of the BIOSSettings leaves the deprecated pointer set permanently. Data is silently inconsistent with no error surfaced.
-
Same bug on BMC side — identical regression in cleanupReferences for BMCSettings. The old code explicitly checked bmcObj.Spec.BMCSettingRef.Name == settings.Name; the new code only checks containsRef(BMCSettingsRefs, ...). Same permanent stale-pointer outcome.
-
Finalizer gap in SettingsMap migration — the migration patch returns before PatchEnsureFinalizer runs. If the object is deleted in that window, shouldDelete routes to delete() but no finalizer is present, so cleanup is skipped entirely and the BMC keeps a stale entry in bmcSettingsRefsList.
-
bmcSettingsRefsList JSON tag — Go field BMCSettingsRefs serializes as bmcSettingsRefsList. Any field-selector query, index registration, or tool that derives the path from the Go struct name silently finds nothing. Worth fixing before the field becomes load-bearing in production tooling.
Signed-off-by: Nagadeesh Nagaraja <nagdeesh@gmail.com>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
api/v1alpha1/server_types.go (1)
156-157:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix typo in the ServerSpec BIOS settings description.
Line 156 has
t1hisinstead ofthis. This propagates to generated API docs/CRD descriptions.🔧 Proposed fix
- // the BIOS configuration for t1his server. Replaces the single biosSettingsRef to support + // the BIOS configuration for this server. Replaces the single biosSettingsRef to support🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/v1alpha1/server_types.go` around lines 156 - 157, Fix the typo in the BIOS settings description comment for the ServerSpec struct in api/v1alpha1/server_types.go. Change the word "t1his" to "this" in the comment that describes the BIOS configuration field. This typo appears in the line that mentions replacing the single biosSettingsRef to support multiple simultaneous settings objects.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@api/v1alpha1/server_types.go`:
- Around line 156-157: Fix the typo in the BIOS settings description comment for
the ServerSpec struct in api/v1alpha1/server_types.go. Change the word "t1his"
to "this" in the comment that describes the BIOS configuration field. This typo
appears in the line that mentions replacing the single biosSettingsRef to
support multiple simultaneous settings objects.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 54afb2ad-d8b5-4fdd-937c-693ad0e9b1d2
⛔ Files ignored due to path filters (2)
dist/chart/templates/crd/metal.ironcore.dev_bmcs.yamlis excluded by!**/dist/**dist/chart/templates/crd/metal.ironcore.dev_servers.yamlis excluded by!**/dist/**
📒 Files selected for processing (10)
api/v1alpha1/applyconfiguration/api/v1alpha1/bmcspec.goapi/v1alpha1/applyconfiguration/api/v1alpha1/serverspec.goapi/v1alpha1/applyconfiguration/internal/internal.goapi/v1alpha1/bmc_types.goapi/v1alpha1/server_types.goconfig/crd/bases/metal.ironcore.dev_bmcs.yamlconfig/crd/bases/metal.ironcore.dev_servers.yamldocs/api-reference/api.mdinternal/controller/biossettings_controller.gointernal/controller/bmcsettings_controller.go
✅ Files skipped from review due to trivial changes (2)
- api/v1alpha1/applyconfiguration/api/v1alpha1/serverspec.go
- api/v1alpha1/applyconfiguration/internal/internal.go
🚧 Files skipped from review as they are similar to previous changes (6)
- config/crd/bases/metal.ironcore.dev_servers.yaml
- api/v1alpha1/bmc_types.go
- api/v1alpha1/applyconfiguration/api/v1alpha1/bmcspec.go
- config/crd/bases/metal.ironcore.dev_bmcs.yaml
- internal/controller/biossettings_controller.go
- internal/controller/bmcsettings_controller.go
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.
Proposed Changes
Fixes #944
Fixes #814
Summary by CodeRabbit
driftPolicy(enumObserve/Suspend) to BIOS/BMC settings and versions.biosSettingsRefson Servers andbmcSettingsRefson BMCs.settingsFlowfor BMC settings as the priority-ordered replacement for flat settings.biosSettingsRef,bmcSettingsRef) in favor of the new lists.settingsin favor ofsettingsFlow.settingsvssettingsFlowexclusivity, duplicate checks) and automatic migration from deprecated fields.