allow for pvc to be added to multiple workbenches #3231

Gkrumbach07 · 2024-09-19T19:25:51Z

https://issues.redhat.com/browse/RHOAIENG-12803

Description

This PR implements the functionality to allow a PVC to be added to multiple workbenches. The changes include:

Updating the UI to allow selection of multiple workbenches when adding or editing a PVC.
Modifying the logic to handle attaching a PVC to multiple workbenches.

I also updated some storage class logic and manifests

Untitled.mov

How Has This Been Tested?

create two workbenches
- one with its own new pvc
- the second one should be able to use the previous pvc
go to cluster storage tab
create a new pvc and connect one workbench
save and re edit pvc and add another workbench

Test Impact

Cypress tests for adding a new PVC and attaching it to multiple workbenches

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

codecov · 2024-09-19T20:11:54Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.91%. Comparing base (853029b) to head (101332c).
Report is 11 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3231      +/-   ##
==========================================
- Coverage   85.50%   84.91%   -0.59%     
==========================================
  Files        1280     1287       +7     
  Lines       28229    28525     +296     
  Branches     7558     7638      +80     
==========================================
+ Hits        24136    24221      +85     
- Misses       4093     4304     +211

Files with missing lines	Coverage Δ
...pages/projects/notebook/ConnectedNotebookNames.tsx	`94.11% <100.00%> (ø)`
...s/projects/notebook/StorageNotebookConnections.tsx	`88.46% <100.00%> (+1.50%)`	⬆️
...ects/screens/detail/storage/ManageStorageModal.tsx	`86.90% <100.00%> (+4.14%)`	⬆️
...src/pages/projects/screens/spawner/SpawnerPage.tsx	`98.07% <ø> (ø)`
...src/pages/projects/screens/spawner/spawnerUtils.ts	`81.30% <100.00%> (ø)`
...creens/spawner/storage/AddExistingStorageField.tsx	`92.00% <100.00%> (-0.86%)`	⬇️
...cts/screens/spawner/storage/StorageClassSelect.tsx	`90.00% <ø> (ø)`
.../projects/screens/spawner/storage/StorageField.tsx	`100.00% <100.00%> (ø)`
...rc/pages/projects/screens/spawner/storage/utils.ts	`100.00% <ø> (ø)`

... and 29 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 853029b...101332c. Read the comment docs.

Gkrumbach07 · 2024-09-19T20:26:09Z

/retest

fix: Fix issue with adding cluster storage in tests fix roles added test

andrewballantyne

Looks good to me -- I have not tested it nor reviewed all the code.

One comment about permissions.

andrewballantyne · 2024-09-20T14:50:00Z

manifests/core-bases/base/cluster-role.yaml

+      - get
+      - list


Get & List should not be needed -- everyone on the cluster should be able to get/list them, right?

can they? i will have to test this next week.

andrewballantyne

You have a bug where you're stopping them from any connection path that starts with a prexisting connection path

PVC1 - /data
PVC2 - /data2 ERROR << This should not error

DaoDaoNoCode · 2024-09-20T17:23:00Z

frontend/src/pages/projects/notebook/ConnectedNotebookNames.tsx

