Move BMCSettings mock redfish_local method into go mock server#790
Merged
Conversation
Contributor
|
Caution Review failedPull request was closed or merged during review Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdded a BMC Settings resource and attribute registry to the mock server, implemented immediate-vs-pending BMC settings semantics and lock-driven reset/apply behavior, updated Redfish client to PATCH the Settings endpoint, and migrated tests to use a shared mockServer with dynamic endpoints and reset helper APIs. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant Server as Mock Server
participant BMCMgr as BMC Manager (Current)
participant BMCSet as BMC Settings (Pending)
Client->>Server: PATCH /redfish/v1/Managers/BMC/Settings\nAttributes + `@Redfish.SettingsApplyTime`
Server->>Server: Load BMCAttributeRegistry\nDetermine immediate (ResetRequired==false) keys
alt BMC unlocked
Server->>BMCMgr: Apply immediate attributes to current Manager
Server->>BMCSet: Persist remaining attributes as pending
else BMC locked
Server->>BMCSet: Persist all attributes as pending
end
Server-->>Client: 200 / 202 / 204
Client->>Server: Trigger BMC Reset
Server->>Server: Clear BMC lock
Server->>BMCSet: Read pending Attributes
Server->>BMCMgr: Move pending Attributes into current Manager
Server->>BMCSet: Clear pending Attributes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bmc/mock/server/data/Registries/index.json (1)
5-15:⚠️ Potential issue | 🟡 MinorUpdate
Members@odata.countto reflect the actual member count.The
Members@odata.countfield is1but there are now 3 members in the array. This inconsistency could cause issues if Redfish clients rely on the count for iteration or validation.Proposed fix
- "Members@odata.count": 1, + "Members@odata.count": 3,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bmc/mock/server/data/Registries/index.json` around lines 5 - 15, The Members@odata.count value is incorrect (currently 1) and must match the actual number of entries in the Members array; update the "Members@odata.count" field in Registries/index.json to 3 so it reflects the three "@odata.id" entries in "Members" (e.g., "/redfish/v1/Registries/Base.1.5.0", "/redfish/v1/Registries/BiosAttributeRegistry.v1_0_0", "/redfish/v1/Registries/BMCAttributeRegistry").
🧹 Nitpick comments (1)
bmc/redfish_kube.go (1)
238-246: Resolve the settings URI from the selected manager instead of hardcoding it.Both methods ignore the requested BMC UUID and assume the settings resource is always
/redfish/v1/Managers/BMC/Settings. The local implementation inbmc/redfish_local.goalready follows the manager-advertised@Redfish.Settings.SettingsObject; reusing that pattern here would also let you send the same@Redfish.SettingsApplyTimepayload.Also applies to: 295-300
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bmc/redfish_kube.go` around lines 238 - 246, The patch hardcodes "/redfish/v1/Managers/BMC/Settings" in SetBMCAttributesImmediately (in RedfishKubeBMC) instead of resolving the manager's advertised settings URI; update this method (and the corresponding apply/settings method around the 295-300 region) to locate the selected manager by the requested BMC UUID, read its `@Redfish.Settings.SettingsObject` (the pattern used in the local implementation in bmc/redfish_local.go), use that SettingsObject URI as the PATCH target, and support sending the same `@Redfish.SettingsApplyTime` payload as the local implementation does.
🤖 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/mock/server/server.go`:
- Around line 56-57: The immediate-apply whitelist variable noRebootBMCSettings
is missing the "composite" attribute listed as ResetRequired:false in the BMC
attribute registry; update the slice to include "composite" (e.g., var
noRebootBMCSettings = []string{"abc", "composite"}) so PATCHes for composite are
applied immediately; optionally note for future work to derive this list
programmatically from the BMCAttributeRegistry index to avoid drift.
- Around line 459-473: The reset path (doBMCReset) flips the resource lock via
setLocked but applyPendingBMCSettings/applyBMCSettings never checks that lock,
allowing PATCHes during the reset sleep to be accepted and applied; modify
applyPendingBMCSettings and the PATCH handling code to check the same lock state
(e.g., call an isLocked(bmcPath) helper or inspect the overrides[bmcPath] locked
flag) and return/reject with an error when the resource is locked, so any
incoming PATCHes while doBMCReset has setLocked(..., true) are refused rather
than queued/applied.
In `@bmc/redfish_kube.go`:
- Around line 244-250: The PATCH response from c.Patch in bmc/redfish_kube.go
(the resp returned by c.Patch("/redfish/v1/Managers/BMC/Settings", payload)) is
not being closed; after checking err != nil add a defer resp.Body.Close() to
ensure the response body is closed and connection-pool resources are freed
(place the defer immediately after the nil-error check in the same block where
resp is available).
---
Outside diff comments:
In `@bmc/mock/server/data/Registries/index.json`:
- Around line 5-15: The Members@odata.count value is incorrect (currently 1) and
must match the actual number of entries in the Members array; update the
"Members@odata.count" field in Registries/index.json to 3 so it reflects the
three "@odata.id" entries in "Members" (e.g.,
"/redfish/v1/Registries/Base.1.5.0",
"/redfish/v1/Registries/BiosAttributeRegistry.v1_0_0",
"/redfish/v1/Registries/BMCAttributeRegistry").
---
Nitpick comments:
In `@bmc/redfish_kube.go`:
- Around line 238-246: The patch hardcodes "/redfish/v1/Managers/BMC/Settings"
in SetBMCAttributesImmediately (in RedfishKubeBMC) instead of resolving the
manager's advertised settings URI; update this method (and the corresponding
apply/settings method around the 295-300 region) to locate the selected manager
by the requested BMC UUID, read its `@Redfish.Settings.SettingsObject` (the
pattern used in the local implementation in bmc/redfish_local.go), use that
SettingsObject URI as the PATCH target, and support sending the same
`@Redfish.SettingsApplyTime` payload as the local implementation does.
🪄 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: fcf2b889-3a5e-4ded-a4a0-9f63d2f35f36
📒 Files selected for processing (13)
bmc/mock/server/data/Managers/BMC/Settings/index.jsonbmc/mock/server/data/Managers/BMC/index.jsonbmc/mock/server/data/Registries/BMCAttributeRegistry/index.jsonbmc/mock/server/data/Registries/index.jsonbmc/mock/server/server.gobmc/mockup.gobmc/redfish_kube.gobmc/redfish_local.gointernal/controller/biossettings_controller_test.gointernal/controller/bmcsettings_controller_test.gointernal/controller/bmcsettingsset_controller_test.gointernal/controller/helper.gointernal/controller/suite_test.go
💤 Files with no reviewable changes (1)
- bmc/mockup.go
afritzler
requested changes
Apr 8, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
bmc/mock/server/server.go (1)
536-540:⚠️ Potential issue | 🟠 MajorReject settings PATCHes while the BMC is reset-locked.
Returning
nilhere still letshandlePatchpersist the body to/Managers/BMC/Settingsand answer 204, so writes made during the simulated offline window are queued and then applied byapplyPendingBMCSettings(). The lock is therefore not actually blocking mutations.🔒 Minimal fix
// If the BMC is mid-reset (locked), leave the immediate settings in attrs - // so they are written to the pending Settings resource and picked up by - // applyPendingBMCSettings once the reset completes. + // reject the PATCH so writes are not accepted while the BMC is offline. if s.isLocked(bmcBase) { - return nil + return errLocked }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bmc/mock/server/server.go` around lines 536 - 540, The branch that checks s.isLocked(bmcBase) currently returns nil which allows handlePatch to continue and persist the PATCH body; change it to reject the request immediately when the BMC is reset-locked by returning an error/HTTP 423-style response from the same check so handlePatch does not write to /Managers/BMC/Settings or return 204; update the code around s.isLocked(bmcBase) to short-circuit with an appropriate error value (so pending writes are not queued) and ensure applyPendingBMCSettings remains the mechanism that applies queued changes after unlock.
🤖 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/mock/server/server.go`:
- Around line 56-85: The current loadNoRebootAttrs silently returns nil on
dataFS.ReadFile or json.Unmarshal errors causing silent behavior changes; update
loadNoRebootAttrs (used by init() to set noRebootSettings and
noRebootBMCSettings) to fail fast by panicking (or returning an error and making
init handle it) when ReadFile or Unmarshal fail, including the registryPath and
the underlying error in the panic/log message so the process exits with a clear
diagnostic instead of silently emptying the allowlist.
- Around line 623-642: The helper resetAttributesFromEmbeddedLocked currently
only replaces Attributes and can leave stale fields (like resourceLock) behind;
instead, read the embedded JSON via dataFS.ReadFile, unmarshal into a
map[string]any (as already done into defaults), and replace the entire in-memory
resource with that defaults map (i.e. assign overrides[filePath] = defaults)
rather than setting only current[attributesKey]; update usages of
loadResourceLocked/overrides accordingly so the full embedded resource (not just
Attributes) is restored in resetAttributesFromEmbeddedLocked.
---
Duplicate comments:
In `@bmc/mock/server/server.go`:
- Around line 536-540: The branch that checks s.isLocked(bmcBase) currently
returns nil which allows handlePatch to continue and persist the PATCH body;
change it to reject the request immediately when the BMC is reset-locked by
returning an error/HTTP 423-style response from the same check so handlePatch
does not write to /Managers/BMC/Settings or return 204; update the code around
s.isLocked(bmcBase) to short-circuit with an appropriate error value (so pending
writes are not queued) and ensure applyPendingBMCSettings remains the mechanism
that applies queued changes after unlock.
🪄 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: 8adadbed-ab98-4f6f-a429-3b808fa2faa0
📒 Files selected for processing (3)
bmc/mock/server/server.gointernal/controller/biossettings_controller_test.gointernal/controller/bmcsettings_controller_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/controller/biossettings_controller_test.go
afritzler
requested changes
Apr 21, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
bmc/mock/server/server.go (1)
44-47: HardcodedBMCmanager ID couples the mock to a single manager.
bmcFilePath/bmcSettingsFilePathand the URL-suffix checkstrings.Contains(urlPath, "Managers/BMC/Settings")bake the manager ID"BMC"intoapplyBMCSettings/applyPendingBMCSettings, whileResetBMCSettings(managerID)already takes it as a parameter. If a future test uses a different manager folder name, PATCH handling will silently skip BMC-settings logic andapplyPendingBMCSettingswill read/write the wrong resource. Consider deriving the manager path from the request URL (e.g. parse the manager segment fromurlPathbefore/Settings) so the mock scales to additional managers without matching the BIOS pattern that takes abasePath.Also applies to: 508-511
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bmc/mock/server/server.go` around lines 44 - 47, The code hardcodes the manager ID "BMC" via bmcFilePath/bmcSettingsFilePath and a strings.Contains(urlPath, "Managers/BMC/Settings"), which breaks applyBMCSettings and applyPendingBMCSettings for other managers; update these functions to extract the manager ID from the incoming request URL (parse the segment between "Managers/" and "/Settings") and construct the resource paths dynamically (e.g., fmt.Sprintf("data/Managers/%s/index.json", managerID) and the Settings path) instead of using bmcFilePath/bmcSettingsFilePath; ensure ResetBMCSettings(managerID) remains compatible by using the same managerID-derived path logic and replace the strings.Contains checks with the parsed-manager comparison.internal/controller/suite_test.go (1)
57-57: ClearmockServerwhen using custom mock servers.
mockServeris package-level and only overwritten in the default-server branch. If a custom-server spec runs after a default-server spec, the global still points at the previous default instance, which is easy to misuse becauseMockServercarries mutableoverridesstate.♻️ Proposed defensive cleanup
if len(redfishMockServers) > 0 { + mockServer = nil for _, serverAddr := range redfishMockServers { By(fmt.Sprintf("Starting the mock Redfish servers %v", serverAddr)) Expect(k8sManager.Add(manager.RunnableFunc(func(ctx context.Context) error {Also applies to: 345-359
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/suite_test.go` at line 57, The package-level variable mockServer (*server.MockServer) retains mutable overrides between specs; when you create or tear down a default-server instance you must explicitly nil out mockServer so custom-server specs don't inherit state. Update the default-server setup/teardown code paths to set mockServer = nil after stopping or before replacing the instance (and likewise ensure any helper that assigns mockServer clears it on failure), and apply the same defensive cleanup around the other occurrences referenced (the block around lines 345–359) so MockServer.overrides cannot leak between specs.
🤖 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/mock/server/server.go`:
- Around line 596-624: The Open call on dataFS in ResetBMCSettings and
ResetBIOSSettings leaks file handles; replace the existence probe that uses
dataFS.Open(filePath) with a non-leaking check (e.g., use fs.ReadFile or
fs.Stat) or simply remove the pre-check entirely and rely on
resetResourceFromEmbeddedLocked to read the file and log errors; update the
functions ResetBMCSettings and ResetBIOSSettings accordingly (touching the same
filePath and settingsFilePath variables) so no fs.File is left unclosed.
---
Nitpick comments:
In `@bmc/mock/server/server.go`:
- Around line 44-47: The code hardcodes the manager ID "BMC" via
bmcFilePath/bmcSettingsFilePath and a strings.Contains(urlPath,
"Managers/BMC/Settings"), which breaks applyBMCSettings and
applyPendingBMCSettings for other managers; update these functions to extract
the manager ID from the incoming request URL (parse the segment between
"Managers/" and "/Settings") and construct the resource paths dynamically (e.g.,
fmt.Sprintf("data/Managers/%s/index.json", managerID) and the Settings path)
instead of using bmcFilePath/bmcSettingsFilePath; ensure
ResetBMCSettings(managerID) remains compatible by using the same
managerID-derived path logic and replace the strings.Contains checks with the
parsed-manager comparison.
In `@internal/controller/suite_test.go`:
- Line 57: The package-level variable mockServer (*server.MockServer) retains
mutable overrides between specs; when you create or tear down a default-server
instance you must explicitly nil out mockServer so custom-server specs don't
inherit state. Update the default-server setup/teardown code paths to set
mockServer = nil after stopping or before replacing the instance (and likewise
ensure any helper that assigns mockServer clears it on failure), and apply the
same defensive cleanup around the other occurrences referenced (the block around
lines 345–359) so MockServer.overrides cannot leak between specs.
🪄 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: e39c62e3-486d-4a8c-9359-e69d4867383c
📒 Files selected for processing (10)
bmc/mock/server/data/Registries/index.jsonbmc/mock/server/server.gobmc/redfish_local.gointernal/controller/biossettings_controller_test.gointernal/controller/biossettingsset_controller_test.gointernal/controller/biosversion_controller_test.gointernal/controller/bmcsettings_controller_test.gointernal/controller/bmcsettingsset_controller_test.gointernal/controller/bmcversion_controller_test.gointernal/controller/suite_test.go
✅ Files skipped from review due to trivial changes (1)
- bmc/mock/server/data/Registries/index.json
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/controller/biossettings_controller_test.go
- internal/controller/bmcsettingsset_controller_test.go
- internal/controller/bmcsettings_controller_test.go
Member
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
afritzler
approved these changes
Apr 22, 2026
This was referenced Apr 23, 2026
davidgrun
pushed a commit
that referenced
this pull request
Apr 24, 2026
* Move BMCSettings redfish_local into go mock server * Review comments * amend server fail logic * fix reset to default logic * fix the data count lenght in mocked data * Rename variable * fix redundant check * fix newer tests workflow from main branch
This was referenced Apr 27, 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
Move mocked unit test methods into go mock server
Fixes #789
Summary by CodeRabbit
New Features
Bug Fixes
Chores