Skip to content

Conversation

@aviavissar
Copy link
Contributor

@aviavissar aviavissar commented Nov 13, 2025

jira :

https://issues.redhat.com/browse/CNV-66290?filter=12475298

📝 Description

Edit role should be able to add a new interface on the vm in the project

before:

image

after:

image

Summary by CodeRabbit

  • Refactor
    • Simplified network interface selection component to improve performance.
    • Streamlined the add network interface button behavior for improved usability.

✏️ Tip: You can customize this high-level summary in your review settings.

@openshift-ci-robot
Copy link
Collaborator

openshift-ci-robot commented Nov 13, 2025

@aviavissar: This pull request references CNV-66290 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.

Details

In response to this:

jira :

https://issues.redhat.com/browse/CNV-66290?filter=12475298

📝 Description

Edit role should be able to add a new interface on the vm in the project

before:

image

after:

image

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
Copy link
Contributor

openshift-ci bot commented Nov 13, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: aviavissar
Once this PR has been reviewed and has the lgtm label, please assign galkremer1 for approval. For more information see the Code Review Process.

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

Details Needs approval from an approver in each of these files:

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

@coderabbitai
Copy link

coderabbitai bot commented Nov 13, 2025

Important

Review skipped

More than 25% of the files skipped due to max files limit. The review is being skipped to prevent a low-quality review.

143 files out of 250 files are above the max files limit of 100. Please upgrade to Pro plan to get higher limits.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

Two UI component files were simplified: NetworkInterfaceNetworkSelect now renders SelectTypeahead unconditionally, removing the loading conditional gate. AddNetworkInterfaceButton eliminated permission gating logic, making the button always actionable without permission pre-checks.

Changes

Cohort / File(s) Summary
UI Component Simplifications
src/utils/components/NetworkInterfaceModal/components/NetworkInterfaceNetworkSelect/NetworkInterfaceNetworkSelect.tsx, src/views/virtualmachines/details/tabs/configuration/network/components/AddNetworkInterfaceButton.tsx
Removed conditional rendering gates and permission validation logic; SelectTypeahead now renders unconditionally; button behavior simplified with permission checks removed

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

  • Verify that removing the loading conditional in NetworkInterfaceNetworkSelect does not introduce race conditions or breaks expected behavior
  • Confirm that removing permission gating from AddNetworkInterfaceButton aligns with intended design and does not introduce unintended access patterns

Suggested labels

lgtm, approved

Suggested reviewers

  • Pedro-S-Abreu
  • rszwajko
  • avivtur

Poem

🐰 Hop-hoppity, the code takes flight,
Conditionals vanish into the night,
Simpler paths for the UI to roam,
Cleaner logic finds its home!

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly references the Jira ticket (CNV-66290) and describes the main change: enabling the edit cluster role to add network interfaces to VMs.
Description check ✅ Passed The description includes the required Jira link, a brief explanation of the change, and before/after screenshots demonstrating the behavior change. However, it omits the structured template sections (📝 Description and 🎥 Demo headers).
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@openshift-ci-robot
Copy link
Collaborator

openshift-ci-robot commented Nov 13, 2025

@aviavissar: This pull request references CNV-66290 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.

Details

In response to this:

jira :

https://issues.redhat.com/browse/CNV-66290?filter=12475298

📝 Description

Edit role should be able to add a new interface on the vm in the project

before:

image

after:

image

Summary by CodeRabbit

  • Bug Fixes

  • Replaced loading indicator with informative error message when network definitions are unavailable, directing users to contact system administrators for assistance.

  • Improvements

  • Streamlined network interface addition workflow by removing access restrictions, enhancing usability of the configuration process.

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: 2

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e899164 and e137bfd.

📒 Files selected for processing (3)
  • src/utils/components/NetworkInterfaceModal/components/NetworkInterfaceNetworkSelect/NetworkInterfaceNetworkSelect.tsx (1 hunks)
  • src/views/virtualmachines/details/tabs/configuration/network/components/AddNetworkInterfaceButton.tsx (1 hunks)
  • users.htpasswd (1 hunks)
⏰ 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: unit-test
  • GitHub Check: build
  • GitHub Check: i18n
🔇 Additional comments (1)
src/views/virtualmachines/details/tabs/configuration/network/components/AddNetworkInterfaceButton.tsx (1)

36-52: Verify permission handling in the submission flow.

The permission gating has been completely removed, allowing all users to open the modal regardless of their capabilities. While this aligns with the PR's goal of enabling the Edit role, ensure that:

  1. Permission validation occurs during the actual network interface creation (in the modal's submit handler or API layer)
  2. Users without permissions receive a clear, actionable error message if they attempt to submit
  3. The Edit role is correctly configured with the necessary permissions in the backend

Run the following script to verify where permission validation occurs in the submission flow:

@aviavissar aviavissar force-pushed the cluster-role-edit-cannot-add-network-interface-for-the-vm branch from af6b050 to d94d8f6 Compare November 14, 2025 15:25
@openshift-ci-robot
Copy link
Collaborator

openshift-ci-robot commented Nov 14, 2025

@aviavissar: This pull request references CNV-66290 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.

Details

In response to this:

jira :

https://issues.redhat.com/browse/CNV-66290?filter=12475298

📝 Description

Edit role should be able to add a new interface on the vm in the project

before:

image

after:

image

Summary by CodeRabbit

  • Improvements
  • Enhanced error messaging: Users now see a clear error message when network attachments are unavailable, replacing the loading indicator.
  • Streamlined network interface workflow: Removed permission-based restrictions from the network interface addition process for unrestricted access.

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.

@aviavissar aviavissar force-pushed the cluster-role-edit-cannot-add-network-interface-for-the-vm branch from d94d8f6 to 04f38b5 Compare November 16, 2025 12:30
@openshift-ci-robot
Copy link
Collaborator

openshift-ci-robot commented Nov 16, 2025

@aviavissar: This pull request references CNV-66290 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.

Details

In response to this:

jira :

https://issues.redhat.com/browse/CNV-66290?filter=12475298

📝 Description

Edit role should be able to add a new interface on the vm in the project

before:

image

after:

image

Summary by CodeRabbit

  • Improvements
  • Clearer error messaging when network attachments are unavailable — replaces a loading indicator with a persistent error helper so users immediately see the issue.
  • Simplified network interface workflow — the Add Network Interface button is no longer permission-gated and opens the add-network modal directly for all users.

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.

@aviavissar aviavissar force-pushed the cluster-role-edit-cannot-add-network-interface-for-the-vm branch from 04f38b5 to dddf961 Compare November 17, 2025 14:58
@openshift-ci-robot
Copy link
Collaborator

openshift-ci-robot commented Nov 17, 2025

@aviavissar: This pull request references CNV-66290 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.

Details

In response to this:

jira :

https://issues.redhat.com/browse/CNV-66290?filter=12475298

📝 Description

Edit role should be able to add a new interface on the vm in the project

before:

image

after:

image

Summary by CodeRabbit

  • Improvements
  • Error messaging for network attachments is now shown inline and persistently, replacing a transient loading indicator so issues are immediately visible.
  • The Add Network Interface button is simplified and always opens the add-network modal without permission-based disabling.

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.

@aviavissar aviavissar force-pushed the cluster-role-edit-cannot-add-network-interface-for-the-vm branch from 39f8417 to 753f57e Compare November 17, 2025 15:05
@openshift-ci-robot
Copy link
Collaborator

openshift-ci-robot commented Nov 17, 2025

@aviavissar: This pull request references CNV-66290 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.

Details

In response to this:

jira :

https://issues.redhat.com/browse/CNV-66290?filter=12475298

📝 Description

Edit role should be able to add a new interface on the vm in the project

before:

image

after:

image

Summary by CodeRabbit

Release Notes

  • UI/UX Improvements

  • Streamlined network interface selection with improved error message positioning and simplified loading state handling.

  • Feature Changes

  • "Add Network Interface" button is now always interactive and accessible without permission-based restrictions.

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 dddf961 and 753f57e.

📒 Files selected for processing (2)
  • src/utils/components/NetworkInterfaceModal/components/NetworkInterfaceNetworkSelect/NetworkInterfaceNetworkSelect.tsx (1 hunks)
  • src/views/virtualmachines/details/tabs/configuration/network/components/AddNetworkInterfaceButton.tsx (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/utils/components/NetworkInterfaceModal/components/NetworkInterfaceNetworkSelect/NetworkInterfaceNetworkSelect.tsx
⏰ 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: unit-test
  • GitHub Check: i18n
  • GitHub Check: build

@aviavissar aviavissar force-pushed the cluster-role-edit-cannot-add-network-interface-for-the-vm branch from 753f57e to 9179660 Compare December 4, 2025 14:23
@openshift-ci-robot
Copy link
Collaborator

openshift-ci-robot commented Dec 4, 2025

@aviavissar: This pull request references CNV-66290 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.

Details

In response to this:

jira :

https://issues.redhat.com/browse/CNV-66290?filter=12475298

📝 Description

Edit role should be able to add a new interface on the vm in the project

before:

image

after:

image

Summary by CodeRabbit

  • UI/UX Improvements

  • Network selection now renders immediately with a clearer error helper when no networks are available, and unnecessary loading gating removed for faster access.

  • Feature Changes

  • "Add Network Interface" button is always actionable and opens the add-network modal without permission-based disabling or extra checks.

✏️ Tip: You can customize this high-level summary in your review settings.

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: 0

🧹 Nitpick comments (1)
src/utils/components/NetworkInterfaceModal/utils/constants.ts (1)

1-10: Align constant naming with guidelines and consider translation timing

showErrorText lives in a utils/constants.ts file, but its name is camelCase and not in the uppercase‑with‑underscores style we use for utility constants. Consider renaming to something like NETWORK_ATTACHMENT_DEFINITIONS_ERROR_TEXT (or similar) to match the guidelines and improve clarity.

Also, note that calling t(...) at module scope will fix the string at import time. If the console supports changing locale at runtime, this constant will not react to language changes. In that case, it may be safer to either:

  • export a function that calls t(...) when needed, or
  • keep only a key/descriptor here and perform the actual t(...) call inside the component using the translation hook.
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 753f57e and 9179660.

📒 Files selected for processing (3)
  • src/utils/components/NetworkInterfaceModal/components/NetworkInterfaceNetworkSelect/NetworkInterfaceNetworkSelect.tsx (2 hunks)
  • src/utils/components/NetworkInterfaceModal/utils/constants.ts (1 hunks)
  • src/views/virtualmachines/details/tabs/configuration/network/components/AddNetworkInterfaceButton.tsx (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/views/virtualmachines/details/tabs/configuration/network/components/AddNetworkInterfaceButton.tsx
  • src/utils/components/NetworkInterfaceModal/components/NetworkInterfaceNetworkSelect/NetworkInterfaceNetworkSelect.tsx
🧰 Additional context used
📓 Path-based instructions (3)
**/*.ts

📄 CodeRabbit inference engine (CODING_STANDARDS.md)

**/*.ts: Use .ts file extension for non-component files containing logic or utilities.
Define constants in utility files with uppercase and underscore-separated naming (e.g., API_URL).
If a TypeScript type is exported, add it to a utility file.

Files:

  • src/utils/components/NetworkInterfaceModal/utils/constants.ts
**/*.{tsx,ts}

📄 CodeRabbit inference engine (CODING_STANDARDS.md)

**/*.{tsx,ts}: Extract as much logic as possible from components into custom hooks or utility files to improve testability. Avoid bloated components.
Hooks should contain only logic and side effects, not return JSX. Keep JSX in components while using hooks for extracting reusable or unit-testable logic.

Files:

  • src/utils/components/NetworkInterfaceModal/utils/constants.ts
**/*.{ts,tsx}

📄 CodeRabbit inference engine (CODING_STANDARDS.md)

**/*.{ts,tsx}: Use descriptive names for variables, functions, and components. Avoid abbreviations unless widely recognized.
Prefer using type instead of interface for defining the shapes of objects or functions in TypeScript.
Avoid using any type in TypeScript as it compromises type safety. Use unknown instead and narrow the type as needed.
Always explicitly define return types for functions in TypeScript rather than relying on type inference.

Files:

  • src/utils/components/NetworkInterfaceModal/utils/constants.ts
🧠 Learnings (1)
📚 Learning: 2025-11-20T20:28:17.043Z
Learnt from: galkremer1
Repo: kubevirt-ui/kubevirt-plugin PR: 3118
File: src/views/checkups/self-validation/utils/constants.ts:62-68
Timestamp: 2025-11-20T20:28:17.043Z
Learning: For the kubevirt-ui/kubevirt-plugin repository: The test suite labels in TEST_SUITE_OPTIONS (Compute, Network, Storage, SSP, Tier2) in src/views/checkups/self-validation/utils/constants.ts should not be internationalized, even though they are displayed in the UI.

Applied to files:

  • src/utils/components/NetworkInterfaceModal/utils/constants.ts
⏰ 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

@aviavissar aviavissar force-pushed the cluster-role-edit-cannot-add-network-interface-for-the-vm branch from 9179660 to ea85526 Compare December 15, 2025 11:06
@openshift-ci-robot
Copy link
Collaborator

openshift-ci-robot commented Dec 15, 2025

@aviavissar: This pull request references CNV-66290 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.22.0" version, but no target version was set.

Details

In response to this:

jira :

https://issues.redhat.com/browse/CNV-66290?filter=12475298

📝 Description

Edit role should be able to add a new interface on the vm in the project

before:

image

after:

image

Summary by CodeRabbit

  • Refactor
  • Simplified network interface selection component to improve performance.
  • Streamlined the add network interface button behavior for improved usability.

✏️ Tip: You can customize this high-level summary in your review settings.

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.

rszwajko and others added 18 commits December 16, 2025 16:50
Use "git add locales" if there are changes in the English translations.
It's no longer required to add them manually after the commit.

Run i18n in parallel with eslint(TS code) and prettier(json) - the file
patterns is overlapping but the translation files are not processed
by eslint/prettier - we can write them without conflict. I18n script
requires read access to the code/json files which should create no
conflicts.
This change also disables i18n when there are no staged files i.e.
during amending.

Signed-off-by: Radoslaw Szwajkowski <[email protected]>
@aviavissar aviavissar force-pushed the cluster-role-edit-cannot-add-network-interface-for-the-vm branch from ea85526 to 4af1dd9 Compare December 16, 2025 15:01
@openshift-merge-robot
Copy link
Collaborator

PR needs rebase.

Details

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 kubernetes-sigs/prow repository.

@aviavissar
Copy link
Contributor Author

/retest

@aviavissar aviavissar force-pushed the cluster-role-edit-cannot-add-network-interface-for-the-vm branch from 4af1dd9 to 470745d Compare December 16, 2025 15:15
@adamviktora adamviktora changed the base branch from main to release-4.18 December 17, 2025 15:23
@openshift-ci-robot
Copy link
Collaborator

openshift-ci-robot commented Dec 17, 2025

@aviavissar: This pull request references CNV-66290 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.18.z" version, but no target version was set.

Details

In response to this:

jira :

https://issues.redhat.com/browse/CNV-66290?filter=12475298

📝 Description

Edit role should be able to add a new interface on the vm in the project

before:

image

after:

image

Summary by CodeRabbit

  • Refactor
  • Simplified network interface selection component to improve performance.
  • Streamlined the add network interface button behavior for improved usability.

✏️ Tip: You can customize this high-level summary in your review settings.

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.

@adamviktora adamviktora changed the base branch from release-4.18 to main December 17, 2025 15:23
@openshift-ci-robot
Copy link
Collaborator

openshift-ci-robot commented Dec 17, 2025

@aviavissar: This pull request references CNV-66290 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.22.0" version, but no target version was set.

Details

In response to this:

jira :

https://issues.redhat.com/browse/CNV-66290?filter=12475298

📝 Description

Edit role should be able to add a new interface on the vm in the project

before:

image

after:

image

Summary by CodeRabbit

  • Refactor
  • Simplified network interface selection component to improve performance.
  • Streamlined the add network interface button behavior for improved usability.

✏️ Tip: You can customize this high-level summary in your review settings.

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
Copy link
Contributor

openshift-ci bot commented Dec 17, 2025

@aviavissar: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/kubevirt-e2e-aws 470745d link true /test kubevirt-e2e-aws
ci/prow/images 470745d link true /test images

Full PR test history. Your PR dashboard.

Details

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 kubernetes-sigs/prow repository. I understand the commands that are listed here.

@aviavissar aviavissar closed this Dec 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants