OCPBUGS-68332: Improve VAC field on PVC details page #15843
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
openshift-ci-robot
added
jira/valid-reference
|
@cajieh: This pull request references Jira Issue OCPBUGS-68332, which is invalid:
Comment 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. |
WalkthroughAdds VolumeAttributesClass (VAC) handling to PVC details: derives VAC error/pending state, shows dismissible danger/info alerts, resets dismissals when the PVC changes, and renders separate "Requested VolumeAttributesClass" and "VolumeAttributesClass" fields. Also adds English localization keys for VAC messages. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (1)**⚙️ CodeRabbit configuration file
Files:
🧬 Code graph analysis (1)frontend/public/components/persistent-volume-claim.tsx (3)
🔇 Additional comments (3)
Comment |
openshift-ci
bot
added
component/core
cajieh
force-pushed
the
improve-vac-field-pvc-details-page
branch
5 times, most recently
from
e2c8423 to
6e1d9bd
Compare
openshift-ci
bot
added
the
kind/i18n
cajieh
force-pushed
the
improve-vac-field-pvc-details-page
branch
4 times, most recently
from
30f2e99 to
634117b
Compare
cajieh
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
cajieh
force-pushed
the
improve-vac-field-pvc-details-page
branch
from
634117b to
34a8cdd
Compare
![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 (1)
frontend/public/components/persistent-volume-claim.tsx (1)
345-349: Minor: Confusing function naming.Using
truncateMiddlewithtruncateEnd: trueis functionally correct but semantically confusing—the name implies middle truncation while the parameter specifies end truncation.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
frontend/public/components/persistent-volume-claim.tsx(6 hunks)frontend/public/locales/en/public.json(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
frontend/public/locales/en/public.jsonfrontend/public/components/persistent-volume-claim.tsx
🧬 Code graph analysis (1)
frontend/public/components/persistent-volume-claim.tsx (3)
frontend/public/module/k8s/types.ts (1)
PersistentVolumeClaimKind(1131-1159)frontend/public/module/k8s/k8s.ts (1)
referenceFor(50-65)frontend/public/models/index.ts (1)
VolumeAttributesClassModel(810-824)
🔇 Additional comments (4)
frontend/public/components/persistent-volume-claim.tsx (3)
82-105: LGTM! VAC state logic is correct.The helper function correctly identifies VAC modification states by checking for
ModifyVolumeErrorconditions and determining pending status based on attribute name mismatches. The logic appropriately handles all scenarios: initial application, modification, and removal.
280-289: LGTM! Alert dismissal state management is well-implemented.The useEffect dependency on
pvcUidcorrectly resets dismissed alerts when the PVC resource changes, while maintaining the dismissed state across status-only updates to the same PVC object.
337-380: Alert rendering logic is well-structured.The VAC alerts correctly handle error and pending states with appropriate titles and messages. The conditional rendering ensures alerts are only shown when VAC is supported and the conditions are met.
frontend/public/locales/en/public.json (1)
1131-1137: LGTM! Translation keys are well-defined.The new VAC-related translation keys comprehensively cover all lifecycle states with clear, user-friendly messages. The i18next interpolation syntax (
{{target}},{{current}}) is correctly used for dynamic values.Also applies to: 1146-1146
| {isVACSupported && ( | ||
| <DescriptionListGroup> | ||
| <DescriptionListTerm> | ||
| {t('public~Requested VolumeAttributesClass')} | ||
| </DescriptionListTerm> | ||
| <DescriptionListDescription data-test-id="pvc-requested-vac"> | ||
| {volumeAttributesClassName ? ( | ||
| <ResourceLink | ||
| kind={referenceFor(VolumeAttributesClassModel)} | ||
| name={volumeAttributesClassName} | ||
| /> | ||
| </DescriptionListDescription> | ||
| </DescriptionListGroup> | ||
| )} | ||
| ) : ( | ||
| DASH | ||
| )} | ||
| </DescriptionListDescription> | ||
| </DescriptionListGroup> | ||
| )} | ||
| {isVACSupported && !!currentVolumeAttributesClassName && ( | ||
| <DescriptionListGroup> | ||
| <DescriptionListTerm>{t('public~VolumeAttributesClass')}</DescriptionListTerm> | ||
| <DescriptionListDescription data-test-id="pvc-current-vac"> | ||
| <ResourceLink | ||
| kind={referenceFor(VolumeAttributesClassModel)} | ||
| name={currentVolumeAttributesClassName} | ||
| /> | ||
| </DescriptionListDescription> | ||
| </DescriptionListGroup> | ||
| )} |
There was a problem hiding this comment.
Critical: Incorrect API reference construction for VolumeAttributesClass.
The usage of referenceFor(VolumeAttributesClassModel) on lines 480 and 494 is problematic:
VolumeAttributesClassModelis aK8sKindwithapiVersion: 'v1'andapiGroup: 'storage.k8s.io'as separate fieldsreferenceForexpects aK8sResourceCommonand uses only theapiVersionfield- This will construct an incorrect reference pointing to
core/v1instead ofstorage.k8s.io/v1 - The
ResourceLinkwill fail to resolve the correct API endpoint
🔎 Verify and fix the API reference construction:
#!/bin/bash
# Verify if referenceForModel exists and check for correct usage patterns
echo "=== Searching for referenceForModel usage ==="
rg -n 'referenceForModel' --type ts --type tsx
echo "=== Searching for similar ResourceLink patterns with models ==="
rg -n 'ResourceLink.*kind=\{referenceFor' --type ts --type tsx -A2 -B2
echo "=== Checking VolumeAttributesClassModel definition ==="
rg -n 'VolumeAttributesClassModel.*K8sKind' --type ts --type tsx -A15Consider one of these fixes:
Option 1: Use referenceForModel if available
import { referenceForModel } from '@console/internal/module/k8s';
// Then use:
kind={referenceForModel(VolumeAttributesClassModel)}Option 2: Construct the reference manually
kind="storage.k8s.io~v1~VolumeAttributesClass"Option 3: Check if the model has a reference property
kind={VolumeAttributesClassModel.reference}🤖 Prompt for AI Agents
In frontend/public/components/persistent-volume-claim.tsx around lines 472 to
499, the code uses referenceFor(VolumeAttributesClassModel) which builds an
incorrect core/v1 reference; replace it with
referenceForModel(VolumeAttributesClassModel) (or alternatively use
VolumeAttributesClassModel.reference) and add the corresponding import from
'@console/internal/module/k8s' so the ResourceLink resolves to
storage.k8s.io/v1/VolumeAttributesClass.
203343e to
922d71e
Compare
cajieh
force-pushed
the
improve-vac-field-pvc-details-page
branch
from
922d71e to
eacb9a5
Compare
|
lgtm flow wise. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cajieh, jhadvig 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 |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@cajieh: This pull request references Jira Issue OCPBUGS-68332, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
|
/test images |
|
/verified later @yapei |
|
/retest |
openshift-ci-robot
added
verified-later
verified
|
@cajieh: This PR has been marked to be verified later by 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. |
| "StorageClass": "StorageClass", | ||
| "Total": "Total", | ||
| "VolumeAttributesClass modification failed": "VolumeAttributesClass modification failed", | ||
| "Failed to modify VolumeAttributesClass for this PersistentVolumeClaim.": "Failed to modify VolumeAttributesClass for this PersistentVolumeClaim.", |
There was a problem hiding this comment.
For the failed alert:
"VolumeAttributesClass modification failed
Your volume settings could not be updated. Check for typos, ensure the parameters are supported by your StorageClass, and try again."
What do you think of this? Wanted to give the user a next step and make it a little more user-friendly @kevinhatchoua @cajieh
There was a problem hiding this comment.
Holding due to review comments
/hold
| "Failed to modify VolumeAttributesClass for this PersistentVolumeClaim.": "Failed to modify VolumeAttributesClass for this PersistentVolumeClaim.", | ||
| "VolumeAttributesClass application pending": "VolumeAttributesClass application pending", | ||
| "VolumeAttributesClass modification in progress": "VolumeAttributesClass modification in progress", | ||
| "VolumeAttributesClass \"{{target}}\" is pending application.": "VolumeAttributesClass \"{{target}}\" is pending application.", |
There was a problem hiding this comment.
For the application pending alert:
"VolumeAttributesClass application in progress
{{target}} is being applied to replace {{current}}. Your volume remains available during this update." WYDT @kevinhatchoua @cajieh ?
openshift-ci
bot
added
the
do-not-merge/hold
|
@cajieh: The following test failed, say
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. |
6 participants
No VAC
VAC modification pending
VAC modification error
1.
2.
VAC modification succeed