Fix dell fetch BMC attribute method

[nagadeesh-nagaraja]

Fixes #843

Summary by CodeRabbit

• Bug Fixes
  • Corrected Dell BMC setting attribute parsing to properly handle nested system configurations
  • Strengthened validation for enumeration-type attributes with explicit value checking
  • Improved error reporting to clearly indicate mismatched attribute values and available options

[coderabbitai]

📝 Walkthrough

Walkthrough

The Dell iDRAC RawData parsing for current BMC setting attributes corrects the JSON structure path from top-level Dell to nested Links.Oem.Dell, and strengthens enumeration attribute validation to explicitly require current values in merged attributes, recording errors when missing instead of attempting enum matching.

Changes

Cohort / File(s)	Summary
Dell Redfish BMC Implementation bmc/redfish_dell.go	Fixed JSON parsing path for Dell OEM links from top-level Dell to nested Links.Oem.Dell structure. Enhanced enumeration attribute validation to explicitly require current values in mergedBMCAttributes, with error recording when absent. Updated enum matching logic and error messaging to reference the stored current value and display allowed values more clearly.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

• Refactor BMC interface to support vendor-specific implementations #689: This PR introduced the regression in Dell Redfish BMC attribute parsing logic that is being fixed by correcting the JSON structure path for Dell OEM links and enumeration validation.

Suggested labels

area/metal-automation, size/M, minor

Suggested reviewers

• afritzler
• asergeant01
• davidgrun

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'Fix dell fetch BMC attribute method' is clear and directly related to the main change—correcting the JSON structure in the Dell BMC settings fetch method.
Description check	✅ Passed	The description is minimal but sufficient, containing only 'Fixes #843'. While it doesn't follow the template structure, it clearly links to the issue being addressed.
Linked Issues check	✅ Passed	The PR directly addresses issue #843 by fixing the incorrect JSON structure in getCurrentBMCSettingAttribute method to use the correct nested Redfish JSON path under Links.Oem.Dell.
Out of Scope Changes check	✅ Passed	All changes are focused on fixing the Dell BMC attribute fetching regression; no out-of-scope modifications are evident in the redfish_dell.go changes.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

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

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fixdellbmcSettings

---

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]

🧹 Nitpick comments (1)

bmc/redfish_dell.go (1)

252-255: Consider symmetric missing-value validation for non-enum attributes.

Enum attributes now fail fast when absent, but non-enum attributes still read from mergedBMCAttributes[name] directly and may return nil without an error. Aligning both branches will avoid partial/nil results slipping through.

Suggested parity fix

 		if strings.EqualFold(string(entry.Type), string(schemas.EnumerationAttributeType)) {
 			currentVal, hasCurrentVal := mergedBMCAttributes[name]
 			if !hasCurrentVal {
 				errs = append(errs, fmt.Errorf("enum attribute '%v' not found in any DellAttributes endpoint", name))
 				continue
 			}
 			// Translate the DisplayName value reported by iDRAC to the canonical
 			// ValueName used by the registry and the PATCH payload.
 			// currentVal may be nil (iDRAC CurrentValue=null for factory-default attributes),
 			// in which case the loop below will find no match and we error accordingly.
 			for _, attrValue := range entry.Value {
 				if attrValue.ValueDisplayName == currentVal {
 					result[name] = attrValue.ValueName
 					break
 				}
 			}
 			if _, ok := result[name]; !ok {
 				errs = append(errs,
 					fmt.Errorf("current setting '%v' for key '%v' not found in possible values: %v",
 						currentVal, name, entry.Value))
 			}
 		} else {
-			result[name] = mergedBMCAttributes[name]
+			currentVal, hasCurrentVal := mergedBMCAttributes[name]
+			if !hasCurrentVal {
+				errs = append(errs, fmt.Errorf("attribute '%v' not found in any DellAttributes endpoint", name))
+				continue
+			}
+			result[name] = currentVal
 		}

Also applies to: 272-274

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@bmc/redfish_dell.go` around lines 252 - 255, The enum branch currently checks
mergedBMCAttributes[name] and appends an error if missing but the non-enum
branch reads mergedBMCAttributes[name] directly and can yield nil; update the
non-enum handling to mirror the enum path by first doing currentVal,
hasCurrentVal := mergedBMCAttributes[name] and if !hasCurrentVal append an error
to errs (using the same error message pattern) and continue before using
currentVal (affecting the code that reads mergedBMCAttributes[name] later in the
function); ensure you reference mergedBMCAttributes, name and errs when making
this parity change so both branches consistently validate missing keys.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@bmc/redfish_dell.go`:
- Around line 252-255: The enum branch currently checks
mergedBMCAttributes[name] and appends an error if missing but the non-enum
branch reads mergedBMCAttributes[name] directly and can yield nil; update the
non-enum handling to mirror the enum path by first doing currentVal,
hasCurrentVal := mergedBMCAttributes[name] and if !hasCurrentVal append an error
to errs (using the same error message pattern) and continue before using
currentVal (affecting the code that reads mergedBMCAttributes[name] later in the
function); ensure you reference mergedBMCAttributes, name and errs when making
this parity change so both branches consistently validate missing keys.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 1bf8fa9e-dfaa-42e0-88c6-7596087ed6a7

📥 Commits

Reviewing files that changed from the base of the PR and between c55f475 and 61c703b.

📒 Files selected for processing (1)

• bmc/redfish_dell.go

[atd9876]

This is the same issue I encountered, and the same fix.

LGTM