OCPBUGS-95592: Humanize memory and storage values in ResourceQuota display - #16874
OCPBUGS-95592: Humanize memory and storage values in ResourceQuota display#16874ericahinkleRH wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
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:
WalkthroughResource quota table values are humanized for memory and storage resources, missing values render as ChangesResource quota formatting
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 2
🧹 Nitpick comments (1)
frontend/public/components/__tests__/resource-quota.spec.tsx (1)
193-243: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the ACRQ formatting path.
These tests exercise only
ResourceQuota. Add anAppliedClusterResourceQuotafixture that verifies humanized namespace usage, total usage, and max values in the branch changed at Lines 209-211. Include missing and zero values to lock down the intendedDASHversus0behavior.🤖 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 `@frontend/public/components/__tests__/resource-quota.spec.tsx` around lines 193 - 243, Add an AppliedClusterResourceQuota fixture and test alongside the existing ResourceQuota cases in the ResourceUsageRow suite. Exercise the ACRQ formatting branch to verify humanized namespace usage, total usage, and maximum values, including missing values rendering as DASH and zero values rendering as 0.
🤖 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 `@frontend/public/components/resource-quota.jsx`:
- Around line 94-99: Update formatResourceValue so numeric zero is preserved
rather than converted to DASH. Replace the broad falsy check with an explicit
check for undefined, null, or an empty string, while leaving binary-resource
formatting and other values unchanged.
- Around line 85-91: Update isBinaryResourceType to use an allowlist of explicit
binary quota resource keys, rather than substring checks for “memory” or
“storage.” Ensure only the known byte-based keys are classified as binary, while
count-based keys such as storageclass quota resources remain non-binary.
---
Nitpick comments:
In `@frontend/public/components/__tests__/resource-quota.spec.tsx`:
- Around line 193-243: Add an AppliedClusterResourceQuota fixture and test
alongside the existing ResourceQuota cases in the ResourceUsageRow suite.
Exercise the ACRQ formatting branch to verify humanized namespace usage, total
usage, and maximum values, including missing values rendering as DASH and zero
values rendering as 0.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 81e7512e-02a4-4d26-804f-a560a256f341
📒 Files selected for processing (2)
frontend/public/components/__tests__/resource-quota.spec.tsxfrontend/public/components/resource-quota.jsx
| const isBinaryResourceType = (resourceType) => { | ||
| // Resource types that should be displayed as binary bytes (e.g., GiB, MiB) | ||
| return ( | ||
| resourceType.includes('memory') || | ||
| resourceType.includes('storage') || | ||
| resourceType.includes('ephemeral-storage') | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the file and inspect the relevant section with line numbers.
git ls-files 'frontend/public/components/resource-quota.jsx'
wc -l frontend/public/components/resource-quota.jsx
sed -n '1,180p' frontend/public/components/resource-quota.jsx | cat -n
# Find where the helper is used and any related quota formatting logic.
rg -n "isBinaryResourceType|humanize|resourceType" frontend/public/components frontend -g '!**/node_modules/**'Repository: openshift/console
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the resource quota tests around memory/storage handling.
sed -n '180,260p' frontend/public/components/__tests__/resource-quota.spec.tsx | cat -n
# Inspect the byte humanization helper to understand what happens to count values.
sed -n '330,430p' frontend/public/components/utils/units.js | cat -n
# Look for any resource quota keys that contain "storage" but are not byte quantities.
rg -n "\bstorage\b|storageclass\.storage\.k8s\.io|ephemeral-storage" frontend/public/components frontend/e2e -g '!**/node_modules/**'Repository: openshift/console
Length of output: 17646
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the humanization functions used by resource-quota.jsx.
sed -n '140,230p' frontend/public/components/utils/units.js | cat -n
# Check the Kubernetes quota docs mention the storage-class-specific PVC quota key.
python3 - <<'PY'
import urllib.request, re
url = "https://kubernetes.io/docs/concepts/policy/resource-quotas/"
html = urllib.request.urlopen(url, timeout=20).read().decode("utf-8", "replace")
for pat in [
r"storageclass\.storage\.k8s\.io/persistentvolumeclaims",
r"<storage-class-name>\.storageclass\.storage\.k8s\.io/persistentvolumeclaims",
]:
m = re.search(pat, html)
print(pat, "=>", bool(m))
PYRepository: openshift/console
Length of output: 5826
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the remaining unit-conversion internals.
sed -n '1,140p' frontend/public/components/utils/units.js | cat -n
sed -n '140,260p' frontend/public/components/utils/units.js | cat -n
# Search for resource-quota tests that exercise non-byte quota keys.
rg -n "persistentvolumeclaims|pods|services|configmaps|secrets|count/" frontend/public/components/__tests__/resource-quota.spec.tsx frontend/public/components/resource-quota.jsx frontend/public/components -g '!**/node_modules/**'Repository: openshift/console
Length of output: 29271
Match only explicit binary quota keys. includes('storage') also matches count-based keys like storageclass.storage.k8s.io/persistentvolumeclaims, so their values get byte formatting. Use an allowlist of the known byte-based resource keys instead.
🤖 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 `@frontend/public/components/resource-quota.jsx` around lines 85 - 91, Update
isBinaryResourceType to use an allowlist of explicit binary quota resource keys,
rather than substring checks for “memory” or “storage.” Ensure only the known
byte-based keys are classified as binary, while count-based keys such as
storageclass quota resources remain non-binary.
Source: MCP tools
| const formatResourceValue = (value, resourceType) => { | ||
| if (!value) { | ||
| return DASH; | ||
| } | ||
| return isBinaryResourceType(resourceType) ? humanizeBinaryBytes(value).string : value; | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve numeric zero values.
!value converts a legitimate 0 usage into DASH. The existing ACRQ fixture supplies numeric zero at Line 156 and expects 0 at Line 183, so this change causes that rendering/test to fail. Check explicitly for undefined, null, or an empty string instead.
🤖 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 `@frontend/public/components/resource-quota.jsx` around lines 94 - 99, Update
formatResourceValue so numeric zero is preserved rather than converted to DASH.
Replace the broad falsy check with an explicit check for undefined, null, or an
empty string, while leaving binary-resource formatting and other values
unchanged.
openshift-ci-robot
added
jira/valid-reference
|
@ericahinkleRH: This pull request references Jira Issue OCPBUGS-95592, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
The bug has been updated to refer to the pull request using the external bug tracker. 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. |
a2b96d5 to
ec68b31
Compare
|
/test pull-ci-openshift-console-main-frontend |
|
@ericahinkleRH: This pull request references Jira Issue OCPBUGS-95592, which is valid. 3 validation(s) were run on this bug
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. |
ec68b31 to
2359c76
Compare
|
/retest-required |
|
/ok-to-test |
openshift-ci
Bot
added
the
ok-to-test
|
/test backend |
|
/pipeline required |
|
Scheduling tests matching the |
|
/lgtm |
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ericahinkleRH, nader-ziada 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 |
2359c76 to
1352ece
Compare
|
New changes are detected. LGTM label has been removed. |
…splay This change improves the readability of ResourceQuota displays by formatting memory and storage-related values as human-readable binary units (e.g., GiB, MiB) instead of raw byte values. Key changes: - Add humanizeBinaryBytes import from utils/units - Add isBinaryResourceType() helper to identify memory/storage resources - Add formatResourceValue() to conditionally format values - Update ResourceUsageRow to use formatResourceValue for all displayed values Resources affected: - memory (e.g., limits.memory, requests.memory) - storage (e.g., persistentvolumeclaims, requests.storage) - ephemeral-storage Jira: https://issues.redhat.com/browse/OCPBUGS-95592
1352ece to
f8d1b56
Compare
|
/pipeline required |
|
Scheduling tests matching the |
|
/test e2e-gcp-console |
1 similar comment
|
/test e2e-gcp-console |
|
@ericahinkleRH: all tests passed! 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. |
4 participants
Analysis / Root cause:
ResourceQuota memory and storage limits were displayed as raw bytes (e.g., "2147483648"), making them difficult for users to read and interpret at a glance.
Solution description:
isBinaryResourceType()helper to identify resource types that should be displayed as binary bytes (memory, storage, ephemeral-storage)formatResourceValue()helper that conditionally applieshumanizeBinaryBytes()formattingResourceUsageRowcomponent to useformatResourceValue()for all quota value displaysScreenshots / screen recording:
Before: Memory values displayed as "2147483648"
After: Memory values displayed as "2 GiB"
(User will add screenshot showing the actual UI improvement)
Test setup:
Test cases:
Browser conformance:
Additional info:
Summary by CodeRabbit