Skip to content

Conversation

@galkremer1
Copy link
Member

@galkremer1 galkremer1 commented Nov 11, 2025

📝 Description

  • Display the correct network name when editing a current network interface
  • Enable the user to switch from pod networking to an NAD network and vice versa
  • Code refactoring for the network interface component

Jira Ticket: CNV-71166

🎥 Demo

Before:

Before.mov

After:

After.mov

Summary by CodeRabbit

  • Bug Fixes

    • Improved network interface type switching to properly manage configuration properties when transitioning between different network types.
  • Refactor

    • Enhanced network interface components and selection logic for improved code organization and maintainability.

@openshift-ci-robot
Copy link
Collaborator

openshift-ci-robot commented Nov 11, 2025

@galkremer1: This pull request references CNV-71166 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.21.0" version, but no target version was set.

In response to this:

📝 Description

  • Display the correct network name when editing a current network interface
  • Enable the user to switch from pod networking to an NAD network and vice versa
  • Code refactoring for the network interface component

🎥 Demo

Before:

Before.mov

After:

After.mov

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

@openshift-ci openshift-ci bot requested review from avivtur and upalatucci November 11, 2025 14:49
@openshift-ci openshift-ci bot added the approved This issue is something we want to fix label Nov 11, 2025
@coderabbitai
Copy link

coderabbitai bot commented Nov 11, 2025

Walkthrough

Network interface modal components refactored to derive network state directly from VM properties, eliminating redundant props. New utilities extracted for network name parsing, NAD filtering, and option building. Network patching logic updated to explicitly handle type switching between pod and multus network configurations.

Changes

Cohort / File(s) Summary
Modal Component
src/utils/components/NetworkInterfaceModal/NetworkInterfaceModal.tsx
Removed public props Header and isEdit from NetworkInterfaceModalProps. Reordered interfaceLinkState initialization to default to UP when network absent. Simplified onSubmitModal with optional chaining and passed nicName to NetworkInterfaceNetworkSelect.
Network Select Component
src/utils/components/NetworkInterfaceModal/components/NetworkInterfaceNetworkSelect/NetworkInterfaceNetworkSelect.tsx
Updated component signature: replaced editInitValueNetworkName with nicName and vm parameters, changed isEditing to required boolean. Refactored data flow to derive originalNetworkNameToUse from VM state. Replaced inline filtering and option-building logic with new utility calls (filterNADs, buildNetworkOptions, getInterfaceTypeFromValue). Updated NAD filtering and network option generation logic.
Network Utilities
src/utils/components/NetworkInterfaceModal/components/NetworkInterfaceNetworkSelect/utils.tsx
Added new exported utilities: parseNetworkName, getOriginalNetworkName, getCurrentlyUsedNADsNames, isNadCurrentlyUsed, shouldIncludeNadWhenEditing, filterNADs, createNadOption, createPodNetworkOption, createOriginalNetworkOption, buildNetworkOptions, getInterfaceTypeFromValue. Introduced types ParsedNetworkName and BuildNetworkOptionsParams. Refactored getCreateNetworkOption to use precomputed parts.
Network Patching Utility
src/utils/resources/vm/utils/network/patch.ts
Replaced merge-based network updates with explicit patch sequences that test current value and conditionally remove legacy interface type properties during type switching. Removed mergeAndUpdateAtIndex utility. Added internal removeInterfaceTypeProperties helper. Updated updateInterface and updateNetwork to use test-replace patches with type-cleaning logic.

Sequence Diagram(s)

sequenceDiagram
    participant Modal as NetworkInterfaceModal
    participant Select as NetworkInterfaceNetworkSelect
    participant Utils as utils (Network Parsing)
    participant VM as VM State

    Modal->>Select: nicName, vm, isEditing
    Select->>Utils: getOriginalNetworkName(vm, nicName)
    Utils-->>Select: originalNetworkNameToUse
    
    Select->>Utils: parseNetworkName(currentNetworkName)
    Utils-->>Select: ParsedNetworkName
    
    Select->>Utils: parseNetworkName(originalNetworkNameToUse)
    Utils-->>Select: originalNetworkNameParsed
    
    Select->>Utils: filterNADs(nads, vm, isEditing, ...)
    Utils->>Utils: Check usage, apply edit-aware filtering
    Utils-->>Select: filteredNADs
    
    Select->>Utils: buildNetworkOptions(params)
    Utils->>Utils: createNadOption(), createPodNetworkOption(), etc.
    Utils-->>Select: networkOptions[]
    
    Select->>Utils: getInterfaceTypeFromValue(selectedValue, options)
    Utils-->>Select: interfaceType
    
    Note over Modal,Utils: On submit with updated interface
    Modal->>VM: Apply patch with cleaned interface properties
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

  • Component signature changes: NetworkInterfaceNetworkSelect props refactored with API impact to callers; verify all consumers updated.
  • VM-derived state: Logic shift from explicit editInitValueNetworkName to derived originalNetworkNameToUse; confirm correctness of getOriginalNetworkName implementation.
  • New utility extraction: 10+ new exported functions with interconnected dependencies; validate parsing logic, filter conditions, and option-building correctness.
  • Network patching logic: Test-replace patch construction and type-cleaning behavior; verify handling of pod/multus type switching and edge cases.
  • Cross-file integration: Changes span modal → select → utilities → patching; trace end-to-end flow for consistency.

Poem

