feat(admin-cli): manage tenant SitePrefixes - #4745
Open
chet wants to merge 1 commit into
Open
Conversation
Contributor
Summary by CodeRabbit
WalkthroughThe admin CLI adds SitePrefix commands for creation, update, deletion, search, display, and state-history retrieval. It adds RPC helpers, structured output formats, command dispatch, validation, and parsing tests. ChangesSitePrefix CLI
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SitePrefixArgs
participant RuntimeContext
participant ApiClient
participant SitePrefixOutput
SitePrefixArgs->>RuntimeContext: Run SitePrefix command
RuntimeContext->>ApiClient: Search or fetch SitePrefix data
ApiClient-->>RuntimeContext: Return SitePrefix records
RuntimeContext->>SitePrefixOutput: Write configured output format
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
Author
|
@coderabbitai full_review, thanks! |
Contributor
|
🐇 ✅ Action performedFull review finished. |
![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: 4
🤖 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/admin-cli/src/site_prefix/create.rs`:
- Around line 57-62: The prefix argument in the create command currently parses
any IpNet; add a Clap value parser for prefix that reuses the existing IPv4,
RFC1918, and prefix-length policy, accepting only canonical private IPv4
networks with lengths /8 through /31 and rejecting IPv6, public ranges, /0, and
/32 before RPC submission. Add focused CLI parsing tests covering valid and
invalid boundaries.
In `@crates/admin-cli/src/site_prefix/mod.rs`:
- Around line 34-48: Add the single-resource lookup example to the
`after_long_help` examples for the `site-prefix` command, using a realistic UUID
with `nico-admin-cli site-prefix show <id>`. Keep the existing inventory `show`
example and other examples unchanged.
- Line 49: Update the Clap attribute on the relevant command or argument type by
replacing the invalid rename_all value "kebab_case" with "kebab-case", or remove
the attribute since kebab-case is already the default.
In `@crates/admin-cli/src/site_prefix/show.rs`:
- Line 132: Replace the unsupported "kebab_case" Clap rename style with
"kebab-case" in the derive attributes for both RoutingScopeArg and
LifecycleStateArg at crates/admin-cli/src/site_prefix/show.rs lines 132-132 and
146-146.
🪄 Autofix
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: 42587f1f-48d7-49db-bc53-a2c6d4a2887b
📒 Files selected for processing (14)
crates/admin-cli/cli_domains.yamlcrates/admin-cli/src/cfg/cli_options.rscrates/admin-cli/src/errors.rscrates/admin-cli/src/main.rscrates/admin-cli/src/rpc.rscrates/admin-cli/src/site_prefix/common.rscrates/admin-cli/src/site_prefix/create.rscrates/admin-cli/src/site_prefix/delete.rscrates/admin-cli/src/site_prefix/mod.rscrates/admin-cli/src/site_prefix/output.rscrates/admin-cli/src/site_prefix/show.rscrates/admin-cli/src/site_prefix/state_history.rscrates/admin-cli/src/site_prefix/tests.rscrates/admin-cli/src/site_prefix/update.rs
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/admin-cli/src/site_prefix/state_history.rs (1)
55-62: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winImplement
From<StateHistoryRecord>for this conversion.
HistoryRecordView::from_recordis a bespoke conversion method. ImplementFrom<StateHistoryRecord>and use.map(HistoryRecordView::from).Proposed refactor
-impl HistoryRecordView { - fn from_record(record: StateHistoryRecord) -> Self { +impl From<StateHistoryRecord> for HistoryRecordView { + fn from(record: StateHistoryRecord) -> Self { Self { state: display_state(&record.state), version: record.version, time: record.time.map(|time| time.to_string()), } } +} +impl HistoryRecordView { fn table(records: &[Self]) -> Table {- .map(HistoryRecordView::from_record) + .map(HistoryRecordView::from)As per coding guidelines, “Prefer
From/TryFromandFromStrconversions over bespoke conversion methods.” As per path instructions, prefer “idiomatic From/TryFrom conversions.”Also applies to: 110-118
🤖 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/site_prefix/state_history.rs` around lines 55 - 62, Replace the bespoke HistoryRecordView::from_record conversion with an impl From<StateHistoryRecord> for HistoryRecordView, preserving the existing state, version, and time mappings. Update the related collection conversion to use .map(HistoryRecordView::from) and remove or stop calling from_record.Sources: Coding guidelines, 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.
Nitpick comments:
In `@crates/admin-cli/src/site_prefix/state_history.rs`:
- Around line 55-62: Replace the bespoke HistoryRecordView::from_record
conversion with an impl From<StateHistoryRecord> for HistoryRecordView,
preserving the existing state, version, and time mappings. Update the related
collection conversion to use .map(HistoryRecordView::from) and remove or stop
calling from_record.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: bda640ef-9962-4634-a507-c42b644adaaf
📒 Files selected for processing (14)
crates/admin-cli/cli_domains.yamlcrates/admin-cli/src/cfg/cli_options.rscrates/admin-cli/src/errors.rscrates/admin-cli/src/main.rscrates/admin-cli/src/rpc.rscrates/admin-cli/src/site_prefix/common.rscrates/admin-cli/src/site_prefix/create.rscrates/admin-cli/src/site_prefix/delete.rscrates/admin-cli/src/site_prefix/mod.rscrates/admin-cli/src/site_prefix/output.rscrates/admin-cli/src/site_prefix/show.rscrates/admin-cli/src/site_prefix/state_history.rscrates/admin-cli/src/site_prefix/tests.rscrates/admin-cli/src/site_prefix/update.rs
Add the admin CLI surface for tenant-managed SitePrefixes now that Core owns their lifecycle. Operators can create, list, update, retire, and inspect state history without relying on CIDR as identity. Exact-CIDR searches stay collection-valued because multiple tenants may own the same address space, while mutations require the SitePrefix ID and owning tenant. This supports NVIDIA#4226 Signed-off-by: Chet Nichols III <chetn@nvidia.com>
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.
This adds
nico-admin-cli site-prefixcommands to list or show SitePrefixes, create tenant-managed prefixes, replace their metadata, retire them, and inspect lifecycle history.SitePrefix IDs remain the identity because multiple tenants can own the same CIDR. Exact-CIDR searches therefore return collections, with optional tenant scoping.
deleterecords retirement intent and returns the retainedDeletingresource; it does not force-delete address space that may still have child resources or dataplane state.Related issues
This supports #4226
Type of Change
Breaking Changes
Testing
Additional Notes
Verified the top-level command and the
show,create,update,delete, andstate-historyhelp output directly. The generated CLI reference belongs in the repository workflow's companion documentation Task and PR.