[RHOAIENG-1101] Cannot remove additional storage from the workbench configuration #3320
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
jpuzz0
force-pushed
the
RHOAIENG-1101
branch
4 times, most recently
from
8e91e1d
to
86f0874
Compare
jpuzz0
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
jpuzz0
force-pushed
the
RHOAIENG-1101
branch
4 times, most recently
from
deb1bc6
to
37e41c9
Compare
|
Thanks @jpuzz0 ! Overall LGTM! |
jpuzz0
force-pushed
the
RHOAIENG-1101
branch
2 times, most recently
from
7ca7481
to
9d1c493
Compare
@xianli123 I've made this change. Thanks for calling it out! |
|
Thanks @jpuzz0 . Once it's changed, it will look good! |
| { | ||
| storageType: existingNotebook ? StorageType.EXISTING_PVC : StorageType.NEW_PVC, | ||
| creating: { | ||
| nameDesc: { | ||
| name: k8sNameDescriptionData.data.name || defaultClusterStorage.name, | ||
| description: defaultClusterStorage.description, | ||
| }, | ||
| size: defaultClusterStorage.size || defaultNotebookSize, | ||
| storageClassName: defaultStorageClassName || defaultClusterStorage.storageClassName, | ||
| mountPath: defaultClusterStorage.mountPath, | ||
| }, | ||
| existing: { | ||
| storage: getRootVolumeName(existingNotebook), | ||
| }, | ||
| }, |
There was a problem hiding this comment.
so this exisitng data should include all attached PVCs. create some hook that is basically the inverse of useRelatedNotebooks called useRelatedPVCs
There was a problem hiding this comment.
this may also have to be restructured then because existing would need to be able to update the mount path
my suggestions is something along the lines of
{
storageType: existingNotebook ? StorageType.EXISTING_PVC : StorageType.NEW_PVC,
name: getRootVolumeName(existingNotebook),
nameDesc: {
name: existingName || k8sNameDescriptionData.data.name || defaultClusterStorage.name,
description: existingDescription || defaultClusterStorage.description,
},
size: existingSize || defaultClusterStorage.size || defaultNotebookSize,
storageClassName: existingStorageClassName || defaultStorageClassName || defaultClusterStorage.storageClassName,
mountPath: existingMountPath || defaultClusterStorage.mountPath,
}
| const newStorageFieldInvalid = storageType === StorageType.NEW_PVC && !creating.nameDesc.name; | ||
| const existingStorageFieldInvalid = storageType === StorageType.EXISTING_PVC && !existing.storage; | ||
| const isStorageDataValid = !newStorageFieldInvalid && !existingStorageFieldInvalid; | ||
| const isStorageDataValid = storageData.length > 0; |
There was a problem hiding this comment.
storage data is not required so this is not needed
| @@ -0,0 +1,131 @@ | |||
| import React from 'react'; | |||
There was a problem hiding this comment.
this file is just the placeholder until dipanshu merges right?
There was a problem hiding this comment.
Yes, I think he can use it and expand upon it though.
| { | ||
| storageType: existingNotebook ? StorageType.EXISTING_PVC : StorageType.NEW_PVC, | ||
| creating: { | ||
| nameDesc: { | ||
| name: k8sNameDescriptionData.data.name || defaultClusterStorage.name, | ||
| description: defaultClusterStorage.description, | ||
| }, | ||
| size: defaultClusterStorage.size || defaultNotebookSize, | ||
| storageClassName: defaultStorageClassName || defaultClusterStorage.storageClassName, | ||
| mountPath: defaultClusterStorage.mountPath, | ||
| }, | ||
| existing: { | ||
| storage: getRootVolumeName(existingNotebook), | ||
| }, | ||
| }, |
There was a problem hiding this comment.
this may also have to be restructured then because existing would need to be able to update the mount path
my suggestions is something along the lines of
{
storageType: existingNotebook ? StorageType.EXISTING_PVC : StorageType.NEW_PVC,
name: getRootVolumeName(existingNotebook),
nameDesc: {
name: existingName || k8sNameDescriptionData.data.name || defaultClusterStorage.name,
description: existingDescription || defaultClusterStorage.description,
},
size: existingSize || defaultClusterStorage.size || defaultNotebookSize,
storageClassName: existingStorageClassName || defaultStorageClassName || defaultClusterStorage.storageClassName,
mountPath: existingMountPath || defaultClusterStorage.mountPath,
}
https://issues.redhat.com/browse/RHOAIENG-1101
Description
Workbench create/edit now has a table view to allow for multiple PVC instances to be created, where currently only 1 row is creatable/editable. The buttons above the table are not actionable yet and are placeholders for now. Some logic has been updated as a part of the submission process to attach multiple volumes (from PVC creations) for the notebook when available.
Default table data on creation
Detach modal
Empty state when all are detached
Edit modal on creation
(cc @xianli123)
How Has This Been Tested?
When creating a workbench, verify the set of fields that were previously visible are now replaced with the editable table view. The Edit modal itself is a part of a different story, but one was made here as a starting point, where mainly the mount path and some show/hide of alerts will change (along with some implementation).
Test Impact
Updated mocked cypress tests to simply verify table data for now as a part of creating/editing a workbench. Since this feature has other parts, e2e mock testing will be dependent on their completion.
Request review criteria:
Self checklist (all need to be checked):
If you have UI changes:
After the PR is posted & before it merges:
main