[RHOAIENG-1101] Cannot remove additional storage from the workbench c…

…onfiguration
opendatahub-io · Oct 17, 2024 · ea21b77 · ea21b77
1 parent 4d29174
commit ea21b77
Show file tree

Hide file tree

Showing 22 changed files with 603 additions and 337 deletions.
diff --git a/frontend/src/__mocks__/mockStartNotebookData.ts b/frontend/src/__mocks__/mockStartNotebookData.ts
@@ -94,18 +94,15 @@ export const mockStartNotebookData = ({
   },
 });
 
-export const mockStorageData: StorageData = {
-  storageType: StorageType.NEW_PVC,
-  creating: {
-    nameDesc: {
-      name: 'test-pvc',
-      description: '',
-    },
+export const mockStorageData: StorageData[] = [
+  {
+    storageType: StorageType.NEW_PVC,
+    name: 'test-pvc',
+    description: '',
     size: '20Gi',
     storageClassName: 'gp2-csi',
   },
-  existing: { storage: '' },
-};
+];
 
 export const mockDataConnectionData: DataConnectionData = {
   type: 'creating',

diff --git a/frontend/src/__tests__/cypress/cypress/pages/workbench.ts b/frontend/src/__tests__/cypress/cypress/pages/workbench.ts
@@ -190,6 +190,39 @@ class AttachConnectionModal extends Modal {
   }
 }
 
+class StorageTableRow extends TableRow {
+  private findByDataLabel(label: string) {
+    return this.find().find(`[data-label="${label}"]`);
+  }
+
+  findNameValue() {
+    return this.findByDataLabel('Name');
+  }
+
+  findStorageSizeValue() {
+    return this.findByDataLabel('Storage size');
+  }
+
+  findMountPathValue() {
+    return this.findByDataLabel('Mount path');
+  }
+}
+
+class StorageTable {
+  find() {
+    return cy.findByTestId('cluster-storage-table');
+  }
+
+  getRowById(id: number) {
+    return new StorageTableRow(
+      () =>
+        this.find().findByTestId(['cluster-storage-table-row', id]) as unknown as Cypress.Chainable<
+          JQuery<HTMLTableRowElement>
+        >,
+    );
+  }
+}
+
 class CreateSpawnerPage {
   k8sNameDescription = new K8sNameDescriptionField('workbench');
 
@@ -198,16 +231,8 @@ class CreateSpawnerPage {
     return this;
   }
 
-  findNewStorageRadio() {
-    return cy.findByTestId('persistent-new-storage-type-radio');
-  }
-
-  findExistingStorageRadio() {
-    return cy.findByTestId('persistent-existing-storage-type-radio');
-  }
-
-  findClusterStorageInput() {
-    return cy.findByTestId('create-new-storage-name');
+  getStorageTable() {
+    return new StorageTable();
   }
 
   private findPVSizeField() {
@@ -351,11 +376,6 @@ class EditSpawnerPage extends CreateSpawnerPage {
     cy.testA11y();
   }
 
-  shouldHavePersistentStorage(name: string) {
-    cy.findByTestId('persistent-storage-group').find('input').should('have.value', name);
-    return this;
-  }
-
   shouldHaveNotebookImageSelectInput(name: string) {
     cy.findByTestId('workbench-image-stream-selection').contains(name).should('exist');
     return this;

diff --git a/frontend/src/__tests__/cypress/cypress/tests/mocked/projects/workbench.cy.ts b/frontend/src/__tests__/cypress/cypress/tests/mocked/projects/workbench.cy.ts
@@ -275,22 +275,11 @@ describe('Workbench page', () => {
     environmentVariableField.uploadConfigYaml(configYamlPath);
     environmentVariableField.findRemoveEnvironmentVariableButton().should('be.enabled');
 
-    //cluster storage
-    createSpawnerPage.shouldHaveClusterStorageAlert();
-
-    //add new cluster storage
-    createSpawnerPage.findNewStorageRadio().click();
-    createSpawnerPage.findClusterStorageInput().should('have.value', 'test-project');
-    createSpawnerPage.findClusterStorageDescriptionInput().fill('test-description');
-    createSpawnerPage.findPVSizeMinusButton().click();
-    createSpawnerPage.findPVSizeInput().should('have.value', '19');
-    createSpawnerPage.findPVSizePlusButton().click();
-    createSpawnerPage.findPVSizeInput().should('have.value', '20');
-    createSpawnerPage.selectPVSize('Mi');
-
-    //add existing cluster storage
-    createSpawnerPage.findExistingStorageRadio().click();
-    createSpawnerPage.selectExistingPersistentStorage('Test Storage');
+    // cluster storage
+    const storageTableRow = createSpawnerPage.getStorageTable().getRowById(0);
+    storageTableRow.findNameValue().should('have.text', 'test-project-storage');
+    storageTableRow.findStorageSizeValue().should('have.text', 'Max 20Gi');
+    storageTableRow.findMountPathValue().should('have.text', '/opt/apt-root/src/');
 
     //add data connection
     createSpawnerPage.findDataConnectionCheckbox().check();
@@ -327,15 +316,6 @@ describe('Workbench page', () => {
           name: 'test-project',
           namespace: 'test-project',
         },
-        spec: {
-          template: {
-            spec: {
-              volumes: [
-                { name: 'test-storage-1', persistentVolumeClaim: { claimName: 'test-storage-1' } },
-              ],
-            },
-          },
-        },
       });
     });
     verifyRelativeURL('/projects/test-project?section=workbenches');
@@ -635,7 +615,11 @@ describe('Workbench page', () => {
     editSpawnerPage.k8sNameDescription.findDisplayNameInput().should('have.value', 'Test Notebook');
     editSpawnerPage.shouldHaveNotebookImageSelectInput('Test Image');
     editSpawnerPage.shouldHaveContainerSizeInput('Small');
-    editSpawnerPage.shouldHavePersistentStorage('Test Storage');
+    editSpawnerPage
+      .getStorageTable()
+      .getRowById(0)
+      .findNameValue()
+      .should('have.text', 'test-notebook');
     editSpawnerPage.findSubmitButton().should('be.enabled');
     editSpawnerPage.k8sNameDescription.findDisplayNameInput().fill('Updated Notebook');
 

diff --git a/frontend/src/api/k8s/__tests__/pvcs.spec.ts b/frontend/src/api/k8s/__tests__/pvcs.spec.ts
@@ -10,7 +10,7 @@ import { mockPVCK8sResource } from '~/__mocks__/mockPVCK8sResource';
 import { assemblePvc, createPvc, deletePvc, getDashboardPvcs, updatePvc } from '~/api/k8s/pvcs';
 import { PVCModel } from '~/api/models/k8s';
 import { PersistentVolumeClaimKind } from '~/k8sTypes';
-import { CreatingStorageObject } from '~/pages/projects/types';
+import { StorageData } from '~/pages/projects/types';
 
 jest.mock('@openshift/dynamic-plugin-sdk-utils', () => ({
   k8sGetResource: jest.fn(),
@@ -26,11 +26,9 @@ const k8sCreateResourceMock = jest.mocked(k8sCreateResource<PersistentVolumeClai
 const k8sUpdateResourceMock = jest.mocked(k8sUpdateResource<PersistentVolumeClaimKind>);
 const k8sDeleteResourceMock = jest.mocked(k8sDeleteResource<PersistentVolumeClaimKind, K8sStatus>);
 
-const data: CreatingStorageObject = {
-  nameDesc: {
-    name: 'pvc',
-    description: 'Test Storage',
-  },
+const data: StorageData = {
+  name: 'pvc',
+  description: 'Test Storage',
   size: '5Gi',
 };
 

diff --git a/frontend/src/api/k8s/pvcs.ts b/frontend/src/api/k8s/pvcs.ts
@@ -11,20 +11,15 @@ import { PVCModel } from '~/api/models';
 import { translateDisplayNameForK8s } from '~/concepts/k8s/utils';
 import { LABEL_SELECTOR_DASHBOARD_RESOURCE } from '~/const';
 import { applyK8sAPIOptions } from '~/api/apiMergeUtils';
-import { CreatingStorageObject } from '~/pages/projects/types';
+import { StorageData } from '~/pages/projects/types';
 
 export const assemblePvc = (
-  data: CreatingStorageObject,
+  data: StorageData,
   namespace: string,
   editName?: string,
   hideFromUI?: boolean,
 ): PersistentVolumeClaimKind => {
-  const {
-    nameDesc: { name: pvcName, description },
-    size,
-    storageClassName,
-  } = data;
-
+  const { name: pvcName, description, size, storageClassName } = data;
   const name = editName || translateDisplayNameForK8s(pvcName);
 
   return {
@@ -40,7 +35,7 @@ export const assemblePvc = (
       }),
       annotations: {
         'openshift.io/display-name': pvcName.trim(),
-        'openshift.io/description': description,
+        ...(description && { 'openshift.io/description': description }),
       },
     },
     spec: {
@@ -69,7 +64,7 @@ export const getDashboardPvcs = (projectName: string): Promise<PersistentVolumeC
   });
 
 export const createPvc = (
-  data: CreatingStorageObject,
+  data: StorageData,
   namespace: string,
   opts?: K8sAPIOptions,
   hideFromUI?: boolean,
@@ -82,7 +77,7 @@ export const createPvc = (
 };
 
 export const updatePvc = (
-  data: CreatingStorageObject,
+  data: StorageData,
   existingData: PersistentVolumeClaimKind,
   namespace: string,
   opts?: K8sAPIOptions,

diff --git a/frontend/src/pages/modelServing/screens/projects/__tests__/utils.spec.ts b/frontend/src/pages/modelServing/screens/projects/__tests__/utils.spec.ts
@@ -426,10 +426,8 @@ describe('createNIMPVC', () => {
 
     expect(createPvc).toHaveBeenCalledWith(
       {
-        nameDesc: {
-          name: pvcName,
-          description: '',
-        },
+        name: pvcName,
+        description: '',
         size: pvcSize,
       },
       projectName,
@@ -445,10 +443,8 @@ describe('createNIMPVC', () => {
 
     expect(createPvc).toHaveBeenCalledWith(
       {
-        nameDesc: {
-          name: pvcName,
-          description: '',
-        },
+        name: pvcName,
+        description: '',
         size: pvcSize,
       },
       projectName,

diff --git a/frontend/src/pages/modelServing/screens/projects/utils.ts b/frontend/src/pages/modelServing/screens/projects/utils.ts
@@ -642,10 +642,8 @@ export const createNIMPVC = (
 ): Promise<PersistentVolumeClaimKind> =>
   createPvc(
     {
-      nameDesc: {
-        name: pvcName,
-        description: '',
-      },
+      name: pvcName,
+      description: '',
       size: pvcSize,
     },
     projectName,

diff --git a/frontend/src/pages/projects/pvc/useNotebookPVCItems.ts b/frontend/src/pages/projects/pvc/useNotebookPVCItems.ts
@@ -5,16 +5,16 @@ import { ProjectDetailsContext } from '~/pages/projects/ProjectDetailsContext';
 import { getNotebookPVCNames } from './utils';
 
 const useNotebookPVCItems = (
-  notebook: NotebookKind,
+  notebook: NotebookKind | undefined,
 ): [pvcs: PersistentVolumeClaimKind[], loaded: boolean, loadError?: Error] => {
   const {
     pvcs: { data: allPvcs, loaded, error },
   } = React.useContext(ProjectDetailsContext);
 
-  const pvcNames = useDeepCompareMemoize(getNotebookPVCNames(notebook));
+  const pvcNames = useDeepCompareMemoize(notebook && getNotebookPVCNames(notebook));
 
   const pvcs = React.useMemo(
-    () => allPvcs.filter((pvc) => pvcNames.includes(pvc.metadata.name)),
+    () => allPvcs.filter((pvc) => pvcNames?.includes(pvc.metadata.name)),
     [allPvcs, pvcNames],
   );
 

diff --git a/frontend/src/pages/projects/screens/detail/storage/ManageStorageModal.tsx b/frontend/src/pages/projects/screens/detail/storage/ManageStorageModal.tsx
@@ -87,6 +87,13 @@ const ManageStorageModal: React.FC<AddStorageModalProps> = ({ existingData, onCl
       forNotebook: { name: notebookName, mountPath },
     } = createData;
     const pvcPromises: Promise<PersistentVolumeClaimKind | NotebookKind>[] = [];
+    const storageData = {
+      name: createData.nameDesc.name,
+      description: createData.nameDesc.description,
+      size: createData.size,
+      storageClassName: createData.storageClassName,
+    };
+
     if (existingData) {
       const pvcName = existingData.metadata.name;
       if (
@@ -95,7 +102,7 @@ const ManageStorageModal: React.FC<AddStorageModalProps> = ({ existingData, onCl
         existingData.spec.resources.requests.storage !== createData.size ||
         existingData.spec.storageClassName !== createData.storageClassName
       ) {
-        pvcPromises.push(updatePvc(createData, existingData, namespace, { dryRun }));
+        pvcPromises.push(updatePvc(storageData, existingData, namespace, { dryRun }));
       }
       if (existingData.spec.resources.requests.storage !== createData.size) {
         connectedNotebooks.map((connectedNotebook) =>
@@ -119,7 +126,7 @@ const ManageStorageModal: React.FC<AddStorageModalProps> = ({ existingData, onCl
       }
       return;
     }
-    const createdPvc = await createPvc(createData, namespace, { dryRun });
+    const createdPvc = await createPvc(storageData, namespace, { dryRun });
     if (notebookName) {
       await attachNotebookPVC(notebookName, namespace, createdPvc.metadata.name, mountPath.value, {
         dryRun,