feat(rack): list configured rack profiles - #4425
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Summary by CodeRabbit
WalkthroughThis change adds a Forge RPC and ChangesRack profile listing
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant AdminCLI
participant ApiClient
participant Forge
participant RackHandler
participant RuntimeConfiguration
Operator->>AdminCLI: run rack profile list
AdminCLI->>ApiClient: list_rack_profiles()
ApiClient->>Forge: ListRackProfiles(empty request)
Forge->>RackHandler: list_rack_profiles()
RackHandler->>RuntimeConfiguration: read configured rack_profiles
RackHandler-->>Forge: sorted profile response
Forge-->>ApiClient: ListRackProfilesResponse
ApiClient-->>AdminCLI: configured profiles
AdminCLI-->>Operator: render JSON, YAML, empty message, or table
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/api-core/tests/integration/rack_profile.rs (1)
79-138: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse one table-driven integration test for the listing cases.
These tests call
ListRackProfileswith different runtime configurations. Combine the populated and empty cases into one scenario table. Keep each row’s expected ordered IDs and profile payloads explicit.🤖 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 `@crates/api-core/tests/integration/rack_profile.rs` around lines 79 - 138, Combine list_rack_profiles_returns_configured_profiles_in_id_order and list_rack_profiles_returns_empty_list_for_empty_configuration into one table-driven integration test for ListRackProfiles. Define cases for populated and empty configurations, with each case explicitly specifying the expected ordered rack profile IDs and profile payloads, then iterate through the cases while preserving the existing response assertions.Source: Coding guidelines
🤖 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 `@docs/manuals/nico-admin-cli/commands/rack/rack-profile-list.md`:
- Around line 14-16: Update the DESCRIPTION section of the rack profile list
command documentation to state that it lists effective runtime-configured
profiles and that these profiles are not persisted rack resources.
---
Nitpick comments:
In `@crates/api-core/tests/integration/rack_profile.rs`:
- Around line 79-138: Combine
list_rack_profiles_returns_configured_profiles_in_id_order and
list_rack_profiles_returns_empty_list_for_empty_configuration into one
table-driven integration test for ListRackProfiles. Define cases for populated
and empty configurations, with each case explicitly specifying the expected
ordered rack profile IDs and profile payloads, then iterate through the cases
while preserving the existing response assertions.
🪄 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: 213f2393-8867-44df-a3da-280b69fc5ddd
📒 Files selected for processing (17)
crates/admin-cli/src/rack/profile/args.rscrates/admin-cli/src/rack/profile/list/args.rscrates/admin-cli/src/rack/profile/list/cmd.rscrates/admin-cli/src/rack/profile/list/mod.rscrates/admin-cli/src/rack/profile/mod.rscrates/admin-cli/src/rack/tests.rscrates/admin-cli/src/rpc.rscrates/api-core/src/api.rscrates/api-core/src/auth/internal_rbac_rules.rscrates/api-core/src/handlers/rack.rscrates/api-core/tests/integration/main.rscrates/api-core/tests/integration/rack_profile.rscrates/rpc/build.rscrates/rpc/proto/forge.protocrates/rpc/src/model/rack_type.rsdocs/manuals/nico-admin-cli/commands/rack/rack-profile-list.mddocs/manuals/nico-admin-cli/commands/rack/rack-profile.md
989d044 to
f0c75b0
Compare
|
🌿 Preview your docs: https://nvidia-preview-pull-request-4425.docs.buildwithfern.com/infra-controller |
🔐 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-07-31 15:13:42 UTC | Commit: f0c75b0 |
f0c75b0 to
6608d9b
Compare
|
Should this also be implemented as a REST API endpoint given that the plan is to move admin operations to REST at some point? |
|
@kfelternv Can you include this for the next batch of REST API/TUI exposure of gRPC API? |
polarweasel
left a comment
There was a problem hiding this comment.
I know that the manual files are generated, so pretend I put my suggestion in the appropriate source file 😂
Signed-off-by: Dmitry Porokh <dporokh@nvidia.com>
Co-authored-by: Alex Ball <awball@polarweasel.org> Signed-off-by: Dmitry Poroh <dporoh@pm.me>
f2ff825 to
c54c9f5
Compare
Signed-off-by: Dmitry Porokh <dporokh@nvidia.com>
379f775 to
9557bd8
Compare
Signed-off-by: Dmitry Porokh <dporokh@nvidia.com>
9557bd8 to
a1ec8dd
Compare
…rack-profiles # Conflicts: # rest-api/proto/core/gen/v1/nico_nico.pb.go
…rack-profiles # Conflicts: # rest-api/proto/core/gen/v1/nico_nico.pb.go
Creating an expected rack requires a valid rack_profile_id, but clients previously could only inspect the effective profile of an existing rack through GetRackProfile.
This PR adds ListRackProfiles to the Rack API, allowing clients to discover all configured rack profile IDs and their definitions before calling AddExpectedRack. Results are sorted by profile ID and come directly from runtime configuration, so no existing rack database records are required.
It also adds nico-admin-cli rack profile list, with concise table output and complete profile definitions in JSON or YAML. The existing rack profile show behavior remains unchanged.
Related issues
Closes #4421
Type of Change
Breaking Changes
Testing
Additional Notes
Rack profiles remain read-only runtime configuration. This change does not introduce persisted profile resources or profile mutation APIs.