MGMT-23299: Make secondary VIP fields optional#3417
MGMT-23299: Make secondary VIP fields optional#3417openshift-merge-bot[bot] merged 2 commits intoopenshift-assisted:masterfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@LiorSoffer: This pull request references MGMT-23299 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. 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. |
openshift-ci
bot
added
the
size/M
|
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:
📝 WalkthroughWalkthroughRemoved automatic dual‑stack VIP auto‑initialization in StackTypeControlGroup; VirtualIPControlGroup now ensures Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 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)
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. Comment |
|
@LiorSoffer: This pull request references MGMT-23299 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. 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/ui-lib/lib/common/components/clusterWizard/networkingSteps/VirtualIPControlGroup.tsx (1)
108-142: 🛠️ Refactor suggestion | 🟠 MajorExisting "ensure VIP arrays exist" effect creates 2-element arrays in dual-stack — this conflicts with the new pruning logic.
Line 118 computes
vipCount = values.stackType === DUAL_STACK ? 2 : 1. WhenapiVipsis truly empty (length 0) in dual-stack mode, this effect creates a 2-element array of blank VIPs — which the new pruning effect on lines 145-172 will immediately strip back to 1 element (since both secondaries are empty). This causes two unnecessarysetFieldValuecalls every time VIPs transition from empty in dual-stack mode.To stay consistent with the "secondary VIPs are optional" intent, update
vipCountto always be1:Suggested fix
- const vipCount = values.stackType === DUAL_STACK ? 2 : 1; + const vipCount = 1;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-lib/lib/common/components/clusterWizard/networkingSteps/VirtualIPControlGroup.tsx` around lines 108 - 142, In the VirtualIPControlGroup useEffect that ensures VIP arrays exist, remove the dual-stack branch and always set vipCount to 1 (i.e., replace the vipCount calculation that uses values.stackType === DUAL_STACK ? 2 : 1 with a constant 1) so blankVipEntriesArray is created with a single entry; keep the rest of the effect (apiVips/ingressVips emptiness checks and setFieldValue calls) unchanged to avoid unnecessary setFieldValue calls that the pruning logic will immediately reverse.
🤖 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
`@libs/ui-lib/lib/common/components/clusterWizard/networkingSteps/VirtualIPControlGroup.tsx`:
- Around line 108-142: In the VirtualIPControlGroup useEffect that ensures VIP
arrays exist, remove the dual-stack branch and always set vipCount to 1 (i.e.,
replace the vipCount calculation that uses values.stackType === DUAL_STACK ? 2 :
1 with a constant 1) so blankVipEntriesArray is created with a single entry;
keep the rest of the effect (apiVips/ingressVips emptiness checks and
setFieldValue calls) unchanged to avoid unnecessary setFieldValue calls that the
pruning logic will immediately reverse.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libs/ui-lib/lib/common/components/clusterWizard/networkingSteps/StackTypeControlGroup.tsxlibs/ui-lib/lib/common/components/clusterWizard/networkingSteps/VirtualIPControlGroup.tsx
libs/ui-lib/lib/common/components/clusterWizard/networkingSteps/StackTypeControlGroup.tsx
Show resolved
Hide resolved
|
@LiorSoffer: This pull request references MGMT-23299 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. 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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@libs/ui-lib/lib/common/components/clusterWizard/networkingSteps/VirtualIPControlGroup.tsx`:
- Around line 110-137: The effect in VirtualIPControlGroup currently requires
both values.apiVips and values.ingressVips to have length >= 2 before pruning,
which prevents pruning when one array lacks a secondary slot; remove the
explicit length checks ((values.apiVips?.length ?? 0) >= 2 &&
(values.ingressVips?.length ?? 0) >= 2) and rely on the existing per-index
emptiness checks (apiVips[1]?.ip and ingressVips[1]?.ip) so missing secondary
entries are treated as empty and setFieldValue('apiVips', [apiVips[0]], false) /
setFieldValue('ingressVips', [ingressVips[0]], false) will run when both
secondaries are effectively empty.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libs/ui-lib/lib/common/components/clusterWizard/networkingSteps/StackTypeControlGroup.tsxlibs/ui-lib/lib/common/components/clusterWizard/networkingSteps/VirtualIPControlGroup.tsx
💤 Files with no reviewable changes (1)
- libs/ui-lib/lib/common/components/clusterWizard/networkingSteps/StackTypeControlGroup.tsx
libs/ui-lib/lib/common/components/clusterWizard/networkingSteps/VirtualIPControlGroup.tsx
Outdated
Show resolved
Hide resolved
|
@LiorSoffer: This pull request references MGMT-23299 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. 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. |
|
@LiorSoffer: This pull request references MGMT-23299 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. 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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@libs/ui-lib/lib/common/components/clusterWizard/networkingSteps/VirtualIPControlGroup.tsx`:
- Around line 155-169: The pruning logic can silently drop >2 VIPs; update the
validation to enforce the API contract by adding a maximum length of 2 to the
array schema (add .max(2) to vipArrayValidationSchema) so apiVips and
ingressVips cannot contain more than two entries, and keep the existing prune
logic (setFieldValue('apiVips', [apiVips[0]], false) /
setFieldValue('ingressVips', [ingressVips[0]], false)) as-is; alternatively, if
you prefer runtime defense, tighten the pruning check in the block that computes
shouldPrune to only collapse the second slot (preserve any indices >=2) instead
of replacing with a single-element array.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libs/ui-lib/lib/common/components/clusterWizard/networkingSteps/StackTypeControlGroup.tsxlibs/ui-lib/lib/common/components/clusterWizard/networkingSteps/VirtualIPControlGroup.tsx
💤 Files with no reviewable changes (1)
- libs/ui-lib/lib/common/components/clusterWizard/networkingSteps/StackTypeControlGroup.tsx
libs/ui-lib/lib/common/components/clusterWizard/networkingSteps/VirtualIPControlGroup.tsx
Outdated
Show resolved
Hide resolved
|
@LiorSoffer: This pull request references MGMT-23299 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. 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/ui-lib/lib/common/components/clusterWizard/networkingSteps/VirtualIPControlGroup.tsx (1)
98-98:⚠️ Potential issue | 🟡 MinorPotential stale closure:
enableAllocationdepends onvalues.networkTypenot in dependencies.
enableAllocationis computed fromvalues.networkType(line 98), butvalues.networkTypeis not included in theuseEffectdependency array. IfnetworkTypechanges,enableAllocationwill have a stale value until another dependency triggers re-run.💡 Suggested fix to add missing dependency
}, [ cluster.id, enableAllocation, isViewerMode, setFieldValue, values.apiVips, values.ingressVips, values.managedNetworkingType, + values.networkType, values.stackType, values.vipDhcpAllocation, ]);Also applies to: 166-176
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-lib/lib/common/components/clusterWizard/networkingSteps/VirtualIPControlGroup.tsx` at line 98, The computed flag enableAllocation (const enableAllocation = values.networkType === NETWORK_TYPE_SDN) can become stale because values.networkType is not included in the useEffect dependencies in VirtualIPControlGroup; update the relevant useEffect(s) that reference enableAllocation (around the block originally at lines 166-176) to include values.networkType (or enableAllocation) in the dependency array so the effect re-runs when networkType changes, ensuring derived state stays in sync.
🧹 Nitpick comments (1)
libs/ui-lib/lib/common/components/clusterWizard/networkingSteps/VirtualIPControlGroup.tsx (1)
124-129: Reference equality check may cause unnecessary re-renders or miss updates.The comparisons
nextApiVips !== values.apiVipsandnextIngressVips !== values.ingressVipsuse reference equality. After dual-stack synchronization (lines 137-141) or pruning (lines 154-156),nextApiVips/nextIngressVipsare reassigned to new arrays, but the comparison still checks against the originalvalues.*reference. This means:
- When padding/pruning occurs, the new arrays will always trigger
setFieldValue(correct behavior).- However, when
ensureClusterIdreturns a new array with the same content as before (e.g., if all entries already hadclusterIdbut the.some()check was true due to a race), it might still trigger an update.Consider using a deep equality check or comparing serialized values to avoid potential unnecessary form updates:
💡 Suggested improvement using JSON comparison
- if (nextApiVips !== values.apiVips) { + if (JSON.stringify(nextApiVips) !== JSON.stringify(values.apiVips)) { setFieldValue('apiVips', nextApiVips, false); } - if (nextIngressVips !== values.ingressVips) { + if (JSON.stringify(nextIngressVips) !== JSON.stringify(values.ingressVips)) { setFieldValue('ingressVips', nextIngressVips, false); }Also applies to: 160-165
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-lib/lib/common/components/clusterWizard/networkingSteps/VirtualIPControlGroup.tsx` around lines 124 - 129, The current checks use reference equality (nextApiVips !== values.apiVips and nextIngressVips !== values.ingressVips) which can cause unnecessary setFieldValue calls or miss true content changes; replace those reference comparisons with a deep-equality comparison (e.g., use JSON.stringify(...) or lodash/isEqual) when comparing nextApiVips to values.apiVips and nextIngressVips to values.ingressVips after calling ensureClusterId and during padding/pruning logic so you only call setFieldValue when the actual array contents differ; target the code around nextApiVips/nextIngressVips, ensureClusterId, and the setFieldValue calls for both API and Ingress VIP handling.
🤖 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
`@libs/ui-lib/lib/common/components/clusterWizard/networkingSteps/VirtualIPControlGroup.tsx`:
- Line 98: The computed flag enableAllocation (const enableAllocation =
values.networkType === NETWORK_TYPE_SDN) can become stale because
values.networkType is not included in the useEffect dependencies in
VirtualIPControlGroup; update the relevant useEffect(s) that reference
enableAllocation (around the block originally at lines 166-176) to include
values.networkType (or enableAllocation) in the dependency array so the effect
re-runs when networkType changes, ensuring derived state stays in sync.
---
Nitpick comments:
In
`@libs/ui-lib/lib/common/components/clusterWizard/networkingSteps/VirtualIPControlGroup.tsx`:
- Around line 124-129: The current checks use reference equality (nextApiVips
!== values.apiVips and nextIngressVips !== values.ingressVips) which can cause
unnecessary setFieldValue calls or miss true content changes; replace those
reference comparisons with a deep-equality comparison (e.g., use
JSON.stringify(...) or lodash/isEqual) when comparing nextApiVips to
values.apiVips and nextIngressVips to values.ingressVips after calling
ensureClusterId and during padding/pruning logic so you only call setFieldValue
when the actual array contents differ; target the code around
nextApiVips/nextIngressVips, ensureClusterId, and the setFieldValue calls for
both API and Ingress VIP handling.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libs/ui-lib/lib/common/components/clusterWizard/networkingSteps/StackTypeControlGroup.tsxlibs/ui-lib/lib/common/components/clusterWizard/networkingSteps/VirtualIPControlGroup.tsx
💤 Files with no reviewable changes (1)
- libs/ui-lib/lib/common/components/clusterWizard/networkingSteps/StackTypeControlGroup.tsx
openshift-ci
bot
added
size/S
|
/cherry-pick releases/v2.50 |
|
@LiorSoffer: once the present PR merges, I will cherry-pick it on top of 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 kubernetes-sigs/prow repository. |
openshift-ci
bot
added
size/M
Signed-off-by: Lior Soffer <lsoffer@redhat.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ElayAharoni, jgyselov, LiorSoffer 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 |
openshift-ci
bot
added
the
approved
|
/cherry-pick releases/v2.17-cim |
|
@LiorSoffer: once the present PR merges, I will cherry-pick it on top of 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 kubernetes-sigs/prow repository. |
openshift-merge-bot
bot
merged commit 2150689
into
openshift-assisted:master
|
@LiorSoffer: new pull request created: #3422 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 kubernetes-sigs/prow repository. |
|
@LiorSoffer: new pull request created: #3423 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 kubernetes-sigs/prow repository. |
5 participants
https://issues.redhat.com/browse/MGMT-23299
Summary by CodeRabbit
Bug Fixes
Improvements