Refactor BMC interface to support vendor-specific implementations#689
Merged
Refactor BMC interface to support vendor-specific implementations#689
Conversation
Contributor
WalkthroughRefactors BMC/OEM handling: removes OEM-specific package interfaces, consolidates shared OEM helpers, renames RedfishBMC to RedfishBaseBMC, and introduces vendor-specific BMC types (Dell/HPE/Lenovo/Supermicro) that embed the base and override vendor behaviors. Factory now returns vendor-aware BMC implementations. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant RedfishBaseBMC as BaseBMC
participant VendorBMC
participant RedfishService as BMCService
Client->>BaseBMC: NewRedfishBMCClient(ctx, options)
BaseBMC->>RedfishService: connect & detect manufacturer
RedfishService-->>BaseBMC: Manufacturer info
BaseBMC-->>Client: return VendorBMC (embedded Base)
Client->>VendorBMC: UpgradeBMCVersion(params)
VendorBMC->>VendorBMC: build vendor-specific request (requestBodyFn)
VendorBMC->>RedfishService: POST /UpdateService SimpleUpdate
RedfishService-->>VendorBMC: 200/202 + Location/Body
VendorBMC->>VendorBMC: extract task monitor URI (taskMonitorURIFn)
VendorBMC->>RedfishService: GET taskURI (poll)
RedfishService-->>VendorBMC: task response
VendorBMC->>VendorBMC: parse task details (taskMonitorDetailsFn)
VendorBMC-->>Client: return task info / completion status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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)
bmc/redfish.go (1)
890-903:⚠️ Potential issue | 🟡 MinorDell special characters contain doubled
%%— likely a typo.Line 892:
"!#$%%&()*.?-@[]^_{}|~+="contains%%which produces two%characters in the charset. This doubles the probability of selecting%during password generation. If a single%` was intended, remove the duplicate.🔧 Proposed fix (if single `%` was intended)
ManufacturerDell: { - SpecialChars: "!#$%%&()*.?-@[]^_`{}|~+=", + SpecialChars: "!#$%&()*.?-@[]^_`{}|~+=", },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bmc/redfish.go` around lines 890 - 903, The Dell entry in manufacturerPasswordConfigs (key ManufacturerDell) has a duplicated percent sign in its SpecialChars string ("!#$%%&()*.?-@[]^_`{}|~+="), which will bias password generation toward '%'—remove the extra '%' so the SpecialChars contains a single '%' character; update the ManufacturerDell value in the manufacturerPasswordConfigs map (the ManufacturerDell/ManufacturerPasswordConfig entry) accordingly.
🧹 Nitpick comments (6)
bmc/redfish_lenovo.go (1)
83-142: Complex task-to-job parsing — consider extracting shared logic.The
lenovoParseTaskDetailsandhpeParseTaskDetails(inredfish_hpe.go) share significant structural overlap: read body → unmarshal task → check/transform task state. The Lenovo-specific part (OperationTransitionedToJob handling) is unique, but the boilerplate read-unmarshal-return pattern could be extracted into a shared helper to reduce duplication across vendors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bmc/redfish_lenovo.go` around lines 83 - 142, Extract the common "read body → unmarshal into *redfish.Task → basic post-unmarshal checks" logic used by lenovoParseTaskDetails and hpeParseTaskDetails into a shared helper (e.g., readAndUnmarshalTask(ctx, resp *http.Response) (*redfish.Task, []byte, error) or parseTaskBody) that returns the parsed *redfish.Task and the raw body for vendor-specific handling; then refactor LenovoRedfishBMC.lenovoParseTaskDetails to call that helper, keep only the vendor-specific OperationTransitionedToJob handling (the GET of msg.MessageArgs[0], job unmarshalling and mapping fields into a redfish.Task) and return the resulting task, and update hpeParseTaskDetails to use the same helper so the duplicated boilerplate is removed.bmc/redfish_kube.go (1)
322-329: Two differentcheckAttributesfunctions coexist — potential confusion.Line 193 calls
r.checkAttributes(attrs, filtered)(method onRedfishBaseBMC, acceptingmap[string]RegistryEntryAttributesfor BIOS), while Line 328 calls the package-levelcheckAttributes(attrs, filtered)(acceptingmap[string]redfish.Attributefor BMC). Both are correct, but the naming overlap could cause confusion during future maintenance. Consider renaming the package-level function (e.g.,checkBMCAttributes) to distinguish it from the BIOS variant.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bmc/redfish_kube.go` around lines 322 - 329, The package-level function checkAttributes that accepts map[string]redfish.Attribute should be renamed to checkBMCAttributes to avoid confusion with the RedfishBaseBMC.checkAttributes method; update its definition and all call sites (including RedfishKubeBMC.CheckBMCAttributes which currently calls checkAttributes and any other callers), and ensure imports/signatures remain the same (refer to RedfishKubeBMC.CheckBMCAttributes, getFilteredBMCRegistryAttributes, and the RedfishBaseBMC.checkAttributes method to locate the related code paths).bmc/redfish_dell.go (4)
413-427:UpgradeBiosVersionandUpgradeBMCVersionhave identical implementations (same for the task getters).This appears intentional for Dell iDRAC where BIOS and BMC upgrades follow the same Redfish path. A brief inline comment would help future maintainers understand this is by design and not a copy-paste oversight.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bmc/redfish_dell.go` around lines 413 - 427, UpgradeBiosVersion/UpgradeBMCVersion and their task getters currently call the same helpers with identical implementations, which looks like accidental duplication; add a concise inline comment in each of the methods UpgradeBiosVersion, UpgradeBMCVersion, GetBiosUpgradeTask, and GetBMCUpgradeTask indicating that the duplication is intentional because Dell iDRAC uses the same Redfish path/flow for BIOS and BMC updates, so they deliberately share the same helpers (upgradeVersion/getUpgradeTask with dellBuildRequestBody, dellExtractTaskMonitorURI, dellParseTaskDetails) to prevent future maintainers from refactoring them away.
128-136: Attribute appended even on fetch failure — harmless but misleading.When
getObjFromUrifails (line 130-131), the partially-initializedbmcDellAttributeis still appended on line 135. This is ultimately discarded (lines 137-139 returnnilon error), but acontinueafter appending the error would make the intent clearer.♻️ Suggested improvement
bmcDellAttribute := &dellAttributes{} eTag, err := r.getObjFromUri(data.String(), bmcDellAttribute) if err != nil { errs = append(errs, err) + continue } bmcDellAttribute.Etag = eTag bmcDellAttributes = append(bmcDellAttributes, *bmcDellAttribute)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bmc/redfish_dell.go` around lines 128 - 136, In the loop over tempData.DellOEMData.DellLinkAttributes, avoid appending a partially-initialized bmcDellAttribute when r.getObjFromUri returns an error: after you append err to errs in the error branch (the block handling err from getObjFromUri), add a continue so bmcDellAttributes is only appended on successful fetch; reference the variables and functions bmcDellAttribute, getObjFromUri, errs and bmcDellAttributes to locate the change.
76-76: Go naming convention:Uri→URI.Per Go conventions, acronyms in identifiers should be all-caps. Consider renaming
getObjFromUritogetObjFromURI.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bmc/redfish_dell.go` at line 76, Rename the method getObjFromUri to use the Go acronym convention getObjFromURI on the DellRedfishBMC type, update the function signature and all internal references/call sites to call getObjFromURI, and ensure any interface implementations or tests referencing getObjFromUri are updated as well; search for the symbol getObjFromUri and replace with getObjFromURI (keeping the receiver DellRedfishBMC and parameter/return types unchanged).
99-108:getManagerForOEMis fetched redundantly in some flows.For instance,
SetBMCAttributesImmediatelycallsgetCurrentBMCSettingAttribute()(which callsgetManagerForOEM()internally) and then callsgetManagerForOEM()again on line 290. Consider caching or passing the manager to avoid repeated remote calls for the same data within a single operation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bmc/redfish_dell.go` around lines 99 - 108, getManagerForOEM is being called multiple times in a single operation (e.g., SetBMCAttributesImmediately calls getCurrentBMCSettingAttribute which calls getManagerForOEM, then calls getManagerForOEM again), causing redundant remote calls; refactor to obtain the Manager once and reuse it by either (a) changing getCurrentBMCSettingAttribute to accept a *redfish.Manager parameter so callers like SetBMCAttributesImmediately can pass the already-fetched manager, or (b) add a short-lived cached field on DellRedfishBMC (e.g., lastManager with a nonce/timestamp) used within the scope of the operation to return the same manager instance for subsequent getManagerForOEM calls; update SetBMCAttributesImmediately, getCurrentBMCSettingAttribute, and getManagerForOEM accordingly to accept/return and use the shared manager to eliminate the duplicate remote call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bmc/oem_helpers.go`:
- Around line 190-195: In the loop over attrs in the function that parses
HTTP-style attributes (the for attr, value := range attrs block), you append an
error when parts := strings.Fields(attr) yields len(parts) != 2 but then still
access parts[1], causing a panic on malformed input; fix this by adding a
continue immediately after appending the error so the iteration skips accessing
parts[1] (same pattern as must be applied to the httpBasedGetBMCSettingAttribute
handling of attrs).
- Around line 137-142: The loop over attributes splits keys into parts and
appends an error when len(parts) != 2, but execution falls through and still
calls c.Get(parts[1]) causing an index-out-of-range panic; in the loop that
iterates "for key, data := range attributes" (where parts := strings.Fields(key)
is computed and c.Get(parts[1]) is later called), add an early continue
immediately after appending the validation error so the code skips using parts
when the format is invalid.
- Around line 196-201: The code unsafely asserts value.(string) before
json.Unmarshal, which will panic for non-string attribute values; update the
block around value, valueMap and json.Unmarshal to first use the comma-ok form
to detect whether value is a string (e.g., vStr, ok := value.(string)); if it is
a string, unmarshal vStr as before, otherwise convert non-string values to a
JSON string safely (e.g., marshal the value into bytes) and then unmarshal or
handle accordingly; ensure errors appended to errs (the fmt.Errorf call) include
the original value and keep the same context (parts[1]) so no panics occur for
ints/bools/maps.
- Around line 152-157: In httpBasedGetBMCSettingAttribute the resp.Body.Close()
is deferred inside the loop which delays closing until function exit; change
this to close the body immediately after reading it with io.ReadAll (i.e., call
resp.Body.Close() right after respRawBody is obtained and before any
continue/processing), remove the in-loop defer, and ensure resp is still
referenced only after the body is closed to avoid resource leaks.
In `@bmc/redfish_dell.go`:
- Around line 150-157: In the loop over registries in the function containing
the ManagerAttributeRegistry handling, avoid indexing into registry.Location
without checking length: before using registry.Location[0].URI (the access
inside the getObjFromUri call), ensure len(registry.Location) > 0 and handle the
empty-case (e.g., skip this registry or return a descriptive error) so the code
no longer panics when Location is empty; update the block that references
registry.Location[0].URI accordingly.
- Around line 243-259: The tBMCSetting variable is declared once and reused
across iterations of the loop over bmcAttrValues, causing its Attributes map to
persist and leak data between iterations; to fix, remove the outer declaration
of tBMCSetting and instead declare a fresh local tBMCSetting (same struct type
with Attributes redfish.SettingsAttributes) inside the for _, bmcAttrValue :=
range bmcAttrValues loop immediately before calling r.getObjFromUri so each
iteration unmarshals into a new struct, preventing stale map entries and
eliminating false duplicate detections when merging into
mergedPendingBMCAttributes.
- Line 212: The comparison at the enumeration check in redfish_dell.go
incorrectly lowercases entry.Type then compares to
redfish.EnumerationAttributeType, so it never matches; change the comparison to
a case-insensitive equality (use strings.EqualFold) when checking entry.Type
against redfish.EnumerationAttributeType (the code around the if that currently
reads strings.ToLower(string(entry.Type)) ==
string(redfish.EnumerationAttributeType)), so enumeration attributes hit the
enumeration branch and use the enumeration lookup logic for entry.Type and
related handling.
In `@bmc/redfish_hpe.go`:
- Around line 96-107: The current code in the redfish_hpe.go path unmarshals
into tTask and then accesses tTask.Error.ExtendedInfo[0]["MessageId"] directly
which can panic if Error is nil or ExtendedInfo is empty; update the
post-unmarshal logic in the block after json.Unmarshal so it first checks that
tTask.Error is non-nil, len(tTask.Error.ExtendedInfo) > 0 and that the map
contains the "MessageId" key (and is a string) before inspecting it, and if
those checks fail return a clear error (instead of indexing) — modify the
conditional around the ExtendedInfo access (the check that sets
task.TaskState/PercentComplete/TaskStatus and returns task,nil) to use these
guards and fall back to the existing error return when missing.
---
Outside diff comments:
In `@bmc/redfish.go`:
- Around line 890-903: The Dell entry in manufacturerPasswordConfigs (key
ManufacturerDell) has a duplicated percent sign in its SpecialChars string
("!#$%%&()*.?-@[]^_`{}|~+="), which will bias password generation toward
'%'—remove the extra '%' so the SpecialChars contains a single '%' character;
update the ManufacturerDell value in the manufacturerPasswordConfigs map (the
ManufacturerDell/ManufacturerPasswordConfig entry) accordingly.
---
Nitpick comments:
In `@bmc/redfish_dell.go`:
- Around line 413-427: UpgradeBiosVersion/UpgradeBMCVersion and their task
getters currently call the same helpers with identical implementations, which
looks like accidental duplication; add a concise inline comment in each of the
methods UpgradeBiosVersion, UpgradeBMCVersion, GetBiosUpgradeTask, and
GetBMCUpgradeTask indicating that the duplication is intentional because Dell
iDRAC uses the same Redfish path/flow for BIOS and BMC updates, so they
deliberately share the same helpers (upgradeVersion/getUpgradeTask with
dellBuildRequestBody, dellExtractTaskMonitorURI, dellParseTaskDetails) to
prevent future maintainers from refactoring them away.
- Around line 128-136: In the loop over tempData.DellOEMData.DellLinkAttributes,
avoid appending a partially-initialized bmcDellAttribute when r.getObjFromUri
returns an error: after you append err to errs in the error branch (the block
handling err from getObjFromUri), add a continue so bmcDellAttributes is only
appended on successful fetch; reference the variables and functions
bmcDellAttribute, getObjFromUri, errs and bmcDellAttributes to locate the
change.
- Line 76: Rename the method getObjFromUri to use the Go acronym convention
getObjFromURI on the DellRedfishBMC type, update the function signature and all
internal references/call sites to call getObjFromURI, and ensure any interface
implementations or tests referencing getObjFromUri are updated as well; search
for the symbol getObjFromUri and replace with getObjFromURI (keeping the
receiver DellRedfishBMC and parameter/return types unchanged).
- Around line 99-108: getManagerForOEM is being called multiple times in a
single operation (e.g., SetBMCAttributesImmediately calls
getCurrentBMCSettingAttribute which calls getManagerForOEM, then calls
getManagerForOEM again), causing redundant remote calls; refactor to obtain the
Manager once and reuse it by either (a) changing getCurrentBMCSettingAttribute
to accept a *redfish.Manager parameter so callers like
SetBMCAttributesImmediately can pass the already-fetched manager, or (b) add a
short-lived cached field on DellRedfishBMC (e.g., lastManager with a
nonce/timestamp) used within the scope of the operation to return the same
manager instance for subsequent getManagerForOEM calls; update
SetBMCAttributesImmediately, getCurrentBMCSettingAttribute, and getManagerForOEM
accordingly to accept/return and use the shared manager to eliminate the
duplicate remote call.
In `@bmc/redfish_kube.go`:
- Around line 322-329: The package-level function checkAttributes that accepts
map[string]redfish.Attribute should be renamed to checkBMCAttributes to avoid
confusion with the RedfishBaseBMC.checkAttributes method; update its definition
and all call sites (including RedfishKubeBMC.CheckBMCAttributes which currently
calls checkAttributes and any other callers), and ensure imports/signatures
remain the same (refer to RedfishKubeBMC.CheckBMCAttributes,
getFilteredBMCRegistryAttributes, and the RedfishBaseBMC.checkAttributes method
to locate the related code paths).
In `@bmc/redfish_lenovo.go`:
- Around line 83-142: Extract the common "read body → unmarshal into
*redfish.Task → basic post-unmarshal checks" logic used by
lenovoParseTaskDetails and hpeParseTaskDetails into a shared helper (e.g.,
readAndUnmarshalTask(ctx, resp *http.Response) (*redfish.Task, []byte, error) or
parseTaskBody) that returns the parsed *redfish.Task and the raw body for
vendor-specific handling; then refactor LenovoRedfishBMC.lenovoParseTaskDetails
to call that helper, keep only the vendor-specific OperationTransitionedToJob
handling (the GET of msg.MessageArgs[0], job unmarshalling and mapping fields
into a redfish.Task) and return the resulting task, and update
hpeParseTaskDetails to use the same helper so the duplicated boilerplate is
removed.
afritzler
force-pushed
the
enh/bmc-refactoring
branch
from
da25b19 to
0c4da08
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
bmc/redfish.go (2)
888-901:⚠️ Potential issue | 🟡 Minor
%%in Dell'sSpecialCharsresults in two%characters in the password pool.In a Go string literal
%%is two literal percent signs, not an escape sequence. This gives%twice the probability of being chosen during random fill compared to all other special characters. Dell likely intends a single%.🐛 Proposed fix
- ManufacturerDell: { - SpecialChars: "!#$%%&()*.?-@[]^_`{}|~+=", - }, + ManufacturerDell: { + SpecialChars: "!#$%&()*.?-@[]^_`{}|~+=", + },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bmc/redfish.go` around lines 888 - 901, The Dell entry in manufacturerPasswordConfigs uses "%%" in the SpecialChars string which yields two '%' characters in the character pool; update the ManufacturerDell SpecialChars value in the manufacturerPasswordConfigs map (the ManufacturerDell key / ManufacturerPasswordConfig.SpecialChars) to use a single '%' instead of "%%" so '%' has the same probability as other special characters.
512-517:⚠️ Potential issue | 🔴 CriticalPotential panic:
registry.Location[0]accessed without bounds check.If a
BiosAttributeRegistryentry has an emptyLocationslice, line 514 will panic with an index-out-of-range error. This is the same class of bug previously fixed inbmc/redfish_dell.go.🐛 Proposed fix
for _, registry := range registries { if strings.Contains(registry.ID, "BiosAttributeRegistry") { + if len(registry.Location) == 0 { + return nil, fmt.Errorf("BiosAttributeRegistry %q has no Location entries", registry.ID) + } if err := registry.Get(r.client, registry.Location[0].URI, biosRegistry); err != nil {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bmc/redfish.go` around lines 512 - 517, The loop over registries accesses registry.Location[0].URI without checking that registry.Location has at least one element; update the loop in the registries iteration (the block that checks if strings.Contains(registry.ID, "BiosAttributeRegistry")) to first verify len(registry.Location) > 0 (or that registry.Location != nil and len(...)>0) before using registry.Location[0].URI, and handle the empty-case by either skipping that registry or returning a clear error; ensure the Get(r.client, registry.Location[0].URI, biosRegistry) call only executes when the location is present.
🧹 Nitpick comments (1)
bmc/redfish.go (1)
124-129: Consider logging a warning when manufacturer detection fails and falls back to base.When
getSystemManufacturererrors,base.manufactureris"". Subsequent "not supported" errors will showmanufacturer "", which makes debugging harder. An optional structured log warning at fallback would improve observability.♻️ Proposed improvement
manufacturer, err := base.getSystemManufacturer() if err != nil { - // If we can't determine the manufacturer (e.g. no systems yet during - // endpoint discovery), fall back to the base implementation. + ctrl.LoggerFrom(ctx).V(1).Info("Could not detect manufacturer, falling back to base BMC", "error", err) return base, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bmc/redfish.go` around lines 124 - 129, When base.getSystemManufacturer() returns an error and you fall back to returning base (whose base.manufacturer will be empty), emit a structured warning including the error and a note that you're falling back so debugging shows why manufacturer is blank; update the error path in the block that calls base.getSystemManufacturer() to log the error (e.g., using the package's logger: log.Warnf/log.Warn or processLogger.Warn) with the error and that base.manufacturer is empty, then return base, nil.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bmc/redfish_hpe.go`:
- Around line 60-74: hpeExtractTaskMonitorURI currently returns an empty string
on success if the JSON lacks TaskMonitor and does not check the HTTP Location
header; update the function (hpeExtractTaskMonitorURI) so that after
unmarshalling you check if tResp.TaskMonitor is empty and if so attempt to
extract the URI from response.Header.Get("Location") (use that value if
non-empty), and if both are empty return a clear error instead of nil; keep
existing read/unmarshal error handling but ensure the final success path only
returns nil error when a non-empty monitor URI is found.
- Line 104: Replace the use of ptr.To when setting task.PercentComplete with
gofish.ToRef to match the project's pointer type convention: locate the
assignment to task.PercentComplete in the redfish_hpe.go code and change the
pointer creation from ptr.To(uint(100)) to gofish.ToRef(uint(100)) so the
resulting pointer type matches other mockup usage (see task.PercentComplete and
the gofish.ToRef helper).
In `@bmc/redfish_lenovo.go`:
- Around line 66-80: lenovoExtractTaskMonitorURI currently returns ("", nil)
when the response lacks the `@odata.id`, causing upgradeVersion (which only checks
err != nil) to treat an empty URI as success; modify lenovoExtractTaskMonitorURI
to validate tResp.TaskMonitor after unmarshalling and return a descriptive
non-nil error (e.g., "empty task monitor URI" or "missing `@odata.id` in
response") when it is empty so callers like upgradeVersion receive an error
instead of an empty string and can fail fast; keep the error message descriptive
and include context that this originated from lenovoExtractTaskMonitorURI.
---
Outside diff comments:
In `@bmc/redfish.go`:
- Around line 888-901: The Dell entry in manufacturerPasswordConfigs uses "%%"
in the SpecialChars string which yields two '%' characters in the character
pool; update the ManufacturerDell SpecialChars value in the
manufacturerPasswordConfigs map (the ManufacturerDell key /
ManufacturerPasswordConfig.SpecialChars) to use a single '%' instead of "%%" so
'%' has the same probability as other special characters.
- Around line 512-517: The loop over registries accesses
registry.Location[0].URI without checking that registry.Location has at least
one element; update the loop in the registries iteration (the block that checks
if strings.Contains(registry.ID, "BiosAttributeRegistry")) to first verify
len(registry.Location) > 0 (or that registry.Location != nil and len(...)>0)
before using registry.Location[0].URI, and handle the empty-case by either
skipping that registry or returning a clear error; ensure the Get(r.client,
registry.Location[0].URI, biosRegistry) call only executes when the location is
present.
---
Nitpick comments:
In `@bmc/redfish.go`:
- Around line 124-129: When base.getSystemManufacturer() returns an error and
you fall back to returning base (whose base.manufacturer will be empty), emit a
structured warning including the error and a note that you're falling back so
debugging shows why manufacturer is blank; update the error path in the block
that calls base.getSystemManufacturer() to log the error (e.g., using the
package's logger: log.Warnf/log.Warn or processLogger.Warn) with the error and
that base.manufacturer is empty, then return base, nil.
nagadeesh-nagaraja
approved these changes
Mar 2, 2026
9 tasks
This was referenced Apr 22, 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
Fixes #688