Storage class dropdown select #3212

Gkrumbach07 · 2024-09-13T20:11:13Z

Description

This is a continuation of @dpanshug PR

added default selection
added default label
corrected autofill for default
added alerts for select

Added new field to select storage class to create pvc.

How Has This Been Tested?

Create a new cluster storage and you check list of storage classes in the field.
Create new workbench and select storage class in cluster storage.

Test Impact

added test for default being set

Request review criteria:

Self checklist (all need to be checked):

The developer has manually tested the changes and verified that the changes work
Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
The developer has added tests or explained why testing cannot be added (unit or cypress tests for related changes)

If you have UI changes:

Included any necessary screenshots or gifs if it was a UI change.
Included tags to the UX team if it was a UI/UX change.

After the PR is posted & before it merges:

The developer has tested their solution on a cluster by using the image produced by the PR to main
@xianli123

codecov · 2024-09-13T20:54:16Z

Codecov Report

Attention: Patch coverage is 89.69072% with 10 lines in your changes missing coverage. Please review.

Project coverage is 85.43%. Comparing base (7964daa) to head (8fcdf1a).
Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
...ects/screens/detail/storage/ManageStorageModal.tsx	76.47%	4 Missing ⚠️
...cts/screens/spawner/storage/StorageClassSelect.tsx	90.00%	4 Missing ⚠️
...src/pages/projects/screens/spawner/SpawnerPage.tsx	83.33%	1 Missing ⚠️
.../screens/spawner/storage/useDefaultStorageClass.ts	93.33%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3212      +/-   ##
==========================================
+ Coverage   85.34%   85.43%   +0.08%     
==========================================
  Files        1275     1277       +2     
  Lines       28010    28080      +70     
  Branches     7449     7487      +38     
==========================================
+ Hits        23905    23990      +85     
+ Misses       4105     4090      -15

Files with missing lines	Coverage Δ
frontend/src/api/k8s/pvcs.ts	`100.00% <100.00%> (ø)`
...c/pages/projects/screens/spawner/SpawnerFooter.tsx	`77.31% <100.00%> (-0.56%)`	⬇️
...tend/src/pages/projects/screens/spawner/service.ts	`68.88% <100.00%> (ø)`
...src/pages/projects/screens/spawner/spawnerUtils.ts	`81.30% <100.00%> (+0.08%)`	⬆️
...creens/spawner/storage/CreateNewStorageSection.tsx	`100.00% <100.00%> (ø)`
...creens/spawner/storage/usePreferredStorageClass.ts	`77.77% <100.00%> (+11.11%)`	⬆️
...rc/pages/projects/screens/spawner/storage/utils.ts	`100.00% <100.00%> (ø)`
frontend/src/pages/projects/types.ts	`100.00% <ø> (ø)`
...src/pages/projects/screens/spawner/SpawnerPage.tsx	`98.07% <83.33%> (-1.93%)`	⬇️
.../screens/spawner/storage/useDefaultStorageClass.ts	`93.33% <93.33%> (ø)`
... and 2 more

... and 23 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7964daa...8fcdf1a. Read the comment docs.

xianli123 · 2024-09-14T01:58:03Z

Thanks @Gkrumbach07 @dpanshug .
Is it feasible to show the storage class in the dropdown (as shown in the screenshot below) when the class is deprecated? I also attached the second screenshot as a reference.

We might show 2 states in this case:

When the class is disabled in RHOAI, we can show the class name and disable the dropdown.
When the class is deleted from OpenShift, we can make it as what the 1st screenshot shows.
Does it make sense?

[1st screenshot]

[2nd screenshot]

frontend/src/__tests__/cypress/cypress/pages/storageClasses.ts

frontend/src/pages/storageClasses/utils.ts

frontend/src/pages/projects/screens/spawner/storage/utils.ts

frontend/src/pages/projects/screens/spawner/storage/StorageClassSelect.tsx

jpuzz0 · 2024-09-16T14:07:12Z

frontend/src/pages/projects/screens/spawner/storage/StorageClassSelect.tsx