🐰 Network interfaces now flow with grace,
Utilities extracted to their rightful place,
VM state guides the way through the modal's embrace,
Type-switching patches handle every case,
Refactored with care, a cleaner codebase! 🌐

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main objective of the PR: fixing network interface selection issues, which aligns with the core changes across multiple components.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description check ✅ Passed The PR description includes a brief summary of changes, references a Jira ticket, and provides before/after demo videos as specified in the template.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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

@openshift-ci-robot
Copy link
Collaborator

openshift-ci-robot commented Nov 11, 2025

@galkremer1: This pull request references CNV-71166 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.21.0" version, but no target version was set.

In response to this:

📝 Description

  • Display the correct network name when editing a current network interface
  • Enable the user to switch from pod networking to an NAD network and vice versa
  • Code refactoring for the network interface component

Jira Ticket: CNV-71166

🎥 Demo

Before:

Before.mov

After:

After.mov

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

@openshift-ci-robot
Copy link
Collaborator

openshift-ci-robot commented Nov 11, 2025

@galkremer1: This pull request references CNV-71166 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.21.0" version, but no target version was set.

In response to this:

📝 Description

  • Display the correct network name when editing a current network interface
  • Enable the user to switch from pod networking to an NAD network and vice versa
  • Code refactoring for the network interface component

Jira Ticket: CNV-71166

🎥 Demo

Before:

Before.mov

After:

After.mov

Summary by CodeRabbit

  • Bug Fixes

  • Improved network interface type switching to properly manage configuration properties when transitioning between different network types.

  • Refactor

  • Enhanced network interface components and selection logic for improved code organization and maintainability.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a82f44d and fcf6e90.

📒 Files selected for processing (4)
  • src/utils/components/NetworkInterfaceModal/NetworkInterfaceModal.tsx (4 hunks)
  • src/utils/components/NetworkInterfaceModal/components/NetworkInterfaceNetworkSelect/NetworkInterfaceNetworkSelect.tsx (6 hunks)
  • src/utils/components/NetworkInterfaceModal/components/NetworkInterfaceNetworkSelect/utils.tsx (3 hunks)
  • src/utils/resources/vm/utils/network/patch.ts (3 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/utils/resources/vm/utils/network/patch.ts (1)
src/utils/resources/vm/utils/constants.ts (3)
  • BRIDGE (37-37)
  • MASQUERADE (38-38)
  • SRIOV (39-39)
src/utils/components/NetworkInterfaceModal/components/NetworkInterfaceNetworkSelect/NetworkInterfaceNetworkSelect.tsx (2)
src/utils/components/NetworkInterfaceModal/components/NetworkInterfaceNetworkSelect/utils.tsx (5)
  • getOriginalNetworkName (58-62)
  • parseNetworkName (34-50)
  • filterNADs (149-174)
  • buildNetworkOptions (278-311)
  • getInterfaceTypeFromValue (320-328)
src/utils/components/NetworkInterfaceModal/utils/helpers.tsx (2)
  • podNetworkExists (34-35)
  • isPodNetworkName (37-37)
src/utils/components/NetworkInterfaceModal/components/NetworkInterfaceNetworkSelect/utils.tsx (5)
src/utils/resources/vm/utils/selectors.ts (1)
  • getNetworks (33-33)
src/utils/components/NetworkInterfaceModal/utils/helpers.tsx (3)
  • getNetworkName (39-44)
  • getNadType (103-112)
  • isPodNetworkName (37-37)
src/utils/components/NetworkInterfaceModal/components/hooks/types.ts (1)
  • NetworkAttachmentDefinition (13-15)
src/utils/components/NetworkInterfaceModal/components/NetworkInterfaceNetworkSelect/NetworkInterfaceNetworkSelect.tsx (1)
  • NetworkSelectTypeaheadOptionProps (48-50)
src/utils/resources/vm/utils/constants.ts (1)
  • POD_NETWORK (41-41)
src/utils/components/NetworkInterfaceModal/NetworkInterfaceModal.tsx (1)
src/views/virtualmachines/details/tabs/configuration/network/utils/utils.ts (2)
  • getConfigInterfaceStateFromVM (96-104)
  • isLinkStateEditable (145-146)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: build
  • GitHub Check: unit-test
  • GitHub Check: i18n

@openshift-ci openshift-ci bot added the lgtm Passed code review, ready for merge label Nov 12, 2025
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Nov 12, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: galkremer1, upalatucci

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [galkremer1,upalatucci]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@rszwajko
Copy link
Member

rszwajko commented Nov 12, 2025

/hold
@galkremer1
My understanding was that Pod Network should not be edited. Can we check how it worked in the past?
EDIT: in case of autoattachPodInterface we disabled editing but it seems that at least in 4.19 editing pod network was allowed.

Copy link
Member

@rszwajko rszwajko left a comment

Choose a reason for hiding this comment

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

Converting primary network to secondary network looks risky to me. I'd rather block or limit the edit capability. Technically the primary network might be defined in various ways and is much different from secondary network.

}

if (isCurrentPod && isNextMultus) {
// Switching from pod to multus - remove pod property
Copy link
Member

Choose a reason for hiding this comment

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

The code here makes too many assumptions about the pod network type.
The cases that I'm concerned:

  1. auto-attached pod network (autoattachPodInterface) - here we have no network definition
  2. multus as primary network

It would be also nice to check UDN based primary network.

@openshift-ci openshift-ci bot removed the lgtm Passed code review, ready for merge label Nov 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved This issue is something we want to fix do-not-merge/hold jira/valid-reference

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants