CNV-81600: Support new passtBinding network interface method#3585
CNV-81600: Support new passtBinding network interface method#3585upalatucci wants to merge 1 commit intokubevirt-ui:mainfrom
Conversation
|
@upalatucci: This pull request references CNV-81600 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 epic to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds cluster-aware PASST feature-flag support (including isLegacyPasst) across network-interface creation and modals, relocates and extends the usePasstFeatureFlag hook, reorganizes PASST constants, and updates network patching/selectors to handle passtBinding alongside existing bindings. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment Tip CodeRabbit can use TruffleHog to scan for secrets in your code with verification capabilities.Add a TruffleHog config file (e.g. trufflehog-config.yml, trufflehog.yml) to your project to customize detectors and scanning behavior. The tool runs only when a config file is present. |
|
@upalatucci: This pull request references CNV-81600 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 epic to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/utils/resources/vm/utils/network/patch.ts (1)
105-106: Avoidanytype; consider a shared extended type.The
anycasts work around the fact thatV1Interfacedoesn't includebindingorpasstBindingproperties. Since this pattern appears in multiple files (here and inselectors.ts), consider defining a shared extended type:♻️ Suggested approach
// In a shared types file, e.g., src/utils/resources/vm/utils/network/types.ts export type V1InterfaceExtended = V1Interface & { binding?: { name: string }; passtBinding?: Record<string, never>; };Then use this type in both
patch.tsandselectors.tsto avoid scatteredanycasts.As per coding guidelines: "Never use
anytype; useunknownfor truly unknown data with proper type narrowing via type guards instead."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/resources/vm/utils/network/patch.ts` around lines 105 - 106, The current checks in patch.ts use unsafe any casts on nextValue and replaceValue to access binding/passtBinding; create a shared extended type (e.g., V1InterfaceExtended = V1Interface & { binding?: { name: string }; passtBinding?: Record<string, never> }) in a common types file (e.g., src/utils/resources/vm/utils/network/types.ts) and import it into patch.ts and selectors.ts, then replace the (any) casts by typing nextValue and replaceValue as V1InterfaceExtended (or narrow via a type guard) and use those properties without any casts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/utils/components/NetworkInterfaceModal/NetworkInterfaceModal.tsx`:
- Line 78: The modal currently reads isLegacyPasst from
usePasstBindingMethod(cluster) which defaults to false while the HyperConverged
watch is resolving, allowing premature submission; update the component to treat
the unresolved state as loading and disable/gate the submit flow until the HC
config has loaded by either (A) changing usePasstBindingMethod to return an
explicit loading flag (e.g., { isLegacyPasst, isLoading }) and use isLoading to
disable the submit button and skip forwarding the value, or (B) in
NetworkInterfaceModal detect an undefined/placeholder isLegacyPasst value and
block submission (disable submit, show spinner/message) until a concrete
true/false is returned; apply this same loading-guard logic to the other usages
noted (around the isLegacyPasst references at lines ~98-100, 103-110, 129-133)
so no passtBinding submission is allowed while HC state is unresolved.
In `@src/utils/hooks/usePasstBindingMethod.ts`:
- Line 5: Update the import statement in usePasstBindingMethod.ts to remove the
illegal .ts extension: replace the import path
'./useKubevirtHyperconvergeConfiguration.ts' with
'./useKubevirtHyperconvergeConfiguration' so it matches the project's TypeScript
module resolution (affects the import at the top of the file where
useKubevirtHyperconvergeConfiguration is imported).
In `@src/utils/resources/vm/utils/network/selectors.ts`:
- Line 21: Replace the unsafe (iface as any)?.passtBinding check with a proper
type guard: add a predicate function (e.g., function isPasstBinding(obj:
unknown): obj is { passtBinding: boolean }) that checks obj is an object and
'passtBinding' in obj, then update the selector to use isPasstBinding(iface) &&
iface.passtBinding and return PASST_BINDING_NAME; this removes the any cast and
provides correct type narrowing for iface.
---
Nitpick comments:
In `@src/utils/resources/vm/utils/network/patch.ts`:
- Around line 105-106: The current checks in patch.ts use unsafe any casts on
nextValue and replaceValue to access binding/passtBinding; create a shared
extended type (e.g., V1InterfaceExtended = V1Interface & { binding?: { name:
string }; passtBinding?: Record<string, never> }) in a common types file (e.g.,
src/utils/resources/vm/utils/network/types.ts) and import it into patch.ts and
selectors.ts, then replace the (any) casts by typing nextValue and replaceValue
as V1InterfaceExtended (or narrow via a type guard) and use those properties
without any casts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7d247af8-e93c-4a05-abdb-9baf0e4b8515
📒 Files selected for processing (12)
src/utils/components/NetworkInterfaceModal/NetworkInterfaceModal.tsxsrc/utils/components/NetworkInterfaceModal/utils/helpers.tsxsrc/utils/hooks/usePasstBindingMethod.tssrc/utils/resources/vm/utils/constants.tssrc/utils/resources/vm/utils/network/patch.tssrc/utils/resources/vm/utils/network/selectors.tssrc/views/catalog/wizard/tabs/network/components/modal/WizardEditNetworkInterfaceModal.tsxsrc/views/catalog/wizard/tabs/network/components/modal/WizardNetworkInterfaceModal.tsxsrc/views/templates/details/tabs/network/components/modal/TemplatesEditNetworkInterfaceModal.tsxsrc/views/templates/details/tabs/network/components/modal/TemplatesNetworkInterfaceModal.tsxsrc/views/virtualmachines/details/tabs/configuration/network/components/modal/VirtualMachinesEditNetworkInterfaceModal.tsxsrc/views/virtualmachines/details/tabs/configuration/network/components/modal/VirtualMachinesNetworkInterfaceModal.tsx
| const { iface = null, network = null } = nicPresentation; | ||
|
|
||
| const { featureEnabled: passtEnabled } = usePasstFeatureFlag(); | ||
| const { isLegacyPasst } = usePasstBindingMethod(cluster); |
There was a problem hiding this comment.
Gate passt submission until the HyperConverged config has loaded.
usePasstBindingMethod returns false until the HC watch resolves, and this modal forwards that value immediately. That means users can submit native passtBinding on clusters that still require the legacy binding plugin if they open the modal before the config loads.
Suggested fix
- const { isLegacyPasst } = usePasstBindingMethod(cluster);
+ const { isLegacyPasst, loaded: isPasstBindingMethodLoaded } = usePasstBindingMethod(cluster);
@@
- const isValid = nicName && networkName && !networkSelectError && !macError;
+ const isValid =
+ isPasstBindingMethodLoaded &&
+ Boolean(nicName) &&
+ Boolean(networkName) &&
+ !networkSelectError &&
+ !macError;As per coding guidelines "Handle loading, error, and empty states in all data-fetching components".
Also applies to: 98-100, 103-110, 129-133
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/components/NetworkInterfaceModal/NetworkInterfaceModal.tsx` at line
78, The modal currently reads isLegacyPasst from usePasstBindingMethod(cluster)
which defaults to false while the HyperConverged watch is resolving, allowing
premature submission; update the component to treat the unresolved state as
loading and disable/gate the submit flow until the HC config has loaded by
either (A) changing usePasstBindingMethod to return an explicit loading flag
(e.g., { isLegacyPasst, isLoading }) and use isLoading to disable the submit
button and skip forwarding the value, or (B) in NetworkInterfaceModal detect an
undefined/placeholder isLegacyPasst value and block submission (disable submit,
show spinner/message) until a concrete true/false is returned; apply this same
loading-guard logic to the other usages noted (around the isLegacyPasst
references at lines ~98-100, 103-110, 129-133) so no passtBinding submission is
allowed while HC state is unresolved.
|
|
||
| import { PASS_IP_STACK_MIGRATION_GATE } from '@kubevirt-utils/resources/vm/utils/constants'; | ||
|
|
||
| import useKubevirtHyperconvergeConfiguration from './useKubevirtHyperconvergeConfiguration.ts'; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== TypeScript configs =="
fd '^tsconfig.*\.json$' . -x sh -c 'echo "--- {} ---"; sed -n "1,220p" "{}"'
echo
echo "== Explicit .ts relative imports in source files =="
rg -n --glob '*.ts' --glob '*.tsx' "from ['\"][./].*\\.ts['\"]|import\\(['\"][./].*\\.ts['\"]\\)" .Repository: kubevirt-ui/kubevirt-plugin
Length of output: 1747
Remove the .ts extension from the import path.
The TypeScript configuration does not enable allowImportingTsExtensions, making the explicit .ts extension invalid. Change './useKubevirtHyperconvergeConfiguration.ts' to './useKubevirtHyperconvergeConfiguration'.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/hooks/usePasstBindingMethod.ts` at line 5, Update the import
statement in usePasstBindingMethod.ts to remove the illegal .ts extension:
replace the import path './useKubevirtHyperconvergeConfiguration.ts' with
'./useKubevirtHyperconvergeConfiguration' so it matches the project's TypeScript
module resolution (affects the import at the top of the file where
useKubevirtHyperconvergeConfiguration is imported).
| export const getNetworkInterfaceType = (iface: V1Interface): string => { | ||
| if (iface?.binding?.name === UDN_BINDING_NAME) return UDN_BINDING_NAME; | ||
| if (iface?.binding?.name === PASST_BINDING_NAME) return PASST_BINDING_NAME; | ||
| if ((iface as any)?.passtBinding) return PASST_BINDING_NAME; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Avoid any type; use a type guard or specific type assertion.
Per coding guidelines, any should not be used. Consider defining a type guard or using a more specific type assertion:
♻️ Suggested refactor using type predicate
+type InterfaceWithPasstBinding = V1Interface & { passtBinding?: Record<string, never> };
+
+const hasPasstBinding = (iface: V1Interface): iface is InterfaceWithPasstBinding =>
+ 'passtBinding' in iface;
+
export const getNetworkInterfaceType = (iface: V1Interface): string => {
if (iface?.binding?.name === UDN_BINDING_NAME) return UDN_BINDING_NAME;
if (iface?.binding?.name === PASST_BINDING_NAME) return PASST_BINDING_NAME;
- if ((iface as any)?.passtBinding) return PASST_BINDING_NAME;
+ if (hasPasstBinding(iface)) return PASST_BINDING_NAME;As per coding guidelines: "Never use any type; use unknown for truly unknown data with proper type narrowing via type guards instead."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ((iface as any)?.passtBinding) return PASST_BINDING_NAME; | |
| type InterfaceWithPasstBinding = V1Interface & { passtBinding?: Record<string, never> }; | |
| const hasPasstBinding = (iface: V1Interface): iface is InterfaceWithPasstBinding => | |
| 'passtBinding' in iface; | |
| export const getNetworkInterfaceType = (iface: V1Interface): string => { | |
| if (iface?.binding?.name === UDN_BINDING_NAME) return UDN_BINDING_NAME; | |
| if (iface?.binding?.name === PASST_BINDING_NAME) return PASST_BINDING_NAME; | |
| if (hasPasstBinding(iface)) return PASST_BINDING_NAME; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/resources/vm/utils/network/selectors.ts` at line 21, Replace the
unsafe (iface as any)?.passtBinding check with a proper type guard: add a
predicate function (e.g., function isPasstBinding(obj: unknown): obj is {
passtBinding: boolean }) that checks obj is an object and 'passtBinding' in obj,
then update the selector to use isPasstBinding(iface) && iface.passtBinding and
return PASST_BINDING_NAME; this removes the any cast and provides correct type
narrowing for iface.
upalatucci
force-pushed
the
CNV-81600-passt-binding-method
branch
from
b89dcfe to
32c19b7
Compare
|
@upalatucci: This pull request references CNV-81600 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 epic to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
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. |
upalatucci
force-pushed
the
CNV-81600-passt-binding-method
branch
from
32c19b7 to
d49a7d6
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/utils/components/AddBootableVolumeModal/utils/constants.ts (1)
4-10:⚠️ Potential issue | 🟠 MajorConsolidate enum imports consistently across the codebase.
The change to import
V1beta1StorageSpecAccessModesEnumandV1beta1StorageSpecVolumeModeEnumfromcontainerized-data-importercreates an inconsistency: this is the only file in the codebase using this import path. Eight other files import the same enums from@kubevirt-ui-ext/kubevirt-api/kubevirt. Either revert this file to match the codebase convention (import fromkubevirt), or update all other files to use the new import path fromcontainerized-data-importer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/components/AddBootableVolumeModal/utils/constants.ts` around lines 4 - 10, This file imports V1beta1StorageSpecAccessModesEnum and V1beta1StorageSpecVolumeModeEnum from '@kubevirt-ui-ext/kubevirt-api/containerized-data-importer', which is inconsistent with the rest of the codebase; change the import source for these two enums to '@kubevirt-ui-ext/kubevirt-api/kubevirt' so they match the other files (leave other imports like V1beta1DataImportCron, V1beta1DataSource, V1beta1DataVolume as-is if required). Update the import statement that includes V1beta1StorageSpecAccessModesEnum and V1beta1StorageSpecVolumeModeEnum to reference the kubevirt module instead.
🧹 Nitpick comments (2)
src/views/templates/details/tabs/network/components/modal/TemplatesEditNetworkInterfaceModal.tsx (1)
35-79: Consider wrappingonSubmitwithuseCallbackfor consistency.Unlike
VirtualMachinesEditNetworkInterfaceModal, this component doesn't memoizeonSubmit. While not introduced by this PR, wrapping it withuseCallbackwould prevent unnecessary re-creations on each render.♻️ Suggested improvement
+import React, { FC, useCallback } from 'react'; -import React, { FC } from 'react'; ... - const onSubmit = + const onSubmit = useCallback( ({ interfaceLinkState, ... }) => () => { ... - }; + }, + [template, nicPresentation], + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/templates/details/tabs/network/components/modal/TemplatesEditNetworkInterfaceModal.tsx` around lines 35 - 79, Wrap the onSubmit handler in a useCallback to memoize it like VirtualMachinesEditNetworkInterfaceModal; replace the standalone const onSubmit = (...) => () => { ... } with a useCallback that returns the same inner function and include all external dependencies (template, nicPresentation?.network?.name, createNetwork, createInterface, produceTemplateNetwork, kubevirtK8sUpdate, getCluster, TemplateModel, getName, getNamespace) in the dependency array so the callback is recreated only when those change.src/views/virtualmachines/details/tabs/configuration/network/components/modal/VirtualMachinesEditNetworkInterfaceModal.tsx (1)
71-71: Remove unusednicPresentationfrom the dependency array.
nicPresentationis included in theuseCallbackdependency array but is not referenced inside the callback body. This causes unnecessary re-renders whennicPresentationchanges.♻️ Proposed fix
- [vm, nicPresentation], + [vm],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/virtualmachines/details/tabs/configuration/network/components/modal/VirtualMachinesEditNetworkInterfaceModal.tsx` at line 71, The useCallback in VirtualMachinesEditNetworkInterfaceModal references vm but not nicPresentation, so remove nicPresentation from the dependency array to avoid needless re-renders; update the dependency array from [vm, nicPresentation] to only include the actually used variables (e.g., [vm]) and run the linter to confirm no other dependencies are missing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/utils/components/AddBootableVolumeModal/utils/constants.ts`:
- Around line 4-10: This file imports V1beta1StorageSpecAccessModesEnum and
V1beta1StorageSpecVolumeModeEnum from
'@kubevirt-ui-ext/kubevirt-api/containerized-data-importer', which is
inconsistent with the rest of the codebase; change the import source for these
two enums to '@kubevirt-ui-ext/kubevirt-api/kubevirt' so they match the other
files (leave other imports like V1beta1DataImportCron, V1beta1DataSource,
V1beta1DataVolume as-is if required). Update the import statement that includes
V1beta1StorageSpecAccessModesEnum and V1beta1StorageSpecVolumeModeEnum to
reference the kubevirt module instead.
---
Nitpick comments:
In
`@src/views/templates/details/tabs/network/components/modal/TemplatesEditNetworkInterfaceModal.tsx`:
- Around line 35-79: Wrap the onSubmit handler in a useCallback to memoize it
like VirtualMachinesEditNetworkInterfaceModal; replace the standalone const
onSubmit = (...) => () => { ... } with a useCallback that returns the same inner
function and include all external dependencies (template,
nicPresentation?.network?.name, createNetwork, createInterface,
produceTemplateNetwork, kubevirtK8sUpdate, getCluster, TemplateModel, getName,
getNamespace) in the dependency array so the callback is recreated only when
those change.
In
`@src/views/virtualmachines/details/tabs/configuration/network/components/modal/VirtualMachinesEditNetworkInterfaceModal.tsx`:
- Line 71: The useCallback in VirtualMachinesEditNetworkInterfaceModal
references vm but not nicPresentation, so remove nicPresentation from the
dependency array to avoid needless re-renders; update the dependency array from
[vm, nicPresentation] to only include the actually used variables (e.g., [vm])
and run the linter to confirm no other dependencies are missing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 151ffc47-98c0-4d24-9fd8-6e0451b4fa1e
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (14)
package.jsonsrc/utils/components/AddBootableVolumeModal/utils/constants.tssrc/utils/components/NetworkInterfaceModal/NetworkInterfaceModal.tsxsrc/utils/components/NetworkInterfaceModal/utils/helpers.tsxsrc/utils/hooks/usePasstBindingMethod.tssrc/utils/resources/vm/utils/constants.tssrc/utils/resources/vm/utils/network/patch.tssrc/utils/resources/vm/utils/network/selectors.tssrc/views/catalog/wizard/tabs/network/components/modal/WizardEditNetworkInterfaceModal.tsxsrc/views/catalog/wizard/tabs/network/components/modal/WizardNetworkInterfaceModal.tsxsrc/views/templates/details/tabs/network/components/modal/TemplatesEditNetworkInterfaceModal.tsxsrc/views/templates/details/tabs/network/components/modal/TemplatesNetworkInterfaceModal.tsxsrc/views/virtualmachines/details/tabs/configuration/network/components/modal/VirtualMachinesEditNetworkInterfaceModal.tsxsrc/views/virtualmachines/details/tabs/configuration/network/components/modal/VirtualMachinesNetworkInterfaceModal.tsx
🚧 Files skipped from review as they are similar to previous changes (7)
- src/views/catalog/wizard/tabs/network/components/modal/WizardEditNetworkInterfaceModal.tsx
- src/utils/resources/vm/utils/network/selectors.ts
- src/utils/hooks/usePasstBindingMethod.ts
- src/utils/resources/vm/utils/network/patch.ts
- src/utils/components/NetworkInterfaceModal/utils/helpers.tsx
- src/utils/components/NetworkInterfaceModal/NetworkInterfaceModal.tsx
- src/views/catalog/wizard/tabs/network/components/modal/WizardNetworkInterfaceModal.tsx
upalatucci
changed the title
|
@upalatucci: This pull request references CNV-81600 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 epic to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
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. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/utils/components/NetworkInterfaceModal/utils/helpers.tsx (1)
75-80: Loosen the helper contract for native passt.
createInterfacenow ignoresinterfaceMACAddressandinterfaceModelfor the non-legacy passt path, butCreateInterfaceOptionsstill requires both. That makes the API misleading and forces callers to supply values that will never be serialized. A discriminated union, or at least optional fields for native passt, would make this harder to misuse.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/components/NetworkInterfaceModal/utils/helpers.tsx` around lines 75 - 80, The CreateInterfaceOptions type currently requires interfaceMACAddress and interfaceModel even though createInterface ignores them for non-legacy passt; change the helper contract so callers aren’t forced to provide unused values by turning CreateInterfaceOptions into a discriminated union (e.g., { isLegacyPasst: true; interfaceMACAddress: string; interfaceModel: string; ... } | { isLegacyPasst?: false; interfaceMACAddress?: never; interfaceModel?: never; ... }) or, at minimum, make interfaceMACAddress and interfaceModel optional when isLegacyPasst is false, and update createInterface’s parameter type to that new union/optional form so types accurately reflect the serialized behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/utils/components/NetworkInterfaceModal/utils/helpers.tsx`:
- Around line 75-80: The CreateInterfaceOptions type currently requires
interfaceMACAddress and interfaceModel even though createInterface ignores them
for non-legacy passt; change the helper contract so callers aren’t forced to
provide unused values by turning CreateInterfaceOptions into a discriminated
union (e.g., { isLegacyPasst: true; interfaceMACAddress: string; interfaceModel:
string; ... } | { isLegacyPasst?: false; interfaceMACAddress?: never;
interfaceModel?: never; ... }) or, at minimum, make interfaceMACAddress and
interfaceModel optional when isLegacyPasst is false, and update
createInterface’s parameter type to that new union/optional form so types
accurately reflect the serialized behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a3723949-cf14-4cfe-b936-10a763552354
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (14)
package.jsonsrc/utils/components/AddBootableVolumeModal/utils/constants.tssrc/utils/components/NetworkInterfaceModal/NetworkInterfaceModal.tsxsrc/utils/components/NetworkInterfaceModal/utils/helpers.tsxsrc/utils/hooks/usePasstBindingMethod.tssrc/utils/resources/vm/utils/constants.tssrc/utils/resources/vm/utils/network/patch.tssrc/utils/resources/vm/utils/network/selectors.tssrc/views/catalog/wizard/tabs/network/components/modal/WizardEditNetworkInterfaceModal.tsxsrc/views/catalog/wizard/tabs/network/components/modal/WizardNetworkInterfaceModal.tsxsrc/views/templates/details/tabs/network/components/modal/TemplatesEditNetworkInterfaceModal.tsxsrc/views/templates/details/tabs/network/components/modal/TemplatesNetworkInterfaceModal.tsxsrc/views/virtualmachines/details/tabs/configuration/network/components/modal/VirtualMachinesEditNetworkInterfaceModal.tsxsrc/views/virtualmachines/details/tabs/configuration/network/components/modal/VirtualMachinesNetworkInterfaceModal.tsx
🚧 Files skipped from review as they are similar to previous changes (10)
- src/utils/resources/vm/utils/network/patch.ts
- src/views/templates/details/tabs/network/components/modal/TemplatesNetworkInterfaceModal.tsx
- src/utils/resources/vm/utils/network/selectors.ts
- package.json
- src/utils/components/NetworkInterfaceModal/NetworkInterfaceModal.tsx
- src/views/virtualmachines/details/tabs/configuration/network/components/modal/VirtualMachinesEditNetworkInterfaceModal.tsx
- src/utils/resources/vm/utils/constants.ts
- src/views/virtualmachines/details/tabs/configuration/network/components/modal/VirtualMachinesNetworkInterfaceModal.tsx
- src/views/catalog/wizard/tabs/network/components/modal/WizardEditNetworkInterfaceModal.tsx
- src/utils/hooks/usePasstBindingMethod.ts
|
@upalatucci: This pull request references CNV-81600 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 epic to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
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. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/utils/components/SSHAccess/components/VirtctlDisabled.tsx (1)
24-24:⚠️ Potential issue | 🟡 MinorPass
clustertousePasstFeatureFlagfor multi-cluster consistency.The component receives a
clusterprop but doesn't pass it tousePasstFeatureFlag(). Since the hook now supports theclusterparameter for cluster-aware configuration detection, this could causepasstEnabledto reflect the wrong cluster's configuration in multi-cluster environments.Proposed fix
- const { featureEnabled: passtEnabled } = usePasstFeatureFlag(); + const { featureEnabled: passtEnabled } = usePasstFeatureFlag(cluster);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/components/SSHAccess/components/VirtctlDisabled.tsx` at line 24, The component VirtctlDisabled reads the cluster prop but calls usePasstFeatureFlag() without it, causing incorrect feature detection across clusters; update the hook invocation to pass the cluster prop (e.g., call usePasstFeatureFlag(cluster) so the existing destructuring const { featureEnabled: passtEnabled } = usePasstFeatureFlag(...) uses the correct cluster-aware value) and ensure any TypeScript types/signature for usePasstFeatureFlag accept the cluster argument if necessary.
🧹 Nitpick comments (1)
src/utils/hooks/usePasstFeatureFlag.ts (1)
15-15: Remove the.tsextension from the import path.Including the
.tsextension in import statements is non-standard and may cause issues with some bundlers or module resolution configurations.Proposed fix
-import useKubevirtHyperconvergeConfiguration from './useKubevirtHyperconvergeConfiguration.ts'; +import useKubevirtHyperconvergeConfiguration from './useKubevirtHyperconvergeConfiguration';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/hooks/usePasstFeatureFlag.ts` at line 15, The import in usePasstFeatureFlag currently includes a .ts extension; update the import statement that references './useKubevirtHyperconvergeConfiguration.ts' to remove the extension (i.e. import useKubevirtHyperconvergeConfiguration from './useKubevirtHyperconvergeConfiguration') so module resolution and bundlers handle the module correctly while keeping the imported symbol useKubevirtHyperconvergeConfiguration unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/utils/hooks/usePasstFeatureFlag.ts`:
- Around line 17-19: The hook usePasstFeatureFlag calls
useKubevirtHyperconvergeConfiguration(cluster) but incorrectly calls
useHyperConvergeConfiguration() without the cluster, causing toggleFeature to
patch the wrong cluster; update the call to useHyperConvergeConfiguration to
pass the same cluster argument so hyperConvergeConfiguration and toggleFeature
operate on the intended cluster (i.e., change the useHyperConvergeConfiguration
invocation in usePasstFeatureFlag to accept the cluster parameter).
---
Outside diff comments:
In `@src/utils/components/SSHAccess/components/VirtctlDisabled.tsx`:
- Line 24: The component VirtctlDisabled reads the cluster prop but calls
usePasstFeatureFlag() without it, causing incorrect feature detection across
clusters; update the hook invocation to pass the cluster prop (e.g., call
usePasstFeatureFlag(cluster) so the existing destructuring const {
featureEnabled: passtEnabled } = usePasstFeatureFlag(...) uses the correct
cluster-aware value) and ensure any TypeScript types/signature for
usePasstFeatureFlag accept the cluster argument if necessary.
---
Nitpick comments:
In `@src/utils/hooks/usePasstFeatureFlag.ts`:
- Line 15: The import in usePasstFeatureFlag currently includes a .ts extension;
update the import statement that references
'./useKubevirtHyperconvergeConfiguration.ts' to remove the extension (i.e.
import useKubevirtHyperconvergeConfiguration from
'./useKubevirtHyperconvergeConfiguration') so module resolution and bundlers
handle the module correctly while keeping the imported symbol
useKubevirtHyperconvergeConfiguration unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bdd72ff8-1631-4fc9-a310-4d7d62ed287f
📒 Files selected for processing (7)
src/utils/components/NetworkInterfaceModal/NetworkInterfaceModal.tsxsrc/utils/components/NetworkInterfaceModal/components/NetworkInterfacePasstSelect/NetworkInterfacePasst.tsxsrc/utils/components/SSHAccess/components/VirtctlDisabled.tsxsrc/utils/hooks/usePasstFeatureFlag.tssrc/utils/resources/vm/utils/constants.tssrc/views/clusteroverview/SettingsTab/PreviewFeaturesTab/hooks/constants.tssrc/views/clusteroverview/SettingsTab/PreviewFeaturesTab/hooks/usePreviewFeaturesData.tsx
💤 Files with no reviewable changes (1)
- src/views/clusteroverview/SettingsTab/PreviewFeaturesTab/hooks/constants.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/utils/components/NetworkInterfaceModal/NetworkInterfaceModal.tsx
- src/utils/resources/vm/utils/constants.ts
|
@upalatucci: This pull request references CNV-81600 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 epic to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
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. |
upalatucci
force-pushed
the
CNV-81600-passt-binding-method
branch
from
9994764 to
d873fcf
Compare
| @@ -24,9 +25,10 @@ const NetworkInterfacePasst: FC<NetworkInterfacePasstProps> = ({ | |||
| setInterfaceType, | |||
| }) => { | |||
| const { t } = useKubevirtTranslation(); | |||
| const cluster = useClusterParam(); | |||
| const [isOpen, setIsOpen] = useState(false); | |||
| const options = getPASSTSelectableOptions(t); | |||
| const passtFeatureFlag = usePasstFeatureFlag(); | |||
| const passtFeatureFlag = usePasstFeatureFlag(cluster); | |||
There was a problem hiding this comment.
I see that in all instances where you are calling usePasstFeatureFlag cluster is passed.
Can't you just call const cluster = useClusterParam(); and use it inside the custom hook?
There was a problem hiding this comment.
yes we can do that. I would put an optional input to this hook so if we are in a all-clusters view, we can alwyas call this hook with the vm cluster or something like that. Currently there is no situation like this but things can change in the future
upalatucci
force-pushed
the
CNV-81600-passt-binding-method
branch
from
58291cc to
3632803
Compare
Add support for the new passtBinding interface field alongside the legacy passt binding plugin approach. When the PassIPStackMigration feature gate is not enabled, VMs use the new native passtBinding field instead of the binding plugin reference. Made-with: Cursor
upalatucci
force-pushed
the
CNV-81600-passt-binding-method
branch
from
3632803 to
72059f2
Compare
|
@galkremer1 done |
|
[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 DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@upalatucci: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
3 participants
📝 Description
Support the new native
passtBindingnetwork interface field introduced alongside the legacy passt binding plugin approach.When the
PassIPStackMigrationfeature gate is not enabled in the HyperConverged config, VMs will use the new nativepasstBindinginterface field instead of the binding plugin reference. This allows the UI to correctly create and detect both legacy and new-style passt interfaces.Changes:
usePasstBindingMethodhook to detect whether the cluster uses legacy passt (viaPassIPStackMigrationfeature gate)createInterfacehelper to emitpasstBinding: {}field (without model/macAddress) for new-style passtgetNetworkInterfaceTypeselector to recognizepasstBindinginterfacesupdateInterfacepatch logic to handlebinding/passtBindingfield cleanupisLegacyPasstthrough all network interface modals (VM, Template, Wizard)🎥 Demo
Made with Cursor
Summary by CodeRabbit
New Features
Refactor
Dependencies