+          <SplitItem>
+            {config?.isDefault && (
+              <Label isCompact color="green">
+                Default class


@xianli123 I'm noticing this reads Default class here for a Select that has a label giving context of Storage class, and in the storage classes table, a row with the "default" openshift label only reads Default. Should they not be the same?

Hi @jpuzz0.They are different.

In the class table, the default label next to the class means that this class is the default class in OpenShift.

The Default class here for a 'Select' in storage creation form means that this class is the default class in RHOAI.
Initially, RHOAI uses Openshift's default class as its default class. If the RHOAI admin chooses another enabled class as the RHOAI default class, the new RHOAI default class will be used when the new storage is created.
Is it clear?

Yes, thanks for clarifying! Should this label have a tooltip letting the user know this is the RHOAI default and not the Openshift default?

frontend/src/pages/projects/screens/spawner/storage/CreateNewStorageSection.tsx

jpuzz0 · 2024-09-16T14:30:02Z

Looks like when I try clicking on a storage class name in the dropdown it is not set in the form:

Screen.Recording.2024-09-16.at.10.27.19.AM.mov

Also, it appears even though the field is marked as required, I'm able to submit the workbench create form without a value specified.

Gkrumbach07 · 2024-09-16T17:16:36Z

@xianli123 I fixed it so now deprecated also shows the name

…tVolumesForNotebook function fix tests fix disabled checks pr fixes to state added tests linter fix

jpuzz0 · 2024-09-17T04:14:30Z

I think this might be incorrectly showing a warning message in this form where nothing has been selected for the storage class dropdown.

jpuzz0 · 2024-09-17T14:27:10Z

I think this might be incorrectly showing a warning message in this form where nothing has been selected for the storage class dropdown.

I'm still seeing this after the recent commit meant to fix the issue:

jpuzz0 · 2024-09-17T14:34:18Z

frontend/src/pages/projects/screens/spawner/storage/StorageClassSelect.tsx

+        popperProps={{ appendTo: menuAppendTo }}
+      />
+      <FormHelperText>
+        {selectedStorageClassConfig && !selectedStorageClassConfig.isEnabled ? (


If a storage class config is not enabled, its considered deprecated? Are we sure this is the condition meant to use to indicate when a storage class is deprecated?

not enabled is deprecated, thats what was in the mocks

I don't consider this blocking since this is how the mocks describe it, but @xianli123 Is "Deprecated" the appropriate term to use here? Can't the user just turn around and enable the storage class from the storage classes table? I guess my question is why not just consider it "Disabled"?

lets merge it now bc this pr blocks purva's, and we can circle back to it

Hi @Gkrumbach07 @jpuzz0 Let me explain how to get the 'deprecated' class:

RHOAI admin enabled a class e.g. 'Class1'.

Users used the 'Class1' to create storage e.g. 'Storage1'.

RHOAI admin disabled 'Class1'.

Users will see 'Class1' is deprecated when checking 'Storage1'.

In a word, if the class is never enabled, it should not be a deprecated class, because users cannot select it. If the class has been enabled and used by some storage, after disabling the class, the class will be marked as 'Deprecated' in the storage that is using this newly disabled class.
Does it make sense?

@xianli123 Thanks for explaining!

@Gkrumbach07 Are we checking to make sure the user created storage using the enabled class? Seems that is a prerequisite on top of the disabled state to mark the class as deprecated.

Gkrumbach07 · 2024-09-17T15:50:28Z

@jpuzz0 I found the issue. when no default is set, i default select the first class, but that class was disabled, so it updated it so now i pick from only enabled classes

Gkrumbach07 · 2024-09-17T19:50:24Z

/approve

openshift-ci · 2024-09-17T19:50:30Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Gkrumbach07, jpuzz0

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~frontend/OWNERS~~ [Gkrumbach07]

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

openshift-ci bot requested review from mturley and ppadti September 13, 2024 20:11

Gkrumbach07 mentioned this pull request Sep 13, 2024

[WIP] RHOAIENG-1109 Select Storage Classes in add cluster storage sections #3184

Closed

6 tasks

Gkrumbach07 force-pushed the storage-class branch from 0cffd35 to a490b7d Compare September 13, 2024 20:13

jpuzz0 reviewed Sep 16, 2024

View reviewed changes

dpanshug and others added 2 commits September 16, 2024 14:54

RHOAIENG-1109 Select Storage Classes in add cluster storage sections

8f9c90d

refactor: Remove unnecessary storageClassName parameter in replaceRoo…

e5497f4

…tVolumesForNotebook function fix tests fix disabled checks pr fixes to state added tests linter fix

Gkrumbach07 force-pushed the storage-class branch from 6afb03b to e5497f4 Compare September 16, 2024 19:55

pnaik1 mentioned this pull request Sep 17, 2024

Display storage class selected in workbenches #3214

Merged

6 tasks

fix alert on empty

52f6775

jpuzz0 reviewed Sep 17, 2024

View reviewed changes

fixed storage class default

8fcdf1a

jpuzz0 approved these changes Sep 17, 2024

View reviewed changes

openshift-ci bot assigned jpuzz0 Sep 17, 2024

openshift-ci bot added the lgtm label Sep 17, 2024

openshift-ci bot added the approved label Sep 17, 2024

openshift-merge-bot bot merged commit b5351a7 into opendatahub-io:main Sep 17, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Storage class dropdown select #3212

Storage class dropdown select #3212

Gkrumbach07 commented Sep 13, 2024

codecov bot commented Sep 13, 2024 •

edited

Loading

xianli123 commented Sep 14, 2024 •

edited

Loading

jpuzz0 Sep 16, 2024

xianli123 Sep 18, 2024

jpuzz0 Sep 18, 2024

jpuzz0 commented Sep 16, 2024

Gkrumbach07 commented Sep 16, 2024

jpuzz0 commented Sep 17, 2024

jpuzz0 commented Sep 17, 2024

jpuzz0 Sep 17, 2024

Gkrumbach07 Sep 17, 2024

jpuzz0 Sep 17, 2024

Gkrumbach07 Sep 17, 2024

xianli123 Sep 18, 2024

jpuzz0 Sep 18, 2024

Gkrumbach07 commented Sep 17, 2024

Gkrumbach07 commented Sep 17, 2024

openshift-ci bot commented Sep 17, 2024

Storage class dropdown select #3212

Storage class dropdown select #3212

Conversation

Gkrumbach07 commented Sep 13, 2024

Description

How Has This Been Tested?

Test Impact

Request review criteria:

codecov bot commented Sep 13, 2024 • edited Loading

Codecov Report

xianli123 commented Sep 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpuzz0 commented Sep 16, 2024

Gkrumbach07 commented Sep 16, 2024

jpuzz0 commented Sep 17, 2024

jpuzz0 commented Sep 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gkrumbach07 commented Sep 17, 2024

Gkrumbach07 commented Sep 17, 2024

openshift-ci bot commented Sep 17, 2024

codecov bot commented Sep 13, 2024 •

edited

Loading

xianli123 commented Sep 14, 2024 •

edited

Loading