Refactor mock server

[afritzler]

Summary by CodeRabbit

• Refactor
  • Restructured mock server with improved state management, modular handlers, and enhanced error response handling.
  • Added resource locking and in-memory overrides for better resource management.
  • Improved BIOS settings application and power state request handling.

[coderabbitai]

Walkthrough

The mock server is refactored to replace hardcoded statics with modular state management. New infrastructure added includes resource loading/saving with in-memory overrides, per-resource locking, generalized HTTP handlers, power state categorization, BIOS settings application flow, and error mapping to HTTP status codes.

Changes

Cohort / File(s)	Summary
Server Initialization & Routing bmc/mock/server/server.go	Reworked NewMockServer to wire routes via http.ServeMux and use receiver-based handlers instead of package-level functions.
HTTP Handler Infrastructure bmc/mock/server/server.go	Replaced hardcoded per-method handlers with generalized handleGet, handlePost, handlePatch, handleDelete and specialized handleSystemReset, handleBMCReset handlers.
Power & Reset Operations bmc/mock/server/server.go	Added power state categorization (powerOffStates, powerOnStates, powerResetStates, powerResetBMCStates) and dedicated action handlers: doPowerOff, doPowerOn, doPowerReset, doBMCReset with async flow support.
Resource Management bmc/mock/server/server.go	Introduced resource loading/saving with in-memory overrides backed by embedded dataFS, includes loadResource, loadResourceLocked, saveResource, and per-resource locking via isLocked, setLocked.
BIOS Settings bmc/mock/server/server.go	Added BIOS-related constants (biosSettingsPathSuffix, attributesKey, noRebootSettings) and applyBiosSettings flow to detect path updates, apply non-rebootable settings immediately, and persist remaining settings.
Error Handling & Utilities bmc/mock/server/server.go	Introduced sentinel errors (errNotFound, errCorruptJSON, errLocked, errBadReset), handleError for HTTP status mapping, helper functions (parseResetType, resolvePath, deepCopy, mergeJSON, containsAny), and writeJSON response writer.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The pull request description is missing entirely; the required template with 'Proposed Changes' and issue reference sections is not filled out.	Add a pull request description following the template: include proposed changes summary and reference any related issue with 'Fixes #' if applicable.
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'Refactor mock server' directly corresponds to the main change—a comprehensive refactor of the mock server with modular state, new handlers, and resource management.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch enh/refactor-mock-server

Important

Action Needed: IP Allowlist Update

If your organization protects your Git platform with IP whitelisting, please add the new CodeRabbit IP address to your allowlist:

• ✨ 136.113.208.247/32 (new)
• 34.170.211.100/32
• 35.222.179.152/32

Failure to add the new IP will result in interrupted reviews.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 2

🤖 Fix all issues with AI agents

In `@bmc/mock/server/server.go`:
- Around line 96-106: In MockServer.handleGet the variable cached (from
s.overrides) is read under RLock but used after the lock is released which
allows concurrent writers (e.g., doPowerOff) to mutate the underlying map during
json.MarshalIndent in writeJSON, causing a data race; fix by making a deep copy
of the cached value while holding the read lock (or alternatively perform the
JSON marshaling while still holding the RLock) and then release the lock before
writing to the ResponseWriter—refer to MockServer.handleGet, s.overrides,
writeJSON and doPowerOff when locating where to copy/marshal.
- Around line 563-573: The deepCopy function only recurses into map[string]any
but leaves slices shared; update deepCopy to also detect slice types (e.g.,
[]any) and create a new slice, iterating elements and deep-copying each element
(recursing into maps and slices, copying primitives by value) before assigning
it to c[k]; keep the existing behavior for map values and ensure nested slices
of maps (and nested slices) are handled by the same recursion in deepCopy so
copied resources like Members no longer share references with the original.

🧹 Nitpick comments (2)

bmc/mock/server/server.go (2)

271-283: Consider parsing the JSON body for more robust reset type detection.

The substring matching approach (containsAny) works for current reset types but is fragile. For example, if a custom reset type contained "On" as a substring, it could be misclassified.

♻️ Optional: Parse JSON for explicit matching

+type resetAction struct {
+	ResetType string `json:"ResetType"`
+}
+
 func (s *MockServer) parseResetType(body []byte, offStates, onStates, resetStates []string) (string, error) {
-	bodyStr := string(body)
+	var action resetAction
+	if err := json.Unmarshal(body, &action); err != nil {
+		return "", fmt.Errorf("%w: %s", errBadReset, string(body))
+	}
 	switch {
-	case containsAny(bodyStr, offStates):
+	case slices.Contains(offStates, action.ResetType):
 		return "off", nil
-	case containsAny(bodyStr, onStates):
+	case slices.Contains(onStates, action.ResetType):
 		return "on", nil
-	case containsAny(bodyStr, resetStates):
+	case slices.Contains(resetStates, action.ResetType):
 		return "reset", nil
 	default:
-		return "", fmt.Errorf("%w: %s", errBadReset, bodyStr)
+		return "", fmt.Errorf("%w: %s", errBadReset, action.ResetType)
 	}
 }

---

510-517: Consider handling the JSON marshal error.

The error from json.MarshalIndent is silently ignored. While unlikely to fail for typical map data, logging the error would aid debugging.

🔧 Optional: Log marshal errors

 func (s *MockServer) writeJSON(w http.ResponseWriter, status int, data any) {
 	w.Header().Set("Content-Type", "application/json")
 	w.WriteHeader(status)
-	resp, _ := json.MarshalIndent(data, "", "  ")
+	resp, err := json.MarshalIndent(data, "", "  ")
+	if err != nil {
+		s.log.Error(err, "Failed to marshal JSON response")
+		return
+	}
 	if _, err := w.Write(resp); err != nil {
 		s.log.Error(err, "Failed to write response")
 	}
 }

[xkonni]

looks good. tests ran fine.