Skip to content

Fix for empty device class #2431

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
Harshvardhan-handa123 wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix for empty device class #2431

Harshvardhan-handa123 wants to merge 1 commit into from

Conversation

@Harshvardhan-handa123
Copy link
Contributor

@Harshvardhan-handa123 Harshvardhan-handa123 commented Dec 11, 2025 •
edited
Loading

@Harshvardhan-handa123 Harshvardhan-handa123 force-pushed the default-device-class-in-storageclusters branch 2 times, most recently from 05208c0 to c0d4b97 Compare December 14, 2025 06:18
@Harshvardhan-handa123
Copy link
Contributor Author

Harshvardhan-handa123 commented Dec 14, 2025

/retest

SanjalKatiyar
SanjalKatiyar requested changes Dec 15, 2025
View reviewed changes
Copy link
Collaborator

@SanjalKatiyar SanjalKatiyar left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. In yup validation:
const isDuplicate = existingDeviceClasses.toLowerCase().includes(value?.toLowerCase());

  1. As discussed, plz remove footerContent: t('Example: ssd'), from the popover.

  2. Plz test these new changes.

packages/odf/components/attach-storage-storagesystem/device-class-form.tsx Outdated
state.lsoStorageClassName
)
.map((ds) => ds.deviceClass);
.map((ds) => ds.deviceClass || 'ssd');
Copy link
Collaborator

@SanjalKatiyar SanjalKatiyar Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
.map((ds) => ds.deviceClass || 'ssd');
.map((ds) => ds.deviceClass || DEFAULT_DEVICECLASS);

Copy link
Contributor Author

@Harshvardhan-handa123 Harshvardhan-handa123 Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

packages/odf/utils/ocs.ts Outdated

const getPVStorageClass = (pv) => pv?.spec?.storageClassName;

const SSD = DiskType.SSD;
Copy link
Collaborator

@SanjalKatiyar SanjalKatiyar Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wrong import...

Suggested change
const SSD = DiskType.SSD;

Copy link
Collaborator

@SanjalKatiyar SanjalKatiyar Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use DEFAULT_DEVICECLASS from shared/constants

Copy link
Contributor Author

@Harshvardhan-handa123 Harshvardhan-handa123 Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

packages/odf/utils/ocs.ts Outdated
},
},
},
deviceClass: SSD,
Copy link
Collaborator

@SanjalKatiyar SanjalKatiyar Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
deviceClass: SSD,
deviceClass: DEFAULT_DEVICECLASS,

Copy link
Contributor Author

@Harshvardhan-handa123 Harshvardhan-handa123 Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

packages/odf/modals/add-capacity/add-capacity-modal.tsx Outdated
Comment on lines 297 to 299
const deviceSets = React.useMemo(() => {
return storageCluster?.spec?.storageDeviceSets || [];
}, [storageCluster]);
Copy link
Collaborator

@SanjalKatiyar SanjalKatiyar Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need for useMemo...

Copy link
Contributor Author

@Harshvardhan-handa123 Harshvardhan-handa123 Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

packages/odf/modals/add-capacity/add-capacity-modal.tsx Outdated
Comment on lines 303 to 305
const selectedSCName: string = React.useMemo(() => {
return getName(storageClass);
}, [storageClass]);
Copy link
Collaborator

@SanjalKatiyar SanjalKatiyar Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need for useMemo...

Copy link
Contributor Author

@Harshvardhan-handa123 Harshvardhan-handa123 Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

packages/odf/modals/add-capacity/add-capacity-modal.tsx
hasMultiDeviceClasses ? deviceClass : null
);
React.useEffect(() => {
if (deviceClasses.length === 1) {
Copy link
Collaborator

@SanjalKatiyar SanjalKatiyar Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (deviceClasses.length === 1) {
if (deviceClasses.length === 1 && !deviceClass) {

Copy link
Contributor Author

@Harshvardhan-handa123 Harshvardhan-handa123 Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

packages/odf/modals/add-capacity/add-capacity-modal.tsx Outdated
if (deviceClasses.length === 1) {
setDeviceClass(deviceClasses[0]);
}
}, [deviceClasses]);
Copy link
Collaborator

@SanjalKatiyar SanjalKatiyar Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
}, [deviceClasses]);
}, [deviceClasses, setDeviceClass]);

Copy link
Contributor Author

@Harshvardhan-handa123 Harshvardhan-handa123 Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

packages/odf/modals/add-capacity/add-capacity-modal.tsx Outdated
);
React.useEffect(() => {
if (deviceClasses.length === 1) {
setDeviceClass(deviceClasses[0]);
Copy link
Collaborator

@SanjalKatiyar SanjalKatiyar Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
setDeviceClass(deviceClasses[0]);
setDeviceClass(deviceClasses[0] || noDeviceClassKey);

Copy link
Contributor Author

@Harshvardhan-handa123 Harshvardhan-handa123 Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

packages/odf/utils/ocs.ts Outdated
Comment on lines 231 to 233
!!deviceClass && deviceClass !== 'None'
? ds.deviceClass === deviceClass
: !ds.deviceClass;
Copy link
Collaborator

@SanjalKatiyar SanjalKatiyar Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
!!deviceClass && deviceClass !== 'None'
? ds.deviceClass === deviceClass
: !ds.deviceClass;
deviceClass === noDeviceClassKey
? !ds.deviceClass
: ds.deviceClass === deviceClass;

Copy link
Contributor Author

@Harshvardhan-handa123 Harshvardhan-handa123 Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

packages/odf/utils/ocs.ts Outdated
? ds.deviceClass === deviceClass
: true;

/* if the deviceClass is an empty string or a "None", we need match it with an index having empty string. */
Copy link
Collaborator

@SanjalKatiyar SanjalKatiyar Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/* if the deviceClass is an empty string or a "None", we need match it with an index having empty string. */
/* if the deviceClass is noDeviceClassKey, we need match it with an index having empty string. */

Copy link
Contributor Author

@Harshvardhan-handa123 Harshvardhan-handa123 Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@SanjalKatiyar
Copy link
Collaborator

SanjalKatiyar commented Dec 15, 2025

plz shorten the commit and PR title.

@Harshvardhan-handa123 Harshvardhan-handa123 force-pushed the default-device-class-in-storageclusters branch from c0d4b97 to 2efb5b1 Compare December 15, 2025 11:40
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 15, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Harshvardhan-handa123
Once this PR has been reviewed and has the lgtm label, please ask for approval from sanjalkatiyar. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@Harshvardhan-handa123 Harshvardhan-handa123 force-pushed the default-device-class-in-storageclusters branch from 2efb5b1 to 05c01d8 Compare December 15, 2025 13:35
@Harshvardhan-handa123 Harshvardhan-handa123 changed the title Fix for the case where there is an empty deviceClass and sending deaf… Fix for empty device class Dec 15, 2025
@Harshvardhan-handa123 Harshvardhan-handa123 force-pushed the default-device-class-in-storageclusters branch from 05c01d8 to 445b8cb Compare December 15, 2025 14:02
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 15, 2025

@Harshvardhan-handa123: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/images 445b8cb link true /test images
ci/prow/odf-console-e2e-aws 445b8cb link true /test odf-console-e2e-aws

Full PR test history. Your PR dashboard.

Details

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. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@SanjalKatiyar SanjalKatiyar SanjalKatiyar requested changes

Assignees

@SanjalKatiyar SanjalKatiyar

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Harshvardhan-handa123 @SanjalKatiyar