Skip to content

chore: Remove deprecated flat fields from NetworkSegment object - #4450

Draft
nvlitagaki wants to merge 3 commits into
NVIDIA:mainfrom
nvlitagaki:chore/remove-deprecated-subnet-fields
Draft

chore: Remove deprecated flat fields from NetworkSegment object#4450
nvlitagaki wants to merge 3 commits into
NVIDIA:mainfrom
nvlitagaki:chore/remove-deprecated-subnet-fields

Conversation

@nvlitagaki

Copy link
Copy Markdown
Contributor

Since the REST API no longer relies on the deprecated NetworkSegment (called Subnet in REST API docs) fields that were relocated to the Metadata, NetworkSegmentConfig and NetworkSegmentStatus, we can now go ahead and remove those fields. This work removes fallback logic to those deprecated fields that was present in the admin cli and updates tests as necessary.

Related issues

(#2796)

Type of Change

  • Change - Changes in existing functionality
  • Remove - Removed features or deprecated functionality

Breaking Changes

Direct clients of the gRPC API for VPCs will be forced to migrate to the structured fields, but we do not guarantee that the gRPC API is backward-compatible (https://github.com/NVIDIA/infra-controller/blob/main/docs/development/release_and_qa_process.md#what-is-explicitly-not-guaranteed)

  • This PR contains breaking changes

Testing

  • Unit tests added/updated

Additional Notes

@nvlitagaki
nvlitagaki requested a review from a team as a code owner July 31, 2026 18:46
@nvlitagaki nvlitagaki added this to the v2.2 milestone Jul 31, 2026
@nvlitagaki
nvlitagaki marked this pull request as draft July 31, 2026 18:47
@coderabbitai

coderabbitai Bot commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Summary by CodeRabbit

  • Breaking Changes
    • Network segment APIs no longer expose deprecated top-level fields such as name, VPC ID, prefixes, lifecycle state, version, and history.
    • Use the structured configuration, status, and metadata fields instead.
  • Improvements
    • CLI network segment details, history, and related instance information now consistently use structured network segment data.
    • Missing segment metadata is displayed with clear default values.
    • Network segment state history is retrieved through the current history interface.

Walkthrough

NetworkSegment removes deprecated flat protobuf fields and uses structured configuration, status, metadata, and dedicated history RPCs. RPC conversion, CLI output, test fixtures, and API tests now use the structured representation.

Changes

NetworkSegment structured-field migration

Layer / File(s) Summary
Structured NetworkSegment contracts and RPC conversion
crates/rpc/proto/forge.proto, rest-api/proto/core/src/v1/nico_nico.proto, crates/rpc/src/model/..., rest-api/site-workflow/...
Deprecated flat fields are removed and reserved. RPC conversion and test-server creation now populate structured fields. Inline history conversion is removed.
CLI structured-field consumption
crates/admin-cli/src/devenv/..., crates/admin-cli/src/instance/..., crates/admin-cli/src/network_segment/...
CLI commands read metadata, configuration, lifecycle status, and versions from structured fields. History retrieval uses the dedicated RPC without legacy fallback.
Structured-field test updates
crates/api-core/src/tests/network_segment.rs, crates/api-core/src/tests/network_segment_find.rs, rest-api/db/pkg/db/model/subnet_test.go
Tests remove legacy-field and inline-history assertions. Lifecycle version checks use structured status values.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Suggested labels: rest-api

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the removal of deprecated flat fields from the NetworkSegment object.
Description check ✅ Passed The description accurately explains the field removal, fallback cleanup, breaking change, and related test updates.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@copy-pr-bot

copy-pr-bot Bot commented Jul 31, 2026

Copy link
Copy Markdown

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.

@nvlitagaki

Copy link
Copy Markdown
Contributor Author

/ok to test eced758

@nvlitagaki

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 31, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@nvlitagaki nvlitagaki changed the title chore: Remove deprecated flag fields from NetworkSegment object chore: Remove deprecated flat fields from NetworkSegment object Jul 31, 2026
@github-actions

Copy link
Copy Markdown

@github-actions

Copy link
Copy Markdown

🔐 TruffleHog Secret Scan

No secrets or credentials found!

Your code has been scanned for 700+ types of secrets and credentials. All clear! 🎉

🔗 View scan details

🕐 Last updated: 2026-07-31 18:50:21 UTC | Commit: eced758

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
crates/admin-cli/src/network_segment/show/cmd.rs (1)

277-302: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Move the state-history fetch inside the non-JSON branch.

show_network_information fetches state history unconditionally at line 291, before the json check. The json branch only serializes segment, never history. If get_segment_state_history fails, the ? operator aborts the whole call, including the --json output path that does not need history data at all.

Move the history fetch into the else branch so a history-retrieval failure cannot break the JSON output path.

🔧 Proposed fix to scope the history fetch to the branch that needs it
-    let history = api_client.get_segment_state_history(segment_id).await?;
-
     if json {
         println!("{}", serde_json::to_string_pretty(&segment)?);
     } else {
+        let history = api_client.get_segment_state_history(segment_id).await?;
         println!(
             "{}",
             convert_network_to_nice_format(segment, Some(history), api_client).await?
         );
     }
🤖 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/network_segment/show/cmd.rs` around lines 277 - 302,
Move the get_segment_state_history call from the unconditional portion of
show_network_information into the non-JSON else branch, immediately before
convert_network_to_nice_format. Keep the JSON branch limited to serializing
segment so it does not depend on history retrieval.
🤖 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.

Outside diff comments:
In `@crates/admin-cli/src/network_segment/show/cmd.rs`:
- Around line 277-302: Move the get_segment_state_history call from the
unconditional portion of show_network_information into the non-JSON else branch,
immediately before convert_network_to_nice_format. Keep the JSON branch limited
to serializing segment so it does not depend on history retrieval.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: b59d3dac-8d12-433a-b7c3-62131dc135e2

📥 Commits

Reviewing files that changed from the base of the PR and between 5f7b57c and eced758.

⛔ Files ignored due to path filters (1)
  • rest-api/proto/core/gen/v1/nico_nico.pb.go is excluded by !**/*.pb.go, !**/gen/**, !rest-api/**/*.pb.go
📒 Files selected for processing (11)
  • crates/admin-cli/src/devenv/config/apply/cmd.rs
  • crates/admin-cli/src/instance/show/cmd.rs
  • crates/admin-cli/src/network_segment/show/cmd.rs
  • crates/api-core/src/tests/network_segment.rs
  • crates/api-core/src/tests/network_segment_find.rs
  • crates/rpc/proto/forge.proto
  • crates/rpc/src/model/network_segment.rs
  • crates/rpc/src/model/state_history.rs
  • rest-api/db/pkg/db/model/subnet_test.go
  • rest-api/proto/core/src/v1/nico_nico.proto
  • rest-api/site-workflow/pkg/grpc/server/nico_test_server.go
💤 Files with no reviewable changes (3)
  • crates/api-core/src/tests/network_segment_find.rs
  • crates/rpc/src/model/state_history.rs
  • rest-api/db/pkg/db/model/subnet_test.go

@thossain-nv thossain-nv added the rest-api Add this label when an issue or PR concerns NICo REST API label Jul 31, 2026 — with ChatGPT Codex Connector
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

rest-api Add this label when an issue or PR concerns NICo REST API

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants