chore(core): tenant_organization_id for OS must be optional to allow clean ownership attribution to tenant or provider#3689
Open
pbreton wants to merge 3 commits into
Conversation
…clear separation of tenant and provider owned OS in core Signed-off-by: Patrice Breton <pbreton@nvidia.com>
pbreton
marked this pull request as draft
July 17, 2026 19:57
|
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. |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Summary by CodeRabbit
WalkthroughOperating system organization ownership is now optional across protobuf contracts, database storage, API creation, CLI arguments, display adapters, iPXE rendering, and associated tests. Omitted organizations persist as ChangesNullable organization ownership
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant AdminCLI
participant create_operating_system
participant OperatingSystemDB
participant operating_system_row_to_ipxe_script
AdminCLI->>create_operating_system: submit optional tenant_organization_id
create_operating_system->>create_operating_system: reject provided empty organization
create_operating_system->>OperatingSystemDB: persist organization or NULL
OperatingSystemDB-->>create_operating_system: return optional organization
create_operating_system->>operating_system_row_to_ipxe_script: render operating system row
operating_system_row_to_ipxe_script-->>AdminCLI: return rendered script data
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
🔐 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-17 20:03:35 UTC | Commit: eb18a6a |
![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: 1
🧹 Nitpick comments (2)
crates/api-core/src/handlers/operating_system.rs (1)
273-280: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the shared
NicoErrorpath for this validation.This new user-input validation branch returns
tonic::Statusdirectly. Core handler guidance calls forNicoError::InvalidArgumentconverted with.into(), keeping error mapping consistent across handlers.As per path instructions, handler validation should use uniform
NicoErrorerrors and.into()rather than rawStatus.🤖 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/src/handlers/operating_system.rs` around lines 273 - 280, Update the tenant_organization_id validation branch in the relevant operating-system handler to return NicoError::InvalidArgument and convert it with .into(), replacing the direct Status::invalid_argument construction while preserving the existing validation message.Source: Path instructions
crates/admin-cli/src/operating_system/create/args.rs (1)
39-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd worked
after_long_helpexamples for the optional--orgcontract.This change introduces two meaningful modes: omit
--orgfor provider-owned definitions or provide it for tenant-owned definitions. Add the requiredEXAMPLES:help section with realistic invocations for both modes.As per path instructions, admin CLI argument structs must include realistic worked examples for meaningful command modes.
🤖 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/admin-cli/src/operating_system/create/args.rs` around lines 39 - 44, Add an after_long_help section to the org argument definition with an EXAMPLES: heading and realistic worked command invocations showing both modes: omitting --org for provider-owned OS definitions and supplying --org for tenant-owned definitions. Keep the existing optional org behavior and help text unchanged.Source: Path instructions
🤖 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 `@crates/api-db/src/operating_system.rs`:
- Around line 421-443: Combine test_create_persists_null_org and
test_create_persists_org into one table-driven persistence test covering both
None and Some("acme") organization cases. Define explicit input and expected
organization values, then iterate each case through the existing create/get flow
while preserving assertions for both created.org and fetched.org.
---
Nitpick comments:
In `@crates/admin-cli/src/operating_system/create/args.rs`:
- Around line 39-44: Add an after_long_help section to the org argument
definition with an EXAMPLES: heading and realistic worked command invocations
showing both modes: omitting --org for provider-owned OS definitions and
supplying --org for tenant-owned definitions. Keep the existing optional org
behavior and help text unchanged.
In `@crates/api-core/src/handlers/operating_system.rs`:
- Around line 273-280: Update the tenant_organization_id validation branch in
the relevant operating-system handler to return NicoError::InvalidArgument and
convert it with .into(), replacing the direct Status::invalid_argument
construction while preserving the existing validation message.
🪄 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: c96e07dd-8b3c-4706-b5dc-fb6601ab2e05
⛔ Files ignored due to path filters (1)
rest-api/proto/core/gen/v1/nico_nico.pb.gois excluded by!**/*.pb.go,!**/gen/**,!rest-api/**/*.pb.go
📒 Files selected for processing (14)
crates/admin-cli/src/operating_system/common.rscrates/admin-cli/src/operating_system/create/args.rscrates/admin-cli/src/operating_system/show/cmd.rscrates/api-core/src/handlers/operating_system.rscrates/api-core/src/ipxe.rscrates/api-core/src/tests/instance_os.rscrates/api-core/tests/integration/operating_system.rscrates/api-db/migrations/20260717120000_operating_systems_org_nullable.sqlcrates/api-db/src/instance.rscrates/api-db/src/operating_system.rscrates/api-model/src/operating_system_definition.rscrates/api-web/src/operating_system.rscrates/rpc/proto/forge.protorest-api/proto/core/src/v1/nico_nico.proto
Signed-off-by: Patrice Breton <pbreton@nvidia.com>
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
pbreton
marked this pull request as ready for review
July 17, 2026 21:46
Contributor
Author
|
/retest |
1 similar comment
Contributor
Author
|
/retest |
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.
Initial implementation of templated OS assumed that any core-created OS (using gRPC API) not existing on the REST layer would be considered as 'owned' by provider (vs tenant). tenant_organization_id is effectively ignored during reconciliation when being OS is added to REST db.
This provides a cleaner model allowing for provider and tenant owned templated OS when created on the core side (instead of provider-only):
Type of Change
Breaking Changes
Testing
Additional Notes
This is just the core/Rust side.