feat: Add net.HardwareAddr wrapper for BMC MAC JSON marshaling#369
Open
jw-nvidia wants to merge 1 commit intoNVIDIA:mainfrom
Open
feat: Add net.HardwareAddr wrapper for BMC MAC JSON marshaling#369jw-nvidia wants to merge 1 commit intoNVIDIA:mainfrom
jw-nvidia wants to merge 1 commit intoNVIDIA:mainfrom
Conversation
jw-nvidia
changed the title
jw-nvidia
force-pushed
the
feat/mac_addr_json
branch
from
abca6ec to
ef8419d
Compare
Summary by CodeRabbit
WalkthroughAdds an exported Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 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)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rla/pkg/inventoryobjects/bmc/bmc.go`:
- Line 31: Fix two issues caused by the MACAddress wrapper: replace assignments
like `bmc.MAC, _ = net.ParseMAC(mac)` with the same pattern used in `bmc.New` —
call `net.ParseMAC(mac)`, handle the error, and assign the parsed value as
`MACAddress{HardwareAddr: parsed}` to the `bmc.MAC` field; and replace struct
nil checks like `if b.MAC != nil` with a check of the embedded hardware address
(e.g., `if b.MAC.HardwareAddr != nil` or `len(b.MAC.HardwareAddr) > 0`) so you
test the actual pointer/length rather than the struct itself.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8d743cab-cf76-4527-9aa2-f599f1fb32d7
📒 Files selected for processing (9)
rla/internal/converter/dao/converter.gorla/internal/converter/dao/converter_test.gorla/internal/converter/protobuf/converter.gorla/internal/converter/protobuf/converter_test.gorla/internal/service/component_api_test.gorla/pkg/inventoryobjects/bmc/bmc.gorla/pkg/inventoryobjects/bmc/bmc_test.gorla/pkg/inventoryobjects/bmc/macaddr.gorla/pkg/inventoryobjects/component/component_test.go
thossain-nv
changed the title
…haling Introduce MACAddress in the bmc package, wrapping net.HardwareAddr and implementing encoding.TextMarshaler/TextUnmarshaler so that BMC.MAC serializes to a human-readable colon-separated string in JSON instead of a base64-encoded byte array. Signed-off-by: Jin Wang <jinwan@nvidia.com>
jw-nvidia
force-pushed
the
feat/mac_addr_json
branch
from
ef8419d to
664e12b
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
rla/pkg/types/types.go (1)
57-57: Avoid leakinginventoryobjects/bmcinto the sharedtypesAPI surface.Line 57 couples
rla/pkg/typestorla/pkg/inventoryobjects/bmc, which increases cross-package dependency for consumers oftypes.BMC. Consider introducing atypes.MACAddresstype (or alias) inrla/pkg/typesand keep package-specific mapping in converters.As per coding guidelines,
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rla/pkg/types/types.go` at line 57, types.BMC currently exposes the concrete type bmc.MACAddress (field MAC) which leaks the inventoryobjects/bmc package into the shared API; change this by declaring a local types.MACAddress (type alias or new named type) in rla/pkg/types and update the BMC struct to use types.MACAddress for the MAC field, then adjust any converter functions/methods (the mapping code that converts between inventoryobjects/bmc and types.BMC) to translate between bmc.MACAddress and the new types.MACAddress so the inventory package is no longer imported by rla/pkg/types.rla/pkg/client/convert.go (1)
127-130: This behavior is intentional and consistent with established converter patterns; documentation is optional.Invalid MAC addresses are silently ignored across the converter layer (
rla/internal/converter/dao/converter.go,rla/internal/converter/protobuf/converter.go) by design, as validated by explicit test cases ("invalid MAC address" in both test files). The internal converters do not document this behavior—it defaults to an unset MAC (zero value), which is a sensible fallback when parsing fails.If consistency or clarity is desired, a brief comment explaining the intentional behavior would be beneficial, particularly since this function lacks a dedicated test file unlike its counterparts in the internal converter layer.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rla/pkg/client/convert.go` around lines 127 - 130, Add a brief inline comment in rla/pkg/client/convert.go immediately above the net.ParseMAC call (the block that sets bmc.MAC = bmcobj.MACAddress{HardwareAddr: addr}) stating that invalid MAC addresses are intentionally ignored and left as the zero-value, mirroring behavior in rla/internal/converter/dao/converter.go and rla/internal/converter/protobuf/converter.go; optionally add a small unit test for this converter to assert that an invalid MAC leaves bmc.MAC unset if you want parity with the internal converters.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@rla/pkg/client/convert.go`:
- Around line 127-130: Add a brief inline comment in rla/pkg/client/convert.go
immediately above the net.ParseMAC call (the block that sets bmc.MAC =
bmcobj.MACAddress{HardwareAddr: addr}) stating that invalid MAC addresses are
intentionally ignored and left as the zero-value, mirroring behavior in
rla/internal/converter/dao/converter.go and
rla/internal/converter/protobuf/converter.go; optionally add a small unit test
for this converter to assert that an invalid MAC leaves bmc.MAC unset if you
want parity with the internal converters.
In `@rla/pkg/types/types.go`:
- Line 57: types.BMC currently exposes the concrete type bmc.MACAddress (field
MAC) which leaks the inventoryobjects/bmc package into the shared API; change
this by declaring a local types.MACAddress (type alias or new named type) in
rla/pkg/types and update the BMC struct to use types.MACAddress for the MAC
field, then adjust any converter functions/methods (the mapping code that
converts between inventoryobjects/bmc and types.BMC) to translate between
bmc.MACAddress and the new types.MACAddress so the inventory package is no
longer imported by rla/pkg/types.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6dae7d3a-ddc5-422d-811b-a53b489f5d1f
📒 Files selected for processing (11)
rla/internal/converter/dao/converter.gorla/internal/converter/dao/converter_test.gorla/internal/converter/protobuf/converter.gorla/internal/converter/protobuf/converter_test.gorla/internal/service/component_api_test.gorla/pkg/client/convert.gorla/pkg/inventoryobjects/bmc/bmc.gorla/pkg/inventoryobjects/bmc/bmc_test.gorla/pkg/inventoryobjects/bmc/macaddr.gorla/pkg/inventoryobjects/component/component_test.gorla/pkg/types/types.go
✅ Files skipped from review due to trivial changes (5)
- rla/internal/converter/dao/converter.go
- rla/internal/service/component_api_test.go
- rla/pkg/inventoryobjects/component/component_test.go
- rla/pkg/inventoryobjects/bmc/bmc_test.go
- rla/pkg/inventoryobjects/bmc/macaddr.go
🚧 Files skipped from review as they are similar to previous changes (4)
- rla/internal/converter/dao/converter_test.go
- rla/internal/converter/protobuf/converter.go
- rla/pkg/inventoryobjects/bmc/bmc.go
- rla/internal/converter/protobuf/converter_test.go
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
1 participant
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.
Description
Introduce MACAddress in the bmc package, wrapping net.HardwareAddr and implementing encoding.TextMarshaler/TextUnmarshaler so that BMC.MAC serializes to a human-readable colon-separated string in JSON instead of a base64-encoded byte array.
Type of Change
Services Affected
Related Issues (Optional)
Breaking Changes
Testing
Additional Notes