feat: Add support for Delta PMC vendor in Powershelf Manager#331
Open
spydaNVIDIA wants to merge 1 commit intomainfrom
Open
feat: Add support for Delta PMC vendor in Powershelf Manager#331spydaNVIDIA wants to merge 1 commit intomainfrom
spydaNVIDIA wants to merge 1 commit intomainfrom
Conversation
📝 WalkthroughWalkthroughThe pull request extends the powershelf-manager to support a new vendor called "Delta" by adding a corresponding enum value in the protobuf definition, vendor code mappings, a new firmware upgrade rule implementation, and graceful handling when firmware artifacts are unavailable for a vendor. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 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 docstrings
🧪 Generate unit tests (beta)
Comment |
🔐 TruffleHog Secret Scan✅ No secrets or credentials found! Your code has been scanned for 700+ types of secrets and credentials. All clear! 🎉 🕐 Last updated: 2026-04-02 18:55:43 UTC | Commit: 88cd6d2 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (5)
powershelf-manager/pkg/firmwaremanager/embedded_firmware.go (1)
110-111: Use structured logrus methods for consistent logging.Same pattern: use
log.Infoforlog.Warnfinstead ofPrintfto leverage logrus's log levels and structured logging.♻️ Suggested fix
- log.Printf("no embedded firmware found for vendor %s; firmware operations will be unavailable for this vendor", v.Name) + log.Infof("no embedded firmware found for vendor %s; firmware operations will be unavailable for this vendor", v.Name)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@powershelf-manager/pkg/firmwaremanager/embedded_firmware.go` around lines 110 - 111, Replace the use of log.Printf with a logrus level-specific method to keep structured logging consistent: in embedded_firmware.go where you log "no embedded firmware found for vendor %s; firmware operations will be unavailable for this vendor" (using v.Name), change the call to log.Warnf (or log.Infof if you prefer informational level) so the message uses logrus's leveled/structured logging rather than Printf.powershelf-manager/pkg/firmwaremanager/firmware_updater.go (1)
63-65: Use structured logrus methods instead ofPrintf.Same as in
firmware_manager.go: the import uses logrus, butPrintfbypasses its log level and structured logging features. Consider usinglog.Warnforlog.Infoffor consistency.♻️ Suggested fix
if len(repo.upgrades) == 0 { - log.Printf("no firmware artifacts available for vendor %s; firmware operations will be unavailable", v.Name) + log.Warnf("no firmware artifacts available for vendor %s; firmware operations will be unavailable", v.Name) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@powershelf-manager/pkg/firmwaremanager/firmware_updater.go` around lines 63 - 65, Replace the direct call to log.Printf in firmware_updater.go that reports an empty repo (the block checking len(repo.upgrades) == 0 referencing repo.upgrades and v.Name) with a structured logrus call such as log.Warnf or log.Infof so the message uses logrus levels and structured logging (e.g., call log.Warnf("no firmware artifacts available for vendor %s; firmware operations will be unavailable", v.Name)). Ensure you import/use the same log variable used elsewhere in this package.powershelf-manager/pkg/firmwaremanager/firmware_manager.go (1)
67-68: Use structured logrus methods instead ofPrintf.The import uses
log "github.com/sirupsen/logrus", butPrintfis a standard library-style call. For consistency with logrus and to leverage structured logging/log levels, consider usinglog.Warnforlog.WithField("vendor", vendor.Name).Warn(...).♻️ Suggested fix
- log.Printf("skipping firmware support for vendor %s: %v", vendor.Name, err) + log.Warnf("skipping firmware support for vendor %s: %v", vendor.Name, err)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@powershelf-manager/pkg/firmwaremanager/firmware_manager.go` around lines 67 - 68, Replace the stdlib log.Printf call with a structured logrus warning; specifically, in firmware_manager.go where the loop logs "skipping firmware support for vendor %s: %v" using vendor.Name and err, change this to use logrus (e.g., log.WithField("vendor", vendor.Name).Warnf(...) or log.WithFields(...).Warn(...)) so the vendor and error are recorded as structured fields and the correct log level is used; update the call that currently references vendor.Name and err to use those as structured fields/message via logrus methods.powershelf-manager/pkg/common/vendor/vendor_test.go (1)
214-224: Consider table-driven expected values to avoid growing if-else chain.The explicit
if-elsechain works but will grow with each new vendor. Consider embedding the expected round-trip results directly in the test case struct to keep assertions simpler.♻️ Example refactor
func TestRoundTrip_NameAndCode(t *testing.T) { testCases := map[string]struct { code VendorCode + wantCode VendorCode + wantName string }{ - "round-trip Liteon": {code: VendorCodeLiteon}, - "round-trip Delta": {code: VendorCodeDelta}, - "round-trip Unsupported": {code: VendorCodeUnsupported}, - "round-trip Max sentinel": {code: VendorCodeMax}, + "round-trip Liteon": {code: VendorCodeLiteon, wantCode: VendorCodeLiteon, wantName: VendorLiteon}, + "round-trip Delta": {code: VendorCodeDelta, wantCode: VendorCodeDelta, wantName: VendorDelta}, + "round-trip Unsupported": {code: VendorCodeUnsupported, wantCode: VendorCodeUnsupported, wantName: "Unsupported"}, + "round-trip Max sentinel": {code: VendorCodeMax, wantCode: VendorCodeUnsupported, wantName: "Unsupported"}, } for name, tc := range testCases { t.Run(name, func(t *testing.T) { v := CodeToVendor(tc.code) v2 := StringToVendor(v.Name) - - // For supported vendors, code should round-trip. For unsupported names, StringToVendor maps to Unsupported. - if tc.code == VendorCodeLiteon { - assert.Equal(t, VendorCodeLiteon, v2.Code) - assert.Equal(t, VendorLiteon, v2.Name) - } else if tc.code == VendorCodeDelta { - assert.Equal(t, VendorCodeDelta, v2.Code) - assert.Equal(t, VendorDelta, v2.Name) - } else { - assert.Equal(t, VendorCodeUnsupported, v2.Code) - assert.Equal(t, v.Name, v2.Name) - } + assert.Equal(t, tc.wantCode, v2.Code) + assert.Equal(t, tc.wantName, v2.Name) }) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@powershelf-manager/pkg/common/vendor/vendor_test.go` around lines 214 - 224, The test's long if-else chain (checking VendorCodeLiteon, VendorCodeDelta, else VendorCodeUnsupported) should be replaced by storing expected round-trip results on each test case and asserting against those values; update the test case struct (the slice of tc entries used in this test) to include expected Code and Name fields (e.g., ExpectedCode, ExpectedName) and then replace the conditional block with simple assertions comparing v2.Code to tc.ExpectedCode and v2.Name to tc.ExpectedName (references: VendorCodeLiteon, VendorCodeDelta, VendorCodeUnsupported, v2, tc, v.Name).powershelf-manager/pkg/firmwaremanager/firmware_upgrade.go (1)
65-74: Consider a shared base rule to reduce duplication.
DeltaUpgradeRuleis functionally identical toLiteonUpgradeRule. If the rules are expected to diverge in the future, this separation is fine—consider adding a brief comment noting why they're separate. Otherwise, a single parameterized type would reduce duplication.♻️ Example: shared DirectUpgradeRule
-// LiteonUpgradeRule allows only direct upgrades where the device's current version equals the edge's source. -type LiteonUpgradeRule struct{} - -func (r LiteonUpgradeRule) isAllowed(currentFw firmwareVersion, upgrade FirmwareUpgrade) bool { - return currentFw.cmp(upgrade.from) == 0 -} - -func (r LiteonUpgradeRule) summary() string { - return "Liteon upgrade rule: only direct upgrades supported" -} - -// DeltaUpgradeRule allows only direct upgrades where the device's current version equals the edge's source. -type DeltaUpgradeRule struct{} - -func (r DeltaUpgradeRule) isAllowed(currentFw firmwareVersion, upgrade FirmwareUpgrade) bool { - return currentFw.cmp(upgrade.from) == 0 -} - -func (r DeltaUpgradeRule) summary() string { - return "Delta upgrade rule: only direct upgrades supported" -} +// DirectUpgradeRule allows only direct upgrades where the device's current version equals the edge's source. +type DirectUpgradeRule struct { + vendorName string +} + +func (r DirectUpgradeRule) isAllowed(currentFw firmwareVersion, upgrade FirmwareUpgrade) bool { + return currentFw.cmp(upgrade.from) == 0 +} + +func (r DirectUpgradeRule) summary() string { + return r.vendorName + " upgrade rule: only direct upgrades supported" +}Then in
newUpgradeRule:switch v.Code { case vendor.VendorCodeLiteon: - return LiteonUpgradeRule{}, nil + return DirectUpgradeRule{vendorName: "Liteon"}, nil case vendor.VendorCodeDelta: - return DeltaUpgradeRule{}, nil + return DirectUpgradeRule{vendorName: "Delta"}, nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@powershelf-manager/pkg/firmwaremanager/firmware_upgrade.go` around lines 65 - 74, DeltaUpgradeRule duplicates LiteonUpgradeRule behavior; introduce a shared parameterized rule (e.g., DirectUpgradeRule) that implements isAllowed(currentFw firmwareVersion, upgrade FirmwareUpgrade) by comparing currentFw.cmp(upgrade.from) == 0 and has a summary that can be parameterized or accept a name, then replace DeltaUpgradeRule and LiteonUpgradeRule with instances of DirectUpgradeRule in newUpgradeRule (or add a brief comment explaining intentional duplication if you want to keep both types).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@powershelf-manager/pkg/common/vendor/vendor_test.go`:
- Around line 214-224: The test's long if-else chain (checking VendorCodeLiteon,
VendorCodeDelta, else VendorCodeUnsupported) should be replaced by storing
expected round-trip results on each test case and asserting against those
values; update the test case struct (the slice of tc entries used in this test)
to include expected Code and Name fields (e.g., ExpectedCode, ExpectedName) and
then replace the conditional block with simple assertions comparing v2.Code to
tc.ExpectedCode and v2.Name to tc.ExpectedName (references: VendorCodeLiteon,
VendorCodeDelta, VendorCodeUnsupported, v2, tc, v.Name).
In `@powershelf-manager/pkg/firmwaremanager/embedded_firmware.go`:
- Around line 110-111: Replace the use of log.Printf with a logrus
level-specific method to keep structured logging consistent: in
embedded_firmware.go where you log "no embedded firmware found for vendor %s;
firmware operations will be unavailable for this vendor" (using v.Name), change
the call to log.Warnf (or log.Infof if you prefer informational level) so the
message uses logrus's leveled/structured logging rather than Printf.
In `@powershelf-manager/pkg/firmwaremanager/firmware_manager.go`:
- Around line 67-68: Replace the stdlib log.Printf call with a structured logrus
warning; specifically, in firmware_manager.go where the loop logs "skipping
firmware support for vendor %s: %v" using vendor.Name and err, change this to
use logrus (e.g., log.WithField("vendor", vendor.Name).Warnf(...) or
log.WithFields(...).Warn(...)) so the vendor and error are recorded as
structured fields and the correct log level is used; update the call that
currently references vendor.Name and err to use those as structured
fields/message via logrus methods.
In `@powershelf-manager/pkg/firmwaremanager/firmware_updater.go`:
- Around line 63-65: Replace the direct call to log.Printf in
firmware_updater.go that reports an empty repo (the block checking
len(repo.upgrades) == 0 referencing repo.upgrades and v.Name) with a structured
logrus call such as log.Warnf or log.Infof so the message uses logrus levels and
structured logging (e.g., call log.Warnf("no firmware artifacts available for
vendor %s; firmware operations will be unavailable", v.Name)). Ensure you
import/use the same log variable used elsewhere in this package.
In `@powershelf-manager/pkg/firmwaremanager/firmware_upgrade.go`:
- Around line 65-74: DeltaUpgradeRule duplicates LiteonUpgradeRule behavior;
introduce a shared parameterized rule (e.g., DirectUpgradeRule) that implements
isAllowed(currentFw firmwareVersion, upgrade FirmwareUpgrade) by comparing
currentFw.cmp(upgrade.from) == 0 and has a summary that can be parameterized or
accept a name, then replace DeltaUpgradeRule and LiteonUpgradeRule with
instances of DirectUpgradeRule in newUpgradeRule (or add a brief comment
explaining intentional duplication if you want to keep both types).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 348ce2b9-b8da-4ba0-99be-6267dfc78ae7
⛔ Files ignored due to path filters (2)
powershelf-manager/internal/proto/v1/powershelf-manager.pb.gois excluded by!**/*.pb.gopowershelf-manager/internal/proto/v1/powershelf-manager_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (10)
powershelf-manager/internal/proto/v1/powershelf-manager.protopowershelf-manager/pkg/common/vendor/vendor.gopowershelf-manager/pkg/common/vendor/vendor_test.gopowershelf-manager/pkg/converter/protobuf/converter.gopowershelf-manager/pkg/converter/protobuf/converter_test.gopowershelf-manager/pkg/firmwaremanager/embedded_firmware.gopowershelf-manager/pkg/firmwaremanager/firmware_manager.gopowershelf-manager/pkg/firmwaremanager/firmware_repo.gopowershelf-manager/pkg/firmwaremanager/firmware_updater.gopowershelf-manager/pkg/firmwaremanager/firmware_upgrade.go
thossain-nv
changed the title
Contributor
|
@spydaNVIDIA I updated the title to be more descriptive. Since PR titles end up as git commit message, titles with clear context makes it much easier to understand commits looking at git history. |
Contributor
Author
Thanks @thossain-nv, I will keep this in mind going forward. |
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
2 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.
Description
Add support for the Delta vendor in PSM.
Type of Change
Services Affected
Related Issues (Optional)
Breaking Changes
Testing
Additional Notes