ECOPROJECT-4793 | feat: add rightsizing information in the cluster recommendation output#766
Open
ammont82 wants to merge 1 commit into
Open
ECOPROJECT-4793 | feat: add rightsizing information in the cluster recommendation output#766ammont82 wants to merge 1 commit into
ammont82 wants to merge 1 commit into
Conversation
…commendation output Signed-off-by: Montse Ortega <mortegag@redhat.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
📝 WalkthroughWalkthroughAdds utilization-based cluster sizing comparison: a new ChangesUtilization Comparison Sizing Feature
Sequence Diagram(s)sequenceDiagram
participant User
participant SizingResult
participant hasUtilizationComparison
participant UtilizationComparisonCards
participant computeEffectiveCpu/Memory
User->>SizingResult: renders with sizerOutput
SizingResult->>hasUtilizationComparison: check(sizerOutput)
alt utilization comparison present
hasUtilizationComparison-->>SizingResult: true (UtilizationComparisonResponse)
SizingResult->>UtilizationComparisonCards: render(sizerOutput, formValues)
UtilizationComparisonCards->>computeEffectiveCpu/Memory: scale inventory by utilization %
computeEffectiveCpu/Memory-->>UtilizationComparisonCards: effective CPU / memory
UtilizationComparisonCards-->>User: baseline + recommended cards with savings banner
else no utilization comparison
hasUtilizationComparison-->>SizingResult: false
SizingResult-->>User: existing description-list sizing UI
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/ui/report/views/cluster-sizer/__tests__/utilizationSizing.test.ts (1)
53-70: ⚡ Quick winAdd explicit edge-case tests for nullable/undefined branches.
Please add assertions for
hasUtilizationComparison(undefined/null)andformatUtilizationPercent(undefined)to lock down the public helper contract.Suggested test additions
describe("hasUtilizationComparison", () => { + it("returns false for null/undefined output", () => { + expect(hasUtilizationComparison(undefined)).toBe(false); + expect(hasUtilizationComparison(null)).toBe(false); + }); + it("returns true when optimization succeeded with confidence above 50%", () => {describe("formatUtilizationPercent", () => { it("formats values with one decimal place", () => { expect(formatUtilizationPercent(45.2)).toBe("45.2%"); expect(formatUtilizationPercent(87.5)).toBe("87.5%"); }); + + it("returns em dash when value is undefined", () => { + expect(formatUtilizationPercent(undefined)).toBe("—"); + }); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ui/report/views/cluster-sizer/__tests__/utilizationSizing.test.ts` around lines 53 - 70, Add explicit edge-case test assertions to validate the behavior of public helper functions when passed undefined or null values. Create test cases for hasUtilizationComparison function with both undefined and null arguments to verify it handles these nullable inputs correctly, and add a test case for formatUtilizationPercent function with an undefined argument to ensure it gracefully handles undefined values. These tests should be added to their respective describe blocks in the test file to lock down the public contract of these utility functions.src/ui/report/views/cluster-sizer/utilizationSizing.ts (1)
1-44: ⚡ Quick winRename this file to match
src/**file naming rules.
utilizationSizing.tsis camelCase, but non-hook files undersrc/**are required to be PascalCase. Please rename it (for example,UtilizationSizing.ts) and update imports accordingly in the touched call sites.As per coding guidelines,
src/**: "File naming: files are PascalCase, hooks (use*) are camelCase, index/constants/types/styles are all-lowercase, directories are kebab-case."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ui/report/views/cluster-sizer/utilizationSizing.ts` around lines 1 - 44, The file utilizationSizing.ts violates the naming convention for non-hook files under src/**, which require PascalCase naming. Rename the file from utilizationSizing.ts to UtilizationSizing.ts and update all imports of this file at the touched call sites to reference the new file name.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/ui/report/views/cluster-sizer/UtilizationComparisonCards.tsx`:
- Around line 133-147: The UtilizationComparisonCards view component contains
business logic for data derivation (computing isSNO, extracting utilization
values, calculating effectiveCpu and effectiveMemory via computeEffectiveCpu and
computeEffectiveMemory functions, and computing savingsPercent) which violates
the guideline that views should be render-only. Create a custom view-model hook
(e.g., useUtilizationComparisonData) that takes the necessary inputs
(formValues, optimizedSizing, inventoryTotals, savings) and returns all these
computed values as an object, then replace all these derivation statements in
the component with a single call to that hook at the top of the component,
keeping the view focused solely on rendering.
---
Nitpick comments:
In `@src/ui/report/views/cluster-sizer/__tests__/utilizationSizing.test.ts`:
- Around line 53-70: Add explicit edge-case test assertions to validate the
behavior of public helper functions when passed undefined or null values. Create
test cases for hasUtilizationComparison function with both undefined and null
arguments to verify it handles these nullable inputs correctly, and add a test
case for formatUtilizationPercent function with an undefined argument to ensure
it gracefully handles undefined values. These tests should be added to their
respective describe blocks in the test file to lock down the public contract of
these utility functions.
In `@src/ui/report/views/cluster-sizer/utilizationSizing.ts`:
- Around line 1-44: The file utilizationSizing.ts violates the naming convention
for non-hook files under src/**, which require PascalCase naming. Rename the
file from utilizationSizing.ts to UtilizationSizing.ts and update all imports of
this file at the touched call sites to reference the new file name.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bcb2851a-628a-4121-a374-30b74097fea3
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
package.jsonsrc/ui/report/view-models/ClusterSizingHelpers.tssrc/ui/report/views/cluster-sizer/ClusterSizingWizard.tsxsrc/ui/report/views/cluster-sizer/SizingResult.tsxsrc/ui/report/views/cluster-sizer/UtilizationComparisonCards.tsxsrc/ui/report/views/cluster-sizer/__tests__/SizingResult.test.tsxsrc/ui/report/views/cluster-sizer/__tests__/mocks/ClusterRequirementsResponse.mock.tssrc/ui/report/views/cluster-sizer/__tests__/utilizationSizing.test.tssrc/ui/report/views/cluster-sizer/types.tssrc/ui/report/views/cluster-sizer/utilizationSizing.ts
Comment on lines
+133
to
+147
| const isSNO = formValues.clusterMode === "single-node"; | ||
|
|
||
| const cpuUtilization = optimizedSizing.cpuUtilizationMax; | ||
| const memoryUtilization = optimizedSizing.memoryUtilizationMax; | ||
| const effectiveCpu = computeEffectiveCpu( | ||
| inventoryTotals.totalCPU, | ||
| cpuUtilization, | ||
| ); | ||
| const effectiveMemory = computeEffectiveMemory( | ||
| inventoryTotals.totalMemory, | ||
| memoryUtilization, | ||
| ); | ||
|
|
||
| const savingsPercent = Math.round(savings.percentageReduction); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Move derived sizing computations into a view-model hook.
This view currently performs business/data-derivation logic (isSNO, effective CPU/memory, savings percent) instead of staying render-only.
Suggested direction
- export const UtilizationComparisonCards: React.FC<
- UtilizationComparisonCardsProps
- > = ({ sizerOutput, formValues }) => {
- const { clusterSizing, optimizedSizing, savings, inventoryTotals } =
- sizerOutput;
- const isSNO = formValues.clusterMode === "single-node";
- const cpuUtilization = optimizedSizing.cpuUtilizationMax;
- const memoryUtilization = optimizedSizing.memoryUtilizationMax;
- const effectiveCpu = computeEffectiveCpu(
- inventoryTotals.totalCPU,
- cpuUtilization,
- );
- const effectiveMemory = computeEffectiveMemory(
- inventoryTotals.totalMemory,
- memoryUtilization,
- );
- const savingsPercent = Math.round(savings.percentageReduction);
+ export const UtilizationComparisonCards: React.FC<
+ UtilizationComparisonCardsProps
+ > = ({ sizerOutput, formValues }) => {
+ const vm = useUtilizationComparisonCardsViewModel({ sizerOutput, formValues });
+ const {
+ clusterSizing,
+ optimizedSizing,
+ savings,
+ isSNO,
+ cpuUtilization,
+ memoryUtilization,
+ effectiveCpu,
+ effectiveMemory,
+ savingsPercent,
+ } = vm;As per coding guidelines, “Views in src/ui/*/views/ should render only with no business logic. Call view model hook at top.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ui/report/views/cluster-sizer/UtilizationComparisonCards.tsx` around
lines 133 - 147, The UtilizationComparisonCards view component contains business
logic for data derivation (computing isSNO, extracting utilization values,
calculating effectiveCpu and effectiveMemory via computeEffectiveCpu and
computeEffectiveMemory functions, and computing savingsPercent) which violates
the guideline that views should be render-only. Create a custom view-model hook
(e.g., useUtilizationComparisonData) that takes the necessary inputs
(formValues, optimizedSizing, inventoryTotals, savings) and returns all these
computed values as an object, then replace all these derivation statements in
the component with a single call to that hook at the top of the component,
keeping the view focused solely on rendering.
Source: Coding guidelines
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Chores
@openshift-migration-advisor/planner-sdkdependency to latest version.