MGMT-20351: selecting CNV - not able to deselect it using UI - new changes#2909
Conversation
…ssisted#2906)" This reverts commit 4f46665.
openshift-ci-robot
added
the
jira/valid-reference
|
@ammont82: This pull request references MGMT-20351 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.19.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
approved
WalkthroughThis PR modifies interactions related to the Migration Toolkit for Virtualization (MTV) across testing and UI components. It removes the click action and corresponding selector for the migration toolkit in tests, deletes the migration toolkit method from the operators page, adds a conditional check in a checkbox component to update state based on MTV support, removes a conditional block from the operators step component, and adjusts the logic in the CNV disabled check function. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/CnvCheckbox.tsxOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the config "@openshift-assisted/eslint-config" to extend from. Please check that the name of the config is correct. The config "@openshift-assisted/eslint-config" was referenced from the config file in "/libs/ui-lib/.eslintrc.cjs". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team. Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (4)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
openshift-ci
bot
added
the
size/S
|
@ammont82: This pull request references MGMT-20351 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.19.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: 0
🧹 Nitpick comments (1)
libs/ui-lib/lib/ocm/components/featureSupportLevels/featureStateUtils.ts (1)
363-370: Consider clarifying error messageThe error message could be more clear that MTV is a prerequisite for CNV, rather than suggesting they need to be installed together.
return mustDisableCnv - ? `Currently, you need to install ${CNV_OPERATOR_LABEL} operator at the same time as ${MTV_OPERATOR_LABEL} operator.` + ? `${MTV_OPERATOR_LABEL} is required to use ${CNV_OPERATOR_LABEL}. Please select ${MTV_OPERATOR_LABEL} first.` : undefined;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
libs/ui-lib-tests/cypress/integration/ui-behaviour/cluster-updates.cy.ts(0 hunks)libs/ui-lib-tests/cypress/views/operatorsPage.ts(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/CnvCheckbox.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsStep.tsx(0 hunks)libs/ui-lib/lib/ocm/components/featureSupportLevels/featureStateUtils.ts(1 hunks)
💤 Files with no reviewable changes (3)
- libs/ui-lib-tests/cypress/views/operatorsPage.ts
- libs/ui-lib-tests/cypress/integration/ui-behaviour/cluster-updates.cy.ts
- libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsStep.tsx
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: translation-files
- GitHub Check: tests
- GitHub Check: circular-deps
- GitHub Check: format
- GitHub Check: unit-tests
- GitHub Check: lint
🔇 Additional comments (3)
libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/CnvCheckbox.tsx (1)
80-84: Approve new MTV coupling behaviorThis change ensures that when OpenShift Virtualization (CNV) is selected or deselected, the Migration Toolkit for Virtualization (MTV) state will automatically be updated to match. This implements the requirement that MTV should be automatically checked when CNV is selected.
libs/ui-lib/lib/ocm/components/featureSupportLevels/featureStateUtils.ts (2)
363-370: Approve updated CNV-MTV relationship logicThis change modifies when CNV should be considered disabled with respect to MTV. The new logic only disables CNV if it's currently selected AND MTV is not selected, allowing CNV to be deselected while MTV remains selected.
This implements the requirement that users can uncheck the CNV operator if they wish to retain only the MTV operator.
363-370:Details
❓ Verification inconclusive
Verify behavior when CNV is disabled
This function returns a reason message only when CNV is selected but MTV is not. Let's verify this works correctly with the UI component when a user attempts to select CNV without MTV.
🏁 Script executed:
#!/bin/bash # Check how this function is used in the UI rg -A 5 "getCnvDisabledWithMtvReason" --type typescriptLength of output: 90
Action Required: Verify CNV Disable Behavior in UI
The function
getCnvDisabledWithMtvReasonis designed to return a reason message when the CNV operator is selected without also selecting the MTV operator. However, the initial verification command failed because the--type typescriptflag is not recognized byrg. Please verify the integration in the UI with the corrected search command below:After running this command, manually check that:
- The UI component calls
getCnvDisabledWithMtvReasonappropriately.- When a user selects CNV without MTV, the UI displays the message:
"Currently, you need to install CNV operator at the same time as MTV operator."- No message is shown when both operators are selected.
This verification will ensure that the operator selection behavior is correctly implemented.
🏁 Script executed:
#!/bin/bash # Re-run search for getCnvDisabledWithMtvReason in all TypeScript files rg -A 5 "getCnvDisabledWithMtvReason" -g "*.ts"Length of output: 851
Action Required: Manually Verify UI Integration
The function
getCnvDisabledWithMtvReasonis defined correctly to return the appropriate error message when CNV is enabled without MTV. However, our search only returned the function’s definition and did not reveal where (or if) it is referenced within the UI code. Please verify manually that the UI component calls this function correctly and shows the message:
- "Currently, you need to install CNV operator at the same time as MTV operator."
when a user selects CNV without also selecting MTV. If you find the function isn’t yet integrated into the UI, please address this gap.
ammont82
force-pushed
the
revert-changes-related-with-cnv
branch
from
ce873b8 to
6456410
Compare
|
@ammont82: This pull request references MGMT-20351 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.19.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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ammont82, rawagner 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 |
|
/lgtm |
openshift-merge-bot
bot
merged commit 54edede
into
openshift-assisted:master
|
/cherry-pick releases/v2.39 |
|
@ammont82: new pull request created: #2915 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. |
…anges (openshift-assisted#2909) * Revert "selecting CNV - not able to deselect it using UI (openshift-assisted#2906)" This reverts commit 4f46665. * Selecting CNV - not able to deselect it using UI
4 participants
https://issues.redhat.com/browse/MGMT-20351
After the change the CNV operator is disabled by default until we select the MTV operator because right now the CNV operator can be selected only when selecting the MTV as well.
and then it will be automatically checked but if user wants it can remove the CNV operator and remain only with MTV.
Summary by CodeRabbit