chore(rest-api): Apply our FromProto/ToProto modeling to InfiniBandPartition#2116
Open
chet wants to merge 1 commit into
Open
chore(rest-api): Apply our FromProto/ToProto modeling to InfiniBandPartition#2116chet wants to merge 1 commit into
chet wants to merge 1 commit into
Conversation
🔐 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-02 18:11:28 UTC | Commit: 0c75763 |
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
chet
changed the title
Contributor
Author
|
@coderabbitai There is a .coderabbit.yaml in rest-api/.coderabbit.yaml in this repo that you can use, with a rest-api/AGENTS.md with general guidelines. This PR actually should be good to go based on previous work we've done, but looping you in for funsies anyway. |
Contributor
🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== .coderabbit.yaml ==="
fd -i '.coderabbit.yaml' rest-api/ | xargs cat 2>/dev/null || cat rest-api/.coderabbit.yaml 2>/dev/null || echo "NOT FOUND"
echo ""
echo "=== AGENTS.md ==="
cat rest-api/AGENTS.md 2>/dev/null || echo "NOT FOUND"Length of output: 28384 ⚙️ CodeRabbit configuration inspected Length of output: 6441
Kicking off the review now! ✏️ Learnings added
✅ Actions performedReview triggered.
|
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Read the CodeRabbit config and AGENTS.md guidelines
echo "=== rest-api/.coderabbit.yaml ==="
cat rest-api/.coderabbit.yaml 2>/dev/null || echo "FILE NOT FOUND"
echo ""
echo "=== rest-api/AGENTS.md ==="
cat rest-api/AGENTS.md 2>/dev/null || echo "FILE NOT FOUND"
echo ""
echo "=== PR diff (files changed) ==="
gh pr diff 2116 --name-only 2>/dev/null
echo ""
echo "=== PR diff ==="
gh pr diff 2116 2>/dev/nullLength of output: 104793
A couple of things worth naming explicitly since they're easy to miss:
Nothing to block on. LGTM. [approve] |
Contributor
WalkthroughThis PR introduces strong typing for InfiniBand Partition status handling by converting from plain ChangesInfiniBand Partition Status Enum and Typed Integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 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 |
![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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rest-api/api/pkg/api/handler/infinibandpartition.go (1)
1102-1104: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winPass the literal workflow name to
TerminateWorkflowOnTimeOut.The workflow name argument should be
"DeleteInfiniBandPartitionV2"(matching the workflow executed on line 1076) rather than the shortened"Delete"label. As per coding guidelines, "Pass the literal workflow name tocommon.TerminateWorkflowOnTimeOut(e.g.,"CreateInstanceV2"), not shortened action labels".Proposed fix
timeoutResp = func() error { - return common.TerminateWorkflowOnTimeOut(c, logger, stc, wid, wferr, "InfiniBandPartition", "Delete") + return common.TerminateWorkflowOnTimeOut(c, logger, stc, wid, wferr, "InfiniBandPartition", "DeleteInfiniBandPartitionV2") }🤖 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 `@rest-api/api/pkg/api/handler/infinibandpartition.go` around lines 1102 - 1104, The call to common.TerminateWorkflowOnTimeOut inside the timeoutResp closure uses the shortened action label "Delete" but should pass the literal workflow name; update the argument passed to common.TerminateWorkflowOnTimeOut in the timeoutResp definition to "DeleteInfiniBandPartitionV2" (matching the workflow executed earlier) so the signature used by TerminateWorkflowOnTimeOut accurately reflects the workflow name.
🧹 Nitpick comments (2)
rest-api/db/pkg/db/model/infinibandpartition_test.go (2)
876-918: ⚡ Quick winAssert the invalid-status failure explicitly.
Line 916 and Line 1052 only check that some error occurred, so these cases still pass on unrelated setup or DAO failures. Pin the regression to the validation path with
require.ErrorContains(..., "invalid InfiniBandPartition Status").Suggested tightening
- require.Equal(t, tt.wantErr, err != nil) - if tt.wantErr { - return - } + if tt.wantErr { + require.ErrorContains(t, err, "invalid InfiniBandPartition Status") + return + } + require.NoError(t, err)Also applies to: 1015-1055
🤖 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 `@rest-api/db/pkg/db/model/infinibandpartition_test.go` around lines 876 - 918, The test currently only asserts an error occurred for invalid status cases; change the assertion to explicitly check the validation error message from InfiniBandPartitionSQLDAO.Create by using require.ErrorContains(err, "invalid InfiniBandPartition Status") (or the exact validation text returned by Create) when tt.wantErr is true, so replace the generic require.Equal(t, tt.wantErr, err != nil) + return with a targeted require.ErrorContains check; apply the same change to the other identical test block around the 1015–1055 region.
1057-1057: ⚡ Quick winDrop the debug print from the update test.
Line 1057 writes to stdout on every passing run and adds CI log noise without improving 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 `@rest-api/db/pkg/db/model/infinibandpartition_test.go` at line 1057, Remove the stray debug fmt.Printf call in the update test in infinibandpartition_test.go (the line printing "got ID: ... Created: ... Updated: ..."); delete that printf so the test no longer writes to stdout on success and noise the CI logs while leaving all assertions intact.
🤖 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 `@rest-api/api/pkg/api/handler/instance_test.go`:
- Around line 4105-4106: The InfiniBand partition fixture ibp5 is created on st2
but the "doesn't have InfiniBand Capability" update case uses an instance on
st1, causing the test to fail for a site mismatch before hitting the capability
check; fix by creating ibp5 on st1 instead (update the testBuildIBPartition call
that constructs ibp5 to pass st1 rather than st2) so the test exercises the
capability-check branch as intended.
In `@rest-api/workflow/pkg/activity/infinibandpartition/infinibandpartition.go`:
- Around line 204-210: The code compares the partition's status against the
wrong "deleting" constant causing deleted partitions to be resurrected; replace
those checks so they use cdbm.InfiniBandPartitionStatusDeleting when comparing
the local status variable and ibp.Status before calling mibp.updateIBPStatusInDB
(and in the second occurrence near the later ibp.Status comparison), ensuring
both delete-state gates use cdbm.InfiniBandPartitionStatusDeleting rather than
the interface-deleting constant.
---
Outside diff comments:
In `@rest-api/api/pkg/api/handler/infinibandpartition.go`:
- Around line 1102-1104: The call to common.TerminateWorkflowOnTimeOut inside
the timeoutResp closure uses the shortened action label "Delete" but should pass
the literal workflow name; update the argument passed to
common.TerminateWorkflowOnTimeOut in the timeoutResp definition to
"DeleteInfiniBandPartitionV2" (matching the workflow executed earlier) so the
signature used by TerminateWorkflowOnTimeOut accurately reflects the workflow
name.
---
Nitpick comments:
In `@rest-api/db/pkg/db/model/infinibandpartition_test.go`:
- Around line 876-918: The test currently only asserts an error occurred for
invalid status cases; change the assertion to explicitly check the validation
error message from InfiniBandPartitionSQLDAO.Create by using
require.ErrorContains(err, "invalid InfiniBandPartition Status") (or the exact
validation text returned by Create) when tt.wantErr is true, so replace the
generic require.Equal(t, tt.wantErr, err != nil) + return with a targeted
require.ErrorContains check; apply the same change to the other identical test
block around the 1015–1055 region.
- Line 1057: Remove the stray debug fmt.Printf call in the update test in
infinibandpartition_test.go (the line printing "got ID: ... Created: ...
Updated: ..."); delete that printf so the test no longer writes to stdout on
success and noise the CI logs while leaving all assertions intact.
🪄 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: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 98bed9c4-0cfd-4a01-b6a0-d3870cc76d95
📒 Files selected for processing (14)
rest-api/api/pkg/api/handler/infinibandinterface_test.gorest-api/api/pkg/api/handler/infinibandpartition.gorest-api/api/pkg/api/handler/infinibandpartition_test.gorest-api/api/pkg/api/handler/instance_test.gorest-api/api/pkg/api/handler/instancebatch_test.gorest-api/api/pkg/api/handler/nvlinkinterface_test.gorest-api/api/pkg/api/model/infinibandpartition.gorest-api/api/pkg/api/model/infinibandpartition_test.gorest-api/db/pkg/db/model/helper_test.gorest-api/db/pkg/db/model/infinibandinterface_test.gorest-api/db/pkg/db/model/infinibandpartition.gorest-api/db/pkg/db/model/infinibandpartition_test.gorest-api/workflow/pkg/activity/infinibandpartition/infinibandpartition.gorest-api/workflow/pkg/util/testing.go
chet
force-pushed
the
proto-infinibandpartition-receivers
branch
from
0c75763 to
80e5a26
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
80e5a26 to
6b63e27
Compare
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
This is NVIDIA/infra-controller-rest#594, re-opened over here (and had already gone through @coderabbitai feedback loops, curious to see what happens here).
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 fix: health service health-report now uses coalescing queue #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
Related Issues (Optional)
Breaking Changes
Testing
Additional Notes
Summary by CodeRabbit
Bug Fixes
Refactor