Update gofish dependency and adjust for changes#644
Merged
afritzler merged 1 commit intoironcore-dev:mainfrom Feb 19, 2026
Merged
Update gofish dependency and adjust for changes#644afritzler merged 1 commit intoironcore-dev:mainfrom
afritzler merged 1 commit intoironcore-dev:mainfrom
Conversation
Contributor
WalkthroughThis PR migrates usages of github.com/stmcginnis/gofish/{redfish,common} to github.com/stmcginnis/gofish/schemas, updating imports, public API signatures, struct fields, constants, and internal types across BMC, OEM, Redfish implementations, mocks, controllers, and API types. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 1
🤖 Fix all issues with AI agents
In `@bmc/oem/dell.go`:
- Around line 254-256: The issue is inconsistent case handling for attribute
type comparisons: code at the filteredAttr processing site uses
strings.ToLower(string(entry.Type)) to compare against
schemas.EnumerationAttributeType while the switch in helpers.go compares raw
values, causing mismatches for mixed-case input. Fix by normalizing attribute
types once when parsing/constructing the filteredAttr entries (e.g., set
entry.Type = strings.ToLower(string(entry.Type)) or use strings.ToLower on the
source field) so all downstream code (including the switch in helpers.go) sees a
consistent canonical casing; alternatively make the helpers.go switch use
case-insensitive comparisons (e.g., strings.EqualFold) for entry.Type vs
schemas.EnumerationAttributeType to match the behavior here. Ensure you update
the code paths that produce or consume entry.Type (the filteredAttr build and
the helpers.go attribute-type switch) so the comparison logic is uniform.
Member
|
@stmcginnis thanks a lot for your PR! This is very much appreciated. Since 0.21.0 of |
stmcginnis
force-pushed
the
gofish-update
branch
from
a59b2cf to
6d53c56
Compare
The gofish library has been extensively refactored. This updates usage based on those changes. Signed-off-by: Sean McGinnis <sean.mcginnis@gmail.com>
stmcginnis
force-pushed
the
gofish-update
branch
from
6d53c56 to
6737a58
Compare
Contributor
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)
bmc/redfish.go (1)
834-855:⚠️ Potential issue | 🔴 CriticalFix package reference:
GetObjectis incommon, notschemas.The call
schemas.GetObject[schemas.ComputerSystem](r.client, systemURI)at line 846 references a function that does not exist in theschemaspackage. In gofish v0.21.1,GetObjectis located in thecommonpackage (github.com/stmcginnis/gofish/common). Add the import and change the call tocommon.GetObject[schemas.ComputerSystem](r.client, systemURI).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bmc/redfish.go` around lines 834 - 855, The call to schemas.GetObject in getSystemFromUri is incorrect because GetObject lives in the common package; update the import to include github.com/stmcginnis/gofish/common and change the call to use common.GetObject[schemas.ComputerSystem](r.client, systemURI) (keeping the rest of getSystemFromUri logic the same) so the function compiles and resolves the correct symbol.
🧹 Nitpick comments (1)
bmc/mockup.go (1)
33-39: Inconsistent attribute type representation between BIOS and BMC mock settings.
BIOSSettingAttr(lines 33–36) uses raw strings"string","integer"whileBMCSettingAttr(lines 38–39) usesschemas.StringAttributeType,schemas.IntegerAttributeType. The same discrepancy recurs inResetBIOSSettings(line 151) vsResetBMCSettings(line 175). Since this is a mock, the functional impact is nil (the BIOS validation path lowercases everything), but aligning both to use theschemasconstants would keep the mock self-consistent.♻️ Suggested alignment
r.BIOSSettingAttr = map[string]map[string]any{ - "abc": {"type": "string", "reboot": false, "value": "bar"}, - "fooreboot": {"type": "integer", "reboot": true, "value": 123}, + "abc": {"type": schemas.StringAttributeType, "reboot": false, "value": "bar"}, + "fooreboot": {"type": schemas.IntegerAttributeType, "reboot": true, "value": 123}, }Apply the same in
ResetBIOSSettings.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bmc/mockup.go` around lines 33 - 39, The BIOS mock uses raw strings for types while BMC uses schema constants; update BIOSSettingAttr and ResetBIOSSettings to use schemas.StringAttributeType and schemas.IntegerAttributeType (matching ResetBMCSettings/BMCSettingAttr) so both mocks are consistent; locate the maps named BIOSSettingAttr and the function ResetBIOSSettings and replace the literal "string"/"integer" values with schemas.StringAttributeType and schemas.IntegerAttributeType respectively.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@bmc/redfish.go`:
- Around line 834-855: The call to schemas.GetObject in getSystemFromUri is
incorrect because GetObject lives in the common package; update the import to
include github.com/stmcginnis/gofish/common and change the call to use
common.GetObject[schemas.ComputerSystem](r.client, systemURI) (keeping the rest
of getSystemFromUri logic the same) so the function compiles and resolves the
correct symbol.
---
Duplicate comments:
In `@bmc/oem/dell.go`:
- Line 254: This comparison uses strings.ToLower(string(entry.Type)) ==
string(schemas.EnumerationAttributeType); to make intent explicit and robust,
replace it with a case-insensitive comparison (e.g., use strings.EqualFold on
string(entry.Type) and string(schemas.EnumerationAttributeType)) or ensure the
constant schemas.EnumerationAttributeType is guaranteed to be lowercase; update
the match in bmc/oem/dell.go where entry.Type is checked so the comparison is
clearly case-insensitive and unambiguous.
In `@bmc/redfish_kube.go`:
- Around line 305-319: The mock uses direct type assertions on attrData["type"]
and attrData["reboot"] which can panic if the map values are missing or of the
wrong type; in getFilteredBMCRegistryAttributes validate presence and types
before casting (e.g., check attrData["type"] exists and is a
schemas.AttributeType via a type assertion or switch, and attrData["reboot"]
exists and is a bool) and return a descriptive error if validation fails; update
the loop that builds filtered to use these safe checks for
UnitTestMockUps.BMCSettingAttr to avoid runtime panics.
---
Nitpick comments:
In `@bmc/mockup.go`:
- Around line 33-39: The BIOS mock uses raw strings for types while BMC uses
schema constants; update BIOSSettingAttr and ResetBIOSSettings to use
schemas.StringAttributeType and schemas.IntegerAttributeType (matching
ResetBMCSettings/BMCSettingAttr) so both mocks are consistent; locate the maps
named BIOSSettingAttr and the function ResetBIOSSettings and replace the literal
"string"/"integer" values with schemas.StringAttributeType and
schemas.IntegerAttributeType respectively.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
bmc/oem/lenovo.go (1)
217-220: Incorrect vendor name in comments (pre-existing).Line 219 says "HPE iLO" but this is the Lenovo implementation. Similarly, line 279 references "Dell iDRAC". These are copy-paste artifacts from other OEM implementations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bmc/oem/lenovo.go` around lines 217 - 220, The comment above the function that currently returns "return false, nil" incorrectly mentions "HPE iLO" (copy-paste); similarly another comment later mentions "Dell iDRAC". Update those comments to refer to the correct Lenovo BMC implementation (e.g., "Lenovo XClarity (XCC) / Lenovo BMC" or "Lenovo IMM/XClarity" as appropriate) so they reflect this Lenovo implementation; locate the comment immediately preceding the function that returns (bool, error) with "return false, nil" and the comment near the reference at the other location (around the block referencing iDRAC) and replace vendor names accordingly while preserving the original meaning.bmc/oem/hpe.go (2)
246-248: Stale comment: "Dell iDRAC" in HPE code.Line 247 says
"We do not have any option to get pending attributes for Dell iDRAC"but this is in the HPE implementation. This is pre-existing but trivially confusing.📝 Suggested fix
- // We do not have any option to get pending attributes for Dell iDRAC + // We do not have any option to get pending attributes for HPE iLO🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bmc/oem/hpe.go` around lines 246 - 248, The comment in HPEILOManager.GetBMCPendingAttributeValues incorrectly references "Dell iDRAC"; update or remove the stale comment so it correctly describes the HPE implementation (e.g., "We do not have any option to get pending attributes for HPE iLO") while keeping the existing return of empty schemas.SettingsAttributes and nil error; locate the comment inside the GetBMCPendingAttributeValues function and replace the incorrect vendor string.
190-198: Copy-paste issue in error message (pre-existing): "does not support scheduled apply time" and PATCH log says "POST".Line 236 (not changed) logs
"failed to POST attribute"for aPATCHcase. This is a pre-existing copy-paste bug — not introduced by this PR, but worth noting for a follow-up fix.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bmc/oem/hpe.go` around lines 190 - 198, The function UpdateBMCAttributesApplyAt has inconsistent messaging: the error for non-immediate apply time and the HTTP-operation log are copy-paste leftovers; update the human-facing error in UpdateBMCAttributesApplyAt to a clear, properly capitalized message (e.g., "does not support scheduled apply time for BMC attributes") if you want to normalize casing, and fix the operation log that currently says "failed to POST attribute" to accurately reflect the actual HTTP method used in that code path (change to "failed to PATCH attribute" where a PATCH is performed, and ensure any related log/error strings around the PATCH code path use consistent wording).
🤖 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_kube.go`:
- Around line 377-386: The call to system.SetBoot is passing a pointer but the
method expects a schemas.Boot value; change system.SetBoot(&setBoot) to
system.SetBoot(setBoot) (this applies to the shown call using the local variable
setBoot which is assigned from pxeBootWithSettingUEFIBootMode /
pxeBootWithoutSettingUEFIBootMode), and make the same adjustment for the other
SetBoot invocation that currently passes a pointer (replace &<BootVar> with
<BootVar>).
---
Nitpick comments:
In `@bmc/oem/hpe.go`:
- Around line 246-248: The comment in HPEILOManager.GetBMCPendingAttributeValues
incorrectly references "Dell iDRAC"; update or remove the stale comment so it
correctly describes the HPE implementation (e.g., "We do not have any option to
get pending attributes for HPE iLO") while keeping the existing return of empty
schemas.SettingsAttributes and nil error; locate the comment inside the
GetBMCPendingAttributeValues function and replace the incorrect vendor string.
- Around line 190-198: The function UpdateBMCAttributesApplyAt has inconsistent
messaging: the error for non-immediate apply time and the HTTP-operation log are
copy-paste leftovers; update the human-facing error in
UpdateBMCAttributesApplyAt to a clear, properly capitalized message (e.g., "does
not support scheduled apply time for BMC attributes") if you want to normalize
casing, and fix the operation log that currently says "failed to POST attribute"
to accurately reflect the actual HTTP method used in that code path (change to
"failed to PATCH attribute" where a PATCH is performed, and ensure any related
log/error strings around the PATCH code path use consistent wording).
In `@bmc/oem/lenovo.go`:
- Around line 217-220: The comment above the function that currently returns
"return false, nil" incorrectly mentions "HPE iLO" (copy-paste); similarly
another comment later mentions "Dell iDRAC". Update those comments to refer to
the correct Lenovo BMC implementation (e.g., "Lenovo XClarity (XCC) / Lenovo
BMC" or "Lenovo IMM/XClarity" as appropriate) so they reflect this Lenovo
implementation; locate the comment immediately preceding the function that
returns (bool, error) with "return false, nil" and the comment near the
reference at the other location (around the block referencing iDRAC) and replace
vendor names accordingly while preserving the original meaning.
Comment on lines
+377
to
386
| var setBoot schemas.Boot | ||
| // TODO: cover setting BootSourceOverrideMode with BIOS settings profile | ||
| // Only skip setting BootSourceOverrideMode for older BMCs that don't report it | ||
| if system.Boot.BootSourceOverrideMode != "" { | ||
| setBoot = pxeBootWithSettingUEFIBootMode | ||
| } else { | ||
| setBoot = pxeBootWithoutSettingUEFIBootMode | ||
| } | ||
| if err := system.SetBoot(setBoot); err != nil { | ||
| if err := system.SetBoot(&setBoot); err != nil { | ||
| return fmt.Errorf("failed to set the boot order: %w", err) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for SetBoot method definition and pxeBoot variables
rg -n "func.*SetBoot|pxeBootWith" --type go -B 2 -A 5Repository: ironcore-dev/metal-operator
Length of output: 2559
🏁 Script executed:
#!/bin/bash
# Search for SetBoot method definition more broadly
rg -n "SetBoot.*\(" --type go -B 2 -A 8 | grep -E "func.*SetBoot|SetBoot\(" | head -20Repository: ironcore-dev/metal-operator
Length of output: 390
🏁 Script executed:
#!/bin/bash
# Search for system variable declaration and imports
rg -n "system\s*:=" --type go -B 10 | head -30Repository: ironcore-dev/metal-operator
Length of output: 805
🏁 Script executed:
#!/bin/bash
# Find the function containing SetBoot call and its parameters
rg -n "func.*SetPXEBootOnce|system.*SetBoot" --type go -B 15 -A 2 | head -60Repository: ironcore-dev/metal-operator
Length of output: 3579
🏁 Script executed:
#!/bin/bash
# Find getSystemFromUri function definition
rg -n "func.*getSystemFromUri" --type go -A 10Repository: ironcore-dev/metal-operator
Length of output: 683
🏁 Script executed:
#!/bin/bash
# Find imports in the redfish files
head -50 bmc/redfish.go | grep -E "^import|schemas"Repository: ironcore-dev/metal-operator
Length of output: 119
🌐 Web query:
gofish SetBoot method signature ComputerSystem
💡 Result:
In github.com/stmcginnis/gofish/redfish, the ComputerSystem.SetBoot method signature is:
func (computersystem *ComputerSystem) SetBoot(b Boot) error[1]
Fix type mismatch: pass Boot value, not pointer to SetBoot.
The SetBoot method from gofish accepts a Boot value type, not a pointer. Change system.SetBoot(&setBoot) to system.SetBoot(setBoot) at line 385 (and similarly at line 215 in bmc/redfish.go).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bmc/redfish_kube.go` around lines 377 - 386, The call to system.SetBoot is
passing a pointer but the method expects a schemas.Boot value; change
system.SetBoot(&setBoot) to system.SetBoot(setBoot) (this applies to the shown
call using the local variable setBoot which is assigned from
pxeBootWithSettingUEFIBootMode / pxeBootWithoutSettingUEFIBootMode), and make
the same adjustment for the other SetBoot invocation that currently passes a
pointer (replace &<BootVar> with <BootVar>).
afritzler
approved these changes
Feb 19, 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.
Proposed Changes
The gofish library has been extensively refactored. This updates usage based on those changes.
This has just been a mechanical update. No testing beyond local linting and unit tests. Feel free to take over for any further adjustments, and please file any issues found with gofish or tag me on any follow ups.
Summary by CodeRabbit
Chores
Refactor
Bug Fixes