NO-ISSUE: Remove orphan files#3460
NO-ISSUE: Remove orphan files#3460LiorSoffer wants to merge 1 commit intoopenshift-assisted:masterfrom
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: LiorSoffer The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
size/L
📝 WalkthroughWalkthroughMoved four cluster-detail helper functions into a new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
📝 Coding Plan
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 Tip You can enable review details to help with troubleshooting, context usage and more.Enable the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
libs/ui-lib/lib/ocm/components/clusterDetail/clusterPropertiesUtils.tsx (1)
5-19: Simplify helpers by returning expressions directlyBoth helpers are correct, but removing temporary mutable variables will make the code shorter and easier to scan.
♻️ Proposed refactor
export const getNetworkType = (clusterNetworkType: Cluster['networkType']): string => { - let networkType: string; - clusterNetworkType === NETWORK_TYPE_SDN - ? (networkType = 'Software-Defined Networking (SDN)') - : (networkType = 'Open Virtual Network (OVN)'); - return networkType; + return clusterNetworkType === NETWORK_TYPE_SDN + ? 'Software-Defined Networking (SDN)' + : 'Open Virtual Network (OVN)'; }; export const getManagementType = ({ userManagedNetworking }: Cluster): string => { - let managementType: string; - userManagedNetworking - ? (managementType = 'User-Managed Networking') - : (managementType = 'Cluster-managed networking'); - return managementType; + return userManagedNetworking ? 'User-Managed Networking' : 'Cluster-managed networking'; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-lib/lib/ocm/components/clusterDetail/clusterPropertiesUtils.tsx` around lines 5 - 19, Replace the temporary mutable variables in getNetworkType and getManagementType with direct expression returns: for getNetworkType return the ternary expression based on clusterNetworkType (comparing to NETWORK_TYPE_SDN) instead of assigning to networkType, and for getManagementType return the ternary expression based on the Cluster's userManagedNetworking property instead of assigning to managementType; update only those functions (getNetworkType, getManagementType) to return the expressions directly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@libs/ui-lib/lib/ocm/components/clusterDetail/clusterPropertiesUtils.tsx`:
- Around line 5-19: Replace the temporary mutable variables in getNetworkType
and getManagementType with direct expression returns: for getNetworkType return
the ternary expression based on clusterNetworkType (comparing to
NETWORK_TYPE_SDN) instead of assigning to networkType, and for getManagementType
return the ternary expression based on the Cluster's userManagedNetworking
property instead of assigning to managementType; update only those functions
(getNetworkType, getManagementType) to return the expressions directly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0bc004a3-8b16-416e-9e01-c87cca2c4a5c
📒 Files selected for processing (8)
libs/locales/lib/en/translation.jsonlibs/ui-lib/lib/ocm/components/clusterConfiguration/review/ReviewClusterDetailTable.tsxlibs/ui-lib/lib/ocm/components/clusterConfiguration/review/ReviewNetworkingTable.tsxlibs/ui-lib/lib/ocm/components/clusterDetail/AssistedInstallerExtraDetailCard.tsxlibs/ui-lib/lib/ocm/components/clusterDetail/ClusterProperties.tsxlibs/ui-lib/lib/ocm/components/clusterDetail/ClusterPropertiesUtils.test.tslibs/ui-lib/lib/ocm/components/clusterDetail/clusterPropertiesUtils.tsxlibs/ui-lib/lib/ocm/components/clusterDetail/index.ts
💤 Files with no reviewable changes (4)
- libs/ui-lib/lib/ocm/components/clusterDetail/index.ts
- libs/locales/lib/en/translation.json
- libs/ui-lib/lib/ocm/components/clusterDetail/AssistedInstallerExtraDetailCard.tsx
- libs/ui-lib/lib/ocm/components/clusterDetail/ClusterProperties.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- libs/ui-lib/lib/ocm/components/clusterConfiguration/review/ReviewNetworkingTable.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/review/ReviewClusterDetailTable.tsx
Signed-off-by: Lior Soffer <liorsoffer1@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
libs/ui-lib/lib/ocm/components/clusterDetail/clusterPropertiesUtils.tsx (1)
5-11: Consider simplifying to a direct ternary return.The intermediate variable and assignment-in-ternary pattern is verbose. A direct return is cleaner and more idiomatic.
♻️ Suggested simplification
export const getNetworkType = (clusterNetworkType: Cluster['networkType']): string => { - let networkType: string; - clusterNetworkType === NETWORK_TYPE_SDN - ? (networkType = 'Software-Defined Networking (SDN)') - : (networkType = 'Open Virtual Network (OVN)'); - return networkType; + return clusterNetworkType === NETWORK_TYPE_SDN + ? 'Software-Defined Networking (SDN)' + : 'Open Virtual Network (OVN)'; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-lib/lib/ocm/components/clusterDetail/clusterPropertiesUtils.tsx` around lines 5 - 11, The function getNetworkType currently uses an intermediate variable and assignment inside a ternary; simplify it to return the string directly from the ternary expression using the Cluster['networkType'] parameter and the NETWORK_TYPE_SDN constant (i.e., replace the variable + ternary-assignment pattern with a single return clusterNetworkType === NETWORK_TYPE_SDN ? 'Software-Defined Networking (SDN)' : 'Open Virtual Network (OVN)').
🤖 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/ocm/components/clusterDetail/clusterPropertiesUtils.tsx`:
- Around line 13-19: The getManagementType function returns inconsistent label
capitalization; update the two returned strings so they match (e.g., change
"Cluster-managed networking" to "Cluster-Managed Networking" to match
"User-Managed Networking") by editing the string literals in getManagementType
(which reads the Cluster.userManagedNetworking flag) so both labels use the same
title-case style.
- Around line 24-44: The getDiskEncryptionEnabledOnStatus function currently
only handles 'all', 'masters', and 'workers' and returns null for other
DiskEncryption['enableOn'] values; update this function to add switch cases for
'arbiters' (render "Enabled on arbiters"), 'masters,arbiters' (render "Enabled
on control plane nodes and arbiters"), 'masters,workers' (render "Enabled on
control plane nodes and workers"), and 'arbiters,workers' (render "Enabled on
arbiters and workers") so the disk encryption row is shown for those
combinations; use the existing JSX pattern (fragments and <br /> where needed)
within getDiskEncryptionEnabledOnStatus to match the current UI style.
---
Nitpick comments:
In `@libs/ui-lib/lib/ocm/components/clusterDetail/clusterPropertiesUtils.tsx`:
- Around line 5-11: The function getNetworkType currently uses an intermediate
variable and assignment inside a ternary; simplify it to return the string
directly from the ternary expression using the Cluster['networkType'] parameter
and the NETWORK_TYPE_SDN constant (i.e., replace the variable +
ternary-assignment pattern with a single return clusterNetworkType ===
NETWORK_TYPE_SDN ? 'Software-Defined Networking (SDN)' : 'Open Virtual Network
(OVN)').
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: edda31f6-2b22-4bb6-ba88-075c3ec9b057
📒 Files selected for processing (8)
libs/locales/lib/en/translation.jsonlibs/ui-lib/lib/ocm/components/clusterConfiguration/review/ReviewClusterDetailTable.tsxlibs/ui-lib/lib/ocm/components/clusterConfiguration/review/ReviewNetworkingTable.tsxlibs/ui-lib/lib/ocm/components/clusterDetail/AssistedInstallerExtraDetailCard.tsxlibs/ui-lib/lib/ocm/components/clusterDetail/ClusterProperties.tsxlibs/ui-lib/lib/ocm/components/clusterDetail/ClusterPropertiesUtils.test.tslibs/ui-lib/lib/ocm/components/clusterDetail/clusterPropertiesUtils.tsxlibs/ui-lib/lib/ocm/components/clusterDetail/index.ts
💤 Files with no reviewable changes (4)
- libs/ui-lib/lib/ocm/components/clusterDetail/AssistedInstallerExtraDetailCard.tsx
- libs/locales/lib/en/translation.json
- libs/ui-lib/lib/ocm/components/clusterDetail/ClusterProperties.tsx
- libs/ui-lib/lib/ocm/components/clusterDetail/index.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- libs/ui-lib/lib/ocm/components/clusterConfiguration/review/ReviewClusterDetailTable.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/review/ReviewNetworkingTable.tsx
- libs/ui-lib/lib/ocm/components/clusterDetail/ClusterPropertiesUtils.test.ts
| export const getManagementType = ({ userManagedNetworking }: Cluster): string => { | ||
| let managementType: string; | ||
| userManagedNetworking | ||
| ? (managementType = 'User-Managed Networking') | ||
| : (managementType = 'Cluster-managed networking'); | ||
| return managementType; | ||
| }; |
There was a problem hiding this comment.
Inconsistent capitalization in management type labels.
"User-Managed Networking" uses title case while "Cluster-managed networking" uses sentence case. This creates an inconsistent UI appearance.
✏️ Proposed fix for consistency
export const getManagementType = ({ userManagedNetworking }: Cluster): string => {
- let managementType: string;
- userManagedNetworking
- ? (managementType = 'User-Managed Networking')
- : (managementType = 'Cluster-managed networking');
- return managementType;
+ return userManagedNetworking
+ ? 'User-Managed Networking'
+ : 'Cluster-Managed Networking';
};📝 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.
| export const getManagementType = ({ userManagedNetworking }: Cluster): string => { | |
| let managementType: string; | |
| userManagedNetworking | |
| ? (managementType = 'User-Managed Networking') | |
| : (managementType = 'Cluster-managed networking'); | |
| return managementType; | |
| }; | |
| export const getManagementType = ({ userManagedNetworking }: Cluster): string => { | |
| return userManagedNetworking | |
| ? 'User-Managed Networking' | |
| : 'Cluster-Managed Networking'; | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/ui-lib/lib/ocm/components/clusterDetail/clusterPropertiesUtils.tsx`
around lines 13 - 19, The getManagementType function returns inconsistent label
capitalization; update the two returned strings so they match (e.g., change
"Cluster-managed networking" to "Cluster-Managed Networking" to match
"User-Managed Networking") by editing the string literals in getManagementType
(which reads the Cluster.userManagedNetworking flag) so both labels use the same
title-case style.
There was a problem hiding this comment.
Should be sentence case per PF guidelines https://www.patternfly.org/ux-writing/capitalization
Feel free to make it consistent across the project @LiorSoffer
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| export const getDiskEncryptionEnabledOnStatus = (diskEncryption: DiskEncryption['enableOn']) => { | ||
| let diskEncryptionType = null; | ||
| switch (diskEncryption) { | ||
| case 'all': | ||
| diskEncryptionType = ( | ||
| <> | ||
| Enabled on control plane nodes | ||
| <br /> | ||
| Enabled on workers | ||
| </> | ||
| ); | ||
| break; | ||
| case 'masters': | ||
| diskEncryptionType = <>Enabled on control plane nodes</>; | ||
| break; | ||
| case 'workers': | ||
| diskEncryptionType = <>Enabled on workers</>; | ||
| break; | ||
| } | ||
| return diskEncryptionType; | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how getDiskEncryptionEnabledOnStatus is used and whether arbiters/combinations appear elsewhere
# Find usages of getDiskEncryptionEnabledOnStatus
echo "=== Usages of getDiskEncryptionEnabledOnStatus ==="
rg -n "getDiskEncryptionEnabledOnStatus" --type=ts --type=tsx -C2
# Check if 'arbiters' or combination values are referenced in disk encryption contexts
echo "=== References to arbiters in disk encryption contexts ==="
rg -n "arbiters" --type=ts --type=tsx -C2
# Check the DiskEncryption type definition
echo "=== DiskEncryption type definition ==="
rg -nP "enableOn.*:" -A10 --type=ts | head -50Repository: openshift-assisted/assisted-installer-ui
Length of output: 3332
🏁 Script executed:
# Search more broadly for the function
echo "=== Direct file search ==="
cat -n libs/ui-lib/lib/ocm/components/clusterDetail/clusterPropertiesUtils.tsx | head -60
echo ""
echo "=== Search for imports of clusterPropertiesUtils ==="
rg -n "clusterPropertiesUtils" --type-list | head -5
rg -n "clusterPropertiesUtils" -A 2
echo ""
echo "=== Search with simpler patterns ==="
rg -n "getDiskEncryptionEnabledOnStatus"
echo ""
echo "=== Check if getDiskEncryptionEnabledOnStatus is exported ==="
grep -n "export\|getDiskEncryptionEnabledOnStatus" libs/ui-lib/lib/ocm/components/clusterDetail/clusterPropertiesUtils.tsxRepository: openshift-assisted/assisted-installer-ui
Length of output: 4616
🏁 Script executed:
# Check the usage context in ReviewClusterDetailTable.tsx
sed -n '60,80p' libs/ui-lib/lib/ocm/components/clusterConfiguration/review/ReviewClusterDetailTable.tsx
# Also check if there's any handling for null/undefined values
sed -n '60,90p' libs/ui-lib/lib/ocm/components/clusterConfiguration/review/ReviewClusterDetailTable.tsxRepository: openshift-assisted/assisted-installer-ui
Length of output: 1453
Handle additional enableOn values in disk encryption status display.
The function unhandled cases ('arbiters', 'masters,arbiters', 'masters,workers', 'arbiters,workers') that are actively generated by DiskEncryptionService. When these values occur, getDiskEncryptionEnabledOnStatus returns null, causing the disk encryption row to not appear in the cluster details review table.
Add cases for these combinations to display appropriate status text:
'arbiters'→ "Enabled on arbiters"'masters,arbiters'→ "Enabled on control plane nodes and arbiters"'masters,workers'→ "Enabled on control plane nodes and workers"'arbiters,workers'→ "Enabled on arbiters and workers"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/ui-lib/lib/ocm/components/clusterDetail/clusterPropertiesUtils.tsx`
around lines 24 - 44, The getDiskEncryptionEnabledOnStatus function currently
only handles 'all', 'masters', and 'workers' and returns null for other
DiskEncryption['enableOn'] values; update this function to add switch cases for
'arbiters' (render "Enabled on arbiters"), 'masters,arbiters' (render "Enabled
on control plane nodes and arbiters"), 'masters,workers' (render "Enabled on
control plane nodes and workers"), and 'arbiters,workers' (render "Enabled on
arbiters and workers") so the disk encryption row is shown for those
combinations; use the existing JSX pattern (fragments and <br /> where needed)
within getDiskEncryptionEnabledOnStatus to match the current UI style.
| import { isDualStack, NETWORK_TYPE_SDN } from '../../../common'; | ||
| import { Cluster, DiskEncryption } from '@openshift-assisted/types/assisted-installer-service'; | ||
|
|
||
| export const getNetworkType = (clusterNetworkType: Cluster['networkType']): string => { |
There was a problem hiding this comment.
This is also unused (the getNetworkType function in /cim is a different one). Let's remove it.
| return networkType; | ||
| }; | ||
|
|
||
| export const getManagementType = ({ userManagedNetworking }: Cluster): string => { |
There was a problem hiding this comment.
While we're at it, this could be simpler. Let's make it a ternary operator.
There was a problem hiding this comment.
Since all of these functions appear to be connected to the Review step, I'd put them in a utils.tsx file in the review folder.
openshift-ci-robot
added
the
jira/valid-reference
|
@LiorSoffer: This pull request explicitly references no jira issue. 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. |
3 participants
Summary by CodeRabbit
Refactor
Chores
Localization