@@ -32,13 +32,13 @@ const ConnectedNotebookNames: React.FC<ConnectedNotebookNamesProps> = ({
  }

  return (
-    <List isPlain>
+    <LabelGroup isCompact>


I tested on the PF website, not sure if the doc is outdated, but I am pretty sure there is no need to add isCompact on LabelGroup. There is no hurt to add it though, just a small nit.

DaoDaoNoCode

Check the code and tested, other than the comment I left above it lgtm.
~~/lgtm~~
EDIT: Sorry I missed the review from Andrew... After that bug is addressed I will add the label back.

DaoDaoNoCode · 2024-09-20T18:05:28Z

@andrewballantyne Should the bug you mentioned above be fixed in my ticket? RHOAIENG-7680 It's all about the validation of the storage path, I will post a PR later this afternoon.

DaoDaoNoCode · 2024-09-20T19:20:36Z

/lgtm
As long as #3231 (comment) and #3231 (comment) are addressed next week, this PR is ready to go.
The bug mentioned in #3231 (review) will be fixed in #3236.

DaoDaoNoCode

/lgtm

Gkrumbach07 · 2024-09-23T16:03:54Z

/approve

openshift-ci · 2024-09-23T16:04:01Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

~~OWNERS~~ [Gkrumbach07]

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

* allow for pvc to be added to multiple workbenches (#3231) * allow for pvc to be added to multiple workbenches fix: Fix issue with adding cluster storage in tests fix roles added test * fix lint * fix removabale notebooks logic * fixed perms and iscompact * v2.26.2-odh-release Signed-off-by: Mike Turley <[email protected]> --------- Signed-off-by: Mike Turley <[email protected]> Co-authored-by: Gage Krumbach <[email protected]>

openshift-ci bot requested review from lucferbux and pnaik1 September 19, 2024 19:27

Gkrumbach07 added 2 commits September 20, 2024 09:25

allow for pvc to be added to multiple workbenches

bc8cb52

fix: Fix issue with adding cluster storage in tests fix roles added test

fix lint

9ac1d95

Gkrumbach07 force-pushed the story/RHOAIENG-12803-allow-pvcs-to-be-added-to-multiple-workbenches branch from 46ca23c to 9ac1d95 Compare September 20, 2024 14:25

fix removabale notebooks logic

64b8f5c

andrewballantyne reviewed Sep 20, 2024

View reviewed changes

DaoDaoNoCode reviewed Sep 20, 2024

View reviewed changes

DaoDaoNoCode approved these changes Sep 20, 2024

View reviewed changes

openshift-ci bot assigned DaoDaoNoCode Sep 20, 2024

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

DaoDaoNoCode removed the lgtm label Sep 20, 2024

DaoDaoNoCode mentioned this pull request Sep 20, 2024

Fix validation in the storage modal mount path #3236

Merged

6 tasks

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

fixed perms and iscompact

101332c

openshift-ci bot removed the lgtm label Sep 23, 2024

DaoDaoNoCode approved these changes Sep 23, 2024

View reviewed changes

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

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

openshift-merge-bot bot merged commit 84628b2 into opendatahub-io:main Sep 23, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow for pvc to be added to multiple workbenches #3231

allow for pvc to be added to multiple workbenches #3231

Gkrumbach07 commented Sep 19, 2024 •

edited

Loading

codecov bot commented Sep 19, 2024 •

edited

Loading

Gkrumbach07 commented Sep 19, 2024

andrewballantyne left a comment

andrewballantyne Sep 20, 2024

Gkrumbach07 Sep 20, 2024

Gkrumbach07 Sep 23, 2024

andrewballantyne left a comment

DaoDaoNoCode Sep 20, 2024

DaoDaoNoCode left a comment •

edited

Loading

DaoDaoNoCode commented Sep 20, 2024

DaoDaoNoCode commented Sep 20, 2024

DaoDaoNoCode left a comment

Gkrumbach07 commented Sep 23, 2024

openshift-ci bot commented Sep 23, 2024

allow for pvc to be added to multiple workbenches #3231

allow for pvc to be added to multiple workbenches #3231

Conversation

Gkrumbach07 commented Sep 19, 2024 • edited Loading

Description

How Has This Been Tested?

Test Impact

Request review criteria:

codecov bot commented Sep 19, 2024 • edited Loading

Codecov Report

Gkrumbach07 commented Sep 19, 2024

andrewballantyne left a comment

Choose a reason for hiding this comment

andrewballantyne Sep 20, 2024

Choose a reason for hiding this comment

Gkrumbach07 Sep 20, 2024

Choose a reason for hiding this comment

Gkrumbach07 Sep 23, 2024

Choose a reason for hiding this comment

andrewballantyne left a comment

Choose a reason for hiding this comment

DaoDaoNoCode Sep 20, 2024

Choose a reason for hiding this comment

DaoDaoNoCode left a comment • edited Loading

Choose a reason for hiding this comment

DaoDaoNoCode commented Sep 20, 2024

DaoDaoNoCode commented Sep 20, 2024

DaoDaoNoCode left a comment

Choose a reason for hiding this comment

Gkrumbach07 commented Sep 23, 2024

openshift-ci bot commented Sep 23, 2024

Gkrumbach07 commented Sep 19, 2024 •

edited

Loading

codecov bot commented Sep 19, 2024 •

edited

Loading

DaoDaoNoCode left a comment •

edited

Loading