This repository was archived by the owner on Jun 2, 2026. It is now read-only.
chore: Apply the new FromProto/ToProto modeling to InfiniBandPartition#594
Closed
chet wants to merge 1 commit into
Closed
chore: Apply the new FromProto/ToProto modeling to InfiniBandPartition#594chet wants to merge 1 commit into
chet wants to merge 1 commit into
Conversation
Contributor
Summary by CodeRabbit
WalkthroughThis PR converts InfiniBandPartition status handling to a typed cdbm.InfiniBandPartitionStatus enum across DB models, API models/handlers, workflow activity, and tests; adds API ToProto helpers for workflow requests; and updates validation and test fixtures to use typed status pointers. ChangesInfiniBandPartition Status Typed Enum Refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 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 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-06-01 18:32:26 UTC | Commit: 7b4ef7e |
![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)
workflow/pkg/activity/infinibandpartition/infinibandpartition.go (1)
199-202:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReplace the cross-domain
Deletingcomparison withInfiniBandPartitionStatusDeleting
ibp.Statusiscdbm.InfiniBandPartitionStatus, andcdbm.InfiniBandInterfaceStatusDeletingis an untyped string constant, so the current comparison type-checks—but it relies on both domains using the same"Deleting"value and obscures intent. Usecdbm.InfiniBandPartitionStatusDeletingat both occurrences (lines 200 and 244).🐛 Proposed correction (apply at both Line 200 and Line 244)
- if ibp.Status == cdbm.InfiniBandInterfaceStatusDeleting { + if ibp.Status == cdbm.InfiniBandPartitionStatusDeleting {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@workflow/pkg/activity/infinibandpartition/infinibandpartition.go` around lines 199 - 202, The code compares ibp.Status to the wrong cross-domain constant cdbm.InfiniBandInterfaceStatusDeleting; replace both comparisons (the one referencing ibp.Status and the similar one later) with cdbm.InfiniBandPartitionStatusDeleting to use the correct InfiniBandPartitionStatus enum; update the two occurrences that currently use cdbm.InfiniBandInterfaceStatusDeleting so they read comparisons against cdbm.InfiniBandPartitionStatusDeleting (references: controllerIbp.Status, ibp.Status, cdbm.InfiniBandPartitionStatusDeleting).
🧹 Nitpick comments (1)
db/pkg/db/model/infinibandpartition_test.go (1)
1057-1057: ⚡ Quick winRemove the leftover debug print from the update test.
This writes to stdout on every successful run and adds noise without strengthening the assertion set.
Suggested cleanup
- fmt.Printf("\ngot ID: %v, Created: %v, Updated: %v", got.ID.String(), got.Created, got.Updated)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@db/pkg/db/model/infinibandpartition_test.go` at line 1057, Remove the leftover debug print: delete the fmt.Printf("\ngot ID: %v, Created: %v, Updated: %v", got.ID.String(), got.Created, got.Updated) call in the update test (the line referencing fmt.Printf and the local variable got) in infinibandpartition_test.go; if you want non-noisy test output keep a t.Logf instead but preferably remove the print altogether so tests don't write to stdout on success.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@db/pkg/db/model/infinibandinterface_test.go`:
- Line 725: Replace the partition status constant usage with the interface
status constant in the test: instead of setting uInfiniBandInterface.Status =
string(InfiniBandPartitionStatusPending), set it to the corresponding
InfiniBandInterface status constant (e.g., InfiniBandInterfaceStatusPending or
whatever constant is defined for interface.Status) so the test uses the correct
domain-specific constant; update the assignment in the test where
uInfiniBandInterface.Status is set and ensure any helper expectations/assertions
reference the same interface status constant.
In `@db/pkg/db/model/infinibandpartition.go`:
- Around line 148-161: The Create and Update DAO methods must call the model's
Validate() to enforce the new invariants (name length/whitespace and status
membership) before persisting changes: in the Create function (where the
InfiniBandPartition entity is assembled) call ibp.Validate() and return the
validation error instead of only checking Status; in the Update function
validate the patched view (the new Name and Status values combined into an
InfiniBandPartition or a small struct) with Validate() before executing the SQL
update so leading/trailing whitespace or too-short names are rejected; reference
the existing InfiniBandPartition.Validate and validateNameWhitespace helpers and
ensure both Create and Update propagate validation errors to callers.
In `@workflow/pkg/activity/infinibandpartition/infinibandpartition.go`:
- Around line 269-273: The log uses the stale outer variable err instead of the
local error serr returned by mibp.updateIBPStatusInDB; update the logger call in
the block after calling updateIBPStatusInDB (where serr is set) to pass serr to
slogger.Error().Err(...) and keep the same message (i.e., replace Err(err) with
Err(serr)) so the actual DB update error is logged; reference the variables serr
and err and the method updateIBPStatusInDB to locate the change.
---
Outside diff comments:
In `@workflow/pkg/activity/infinibandpartition/infinibandpartition.go`:
- Around line 199-202: The code compares ibp.Status to the wrong cross-domain
constant cdbm.InfiniBandInterfaceStatusDeleting; replace both comparisons (the
one referencing ibp.Status and the similar one later) with
cdbm.InfiniBandPartitionStatusDeleting to use the correct
InfiniBandPartitionStatus enum; update the two occurrences that currently use
cdbm.InfiniBandInterfaceStatusDeleting so they read comparisons against
cdbm.InfiniBandPartitionStatusDeleting (references: controllerIbp.Status,
ibp.Status, cdbm.InfiniBandPartitionStatusDeleting).
---
Nitpick comments:
In `@db/pkg/db/model/infinibandpartition_test.go`:
- Line 1057: Remove the leftover debug print: delete the fmt.Printf("\ngot ID:
%v, Created: %v, Updated: %v", got.ID.String(), got.Created, got.Updated) call
in the update test (the line referencing fmt.Printf and the local variable got)
in infinibandpartition_test.go; if you want non-noisy test output keep a t.Logf
instead but preferably remove the print altogether so tests don't write to
stdout on success.
🪄 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: Enterprise
Run ID: 01ef1eec-26f9-40e4-bf4f-375ce4ac8b4e
📒 Files selected for processing (14)
api/pkg/api/handler/infinibandinterface_test.goapi/pkg/api/handler/infinibandpartition.goapi/pkg/api/handler/infinibandpartition_test.goapi/pkg/api/handler/instance_test.goapi/pkg/api/handler/instancebatch_test.goapi/pkg/api/handler/nvlinkinterface_test.goapi/pkg/api/model/infinibandpartition.goapi/pkg/api/model/infinibandpartition_test.godb/pkg/db/model/helper_test.godb/pkg/db/model/infinibandinterface_test.godb/pkg/db/model/infinibandpartition.godb/pkg/db/model/infinibandpartition_test.goworkflow/pkg/activity/infinibandpartition/infinibandpartition.goworkflow/pkg/util/testing.go
chet
force-pushed
the
proto-infinibandpartition-receivers
branch
from
7b4ef7e to
6d7e144
Compare
Brings `InfiniBandPartition` in line with the proto-modeling changes. The handler keeps its REST surface; Create and Update route through `ToProto` on the API request types and the entity gains the full round-trip plus a delete-request builder. The entity also picks up the typed `Labels` field per the `InstanceType` reference in NVIDIA#540 -- it reaches `Labels.ToProto()` / `Labels.FromProto(...)` directly. Primary callouts are: - API request side: `APIInfiniBandPartitionCreateRequest.ToProto(ibp)` and `APIInfiniBandPartitionUpdateRequest.ToProto(ibp)`; both source corresponding fields via `ibp.ToProto()`. - Entity side: `InfiniBandPartition.ToProto`, `FromProto`, and `ToDeletionRequestProto`. There's also a new `InfiniBandPartition.Validate()` so site-driven `FromProto` callers can do `FromProto` + `Validate` -- mirroring the MachineCapability pattern (which went in at NVIDIA#569). - `InfiniBandPartitionStatus` is now a typed string with its own `.FromProto(state)` and `.Message()` methods, replacing the `InfiniBandPartitionStatusFromProto(state)` helper that split a single proto state into `(status, message)`. The workflow activity caller is just a two-liner now. - Handler simplified to one-liners, and the inline `cwssaws` / `model/util` imports drop. As part of the new typed `InfiniBandPartitionStatus` (and the already-typed `Labels`), the typing propagates through the API and DAO layers, but nothing changes on the wire -- JSON serialization of `type X string` is identical to `string`, the OpenAPI spec still just says "string", and the SDK is unaffected. Tests added! Signed-off-by: Chet Nichols III <chetn@nvidia.com>
chet
force-pushed
the
proto-infinibandpartition-receivers
branch
from
6d7e144 to
b4c507e
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
db/pkg/db/model/infinibandinterface_test.go (1)
782-782: ⚡ Quick winDrop debug print from update test path.
Line 782 writes directly to stdout during tests. This is noisy and unnecessary once assertions are in place.
Proposed cleanup
- fmt.Printf("\ngot ID: %v, Created: %v, Updated: %v", got.ID.String(), got.Created, got.Updated)If this is removed,
fmtcan likely be dropped from imports in this file.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@db/pkg/db/model/infinibandinterface_test.go` at line 782, Remove the debug stdout print in the update test (the fmt.Printf line that prints "got ID...") so the test no longer writes to stdout; after removing that statement also delete the now-unused fmt import from infinibandinterface_test.go (or run `go vet`/`go test` to catch the unused import) to keep imports clean.db/pkg/db/model/infinibandpartition_test.go (1)
1057-1057: ⚡ Quick winRemove debug stdout from DAO update test.
Line 1057 prints runtime values during normal test execution. This creates noisy CI logs and obscures failures.
Proposed cleanup
- fmt.Printf("\ngot ID: %v, Created: %v, Updated: %v", got.ID.String(), got.Created, got.Updated)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@db/pkg/db/model/infinibandpartition_test.go` at line 1057, Remove the debug stdout print in the DAO update test by deleting the fmt.Printf("\ngot ID: %v, Created: %v, Updated: %v", got.ID.String(), got.Created, got.Updated) line in infinibandpartition_test.go; if you want retained test diagnostics, replace it with t.Logf(...) inside the test function so output is only shown on failure rather than always printing to stdout.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@db/pkg/db/model/infinibandinterface_test.go`:
- Line 782: Remove the debug stdout print in the update test (the fmt.Printf
line that prints "got ID...") so the test no longer writes to stdout; after
removing that statement also delete the now-unused fmt import from
infinibandinterface_test.go (or run `go vet`/`go test` to catch the unused
import) to keep imports clean.
In `@db/pkg/db/model/infinibandpartition_test.go`:
- Line 1057: Remove the debug stdout print in the DAO update test by deleting
the fmt.Printf("\ngot ID: %v, Created: %v, Updated: %v", got.ID.String(),
got.Created, got.Updated) line in infinibandpartition_test.go; if you want
retained test diagnostics, replace it with t.Logf(...) inside the test function
so output is only shown on failure rather than always printing to stdout.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4ac6e0e5-9c5a-401b-a0fa-cc31b7cc96a7
📒 Files selected for processing (14)
api/pkg/api/handler/infinibandinterface_test.goapi/pkg/api/handler/infinibandpartition.goapi/pkg/api/handler/infinibandpartition_test.goapi/pkg/api/handler/instance_test.goapi/pkg/api/handler/instancebatch_test.goapi/pkg/api/handler/nvlinkinterface_test.goapi/pkg/api/model/infinibandpartition.goapi/pkg/api/model/infinibandpartition_test.godb/pkg/db/model/helper_test.godb/pkg/db/model/infinibandinterface_test.godb/pkg/db/model/infinibandpartition.godb/pkg/db/model/infinibandpartition_test.goworkflow/pkg/activity/infinibandpartition/infinibandpartition.goworkflow/pkg/util/testing.go
🚧 Files skipped from review as they are similar to previous changes (12)
- workflow/pkg/util/testing.go
- api/pkg/api/handler/instancebatch_test.go
- api/pkg/api/handler/nvlinkinterface_test.go
- api/pkg/api/handler/infinibandinterface_test.go
- api/pkg/api/handler/instance_test.go
- api/pkg/api/handler/infinibandpartition_test.go
- db/pkg/db/model/helper_test.go
- api/pkg/api/model/infinibandpartition_test.go
- api/pkg/api/handler/infinibandpartition.go
- workflow/pkg/activity/infinibandpartition/infinibandpartition.go
- db/pkg/db/model/infinibandpartition.go
- api/pkg/api/model/infinibandpartition.go
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
21 tasks
Contributor
Author
|
Going to re-open this on the infra-controller side! |
10 tasks
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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
Brings
InfiniBandPartitionin line with the proto-modeling changes. The handler keeps its REST surface; Create and Update route throughToProtoon the API request types and the entity gains the full round-trip plus a delete-request builder. The entity also picks up the typedLabelsfield per theInstanceTypereference in #540 -- it reachesLabels.ToProto()/Labels.FromProto(...)directly.Primary callouts are:
APIInfiniBandPartitionCreateRequest.ToProto(ibp)andAPIInfiniBandPartitionUpdateRequest.ToProto(ibp); both source corresponding fields viaibp.ToProto().InfiniBandPartition.ToProto,FromProto, andToDeletionRequestProto. There's also a newInfiniBandPartition.Validate()so site-drivenFromProtocallers can doFromProto+Validate-- mirroring the MachineCapability pattern (which went in at chore: Apply the new FromProto/ToProto modeling to MachineCapability #569).InfiniBandPartitionStatusis now a typed string with its own.FromProto(state)and.Message()methods, replacing theInfiniBandPartitionStatusFromProto(state)helper that split a single proto state into(status, message). The workflow activity caller is just a two-liner now.cwssaws/model/utilimports drop.As part of the new typed
InfiniBandPartitionStatus(and the already-typedLabels), the typing propagates through the API and DAO layers, but nothing changes on the wire -- JSON serialization oftype X stringis identical tostring, the OpenAPI spec still just says "string", and the SDK is unaffected.Tests added!
Signed-off-by: Chet Nichols III chetn@nvidia.com
Type of Change
Services Affected
Related Issues (Optional)
Breaking Changes
Testing
Additional Notes