From 604a02cd99a5f401f983cf23aa7cd64f71779f47 Mon Sep 17 00:00:00 2001 From: Christian Vogt Date: Mon, 11 Sep 2023 09:42:51 -0400 Subject: [PATCH 1/6] co-locate unit tests with their target source --- docs/architecture.md | 14 +++-- frontend/jest.config.js | 7 ++- .../src/__tests__/dockerRepositoryURL.spec.ts | 49 ----------------- frontend/src/pages/TestPage.tsx.bak | 55 ------------------- .../__tests__}/useGroups.spec.ts | 2 +- .../src/utilities/__tests__/const.spec.ts | 51 +++++++++++++++++ .../notebookControllerUtils.spec.ts | 0 .../__tests__}/useFetchState.spec.ts | 2 +- 8 files changed, 68 insertions(+), 112 deletions(-) delete mode 100644 frontend/src/__tests__/dockerRepositoryURL.spec.ts delete mode 100644 frontend/src/pages/TestPage.tsx.bak rename frontend/src/{__tests__/unit => pages/projects/projectSharing/__tests__}/useGroups.spec.ts (97%) create mode 100644 frontend/src/utilities/__tests__/const.spec.ts rename frontend/src/{__tests__/unit => utilities/__tests__}/notebookControllerUtils.spec.ts (100%) rename frontend/src/{__tests__/unit => utilities/__tests__}/useFetchState.spec.ts (96%) diff --git a/docs/architecture.md b/docs/architecture.md index 3bde9492ce..564a3af313 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -100,20 +100,26 @@ When building new client features, there are a few things worth noting about the Tests can be divided into the following categories: unit, integration, accessibility, and end to end testing. To keep organized of the different types of tests, there will be a test folder at the root of the frontend project with the following structure. +E2e and integration tests are located in a single root directory: ``` -/frontend/tests - /integration => ComponentName.stories.tsx, ComponentName.spec.ts - /unit => functionName.test.ts +/frontend/src/__tests__ /e2e => storyName.spec.ts + /integration => ComponentName.stories.tsx, ComponentName.spec.ts ``` Some nesting can be used to organize testing groups together. For example, the _projects_ page has screens for _details_, _projects_, and, _spawner_ which can be all grouped together under a projects folder. +Unit tests are co-located in a `__tests__` directory adjacent to the target source file they are testing. +``` +/frontend/src/**/__tests__ + /targetFile.spec.ts +``` + #### Testing Types ##### Unit Testing -Unit tests cover util functions and other non React based functions. These tests are stored in the `/unit `folder and can be organized into folders depending on their parent page and/or screen. Use Jest to test each function using `describe` to group together the utils file and the specific function. Then each test is described using `it`. Some functions are very basic and don't need a test. Use your best judgment if a test is needed. +Unit tests cover util functions and other non React based functions. Use Jest to test each function using `describe` to group together the utils file and the specific function. Then each test is described using `it`. _Example_ diff --git a/frontend/jest.config.js b/frontend/jest.config.js index 979a7d6349..b30cfd2aef 100644 --- a/frontend/jest.config.js +++ b/frontend/jest.config.js @@ -2,8 +2,11 @@ // https://jestjs.io/docs/en/configuration.html module.exports = { - roots: ['/src/__tests__/unit'], - testMatch: ['**/?(*.)+(spec|test).ts?(x)'], + roots: ['/src/'], + testMatch: [ + '**/src/__tests__/unit/**/?(*.)+(spec|test).ts?(x)', + '**/__tests__/?(*.)+(spec|test).ts?(x)', + ], // Automatically clear mock calls and instances between every test clearMocks: true, diff --git a/frontend/src/__tests__/dockerRepositoryURL.spec.ts b/frontend/src/__tests__/dockerRepositoryURL.spec.ts deleted file mode 100644 index 3f2b316211..0000000000 --- a/frontend/src/__tests__/dockerRepositoryURL.spec.ts +++ /dev/null @@ -1,49 +0,0 @@ -// https://cloud.google.com/artifact-registry/docs/docker/names -// The full name for a container image is one of the following formats: -// LOCATION-docker.pkg.dev/PROJECT-ID/REPOSITORY/IMAGE -// LOCATION-docker.pkg.dev/PROJECT-ID/REPOSITORY/IMAGE:TAG -// LOCATION-docker.pkg.dev/PROJECT-ID/REPOSITORY/IMAGE@IMAGE-DIGEST - -import { REPOSITORY_URL_REGEX } from '~/utilities/const'; - -test('Invalid URL', () => { - const url = 'docker.io'; - const match = url.match(REPOSITORY_URL_REGEX); - expect(match?.[1]).toBe(''); -}); - -test('Docker container URL without tag', () => { - const url = 'docker.io/library/mysql'; - const match = url.match(REPOSITORY_URL_REGEX); - expect(match?.[1]).toBe('docker.io'); - expect(match?.[4]).toBe(undefined); -}); - -test('Docker container URL with tag', () => { - const url = 'docker.io/library/mysql:test-tag'; - const match = url.match(REPOSITORY_URL_REGEX); - expect(match?.[1]).toBe('docker.io'); - expect(match?.[4]).toBe('test-tag'); -}); - -test('OpenShift internal registry URL without tag', () => { - const url = 'image-registry.openshift-image-registry.svc:5000/opendatahub/s2i-minimal-notebook'; - const match = url.match(REPOSITORY_URL_REGEX); - expect(match?.[1]).toBe('image-registry.openshift-image-registry.svc:5000'); - expect(match?.[4]).toBe(undefined); -}); - -test('OpenShift internal registry URL with tag', () => { - const url = - 'image-registry.openshift-image-registry.svc:5000/opendatahub/s2i-minimal-notebook:v0.3.0-py36'; - const match = url.match(REPOSITORY_URL_REGEX); - expect(match?.[1]).toBe('image-registry.openshift-image-registry.svc:5000'); - expect(match?.[4]).toBe('v0.3.0-py36'); -}); - -test('Quay URL with port and tag', () => { - const url = 'quay.io:443/opendatahub/odh-dashboard:main-55e19fa'; - const match = url.match(REPOSITORY_URL_REGEX); - expect(match?.[1]).toBe('quay.io:443'); - expect(match?.[4]).toBe('main-55e19fa'); -}); diff --git a/frontend/src/pages/TestPage.tsx.bak b/frontend/src/pages/TestPage.tsx.bak deleted file mode 100644 index ce068f1c1d..0000000000 --- a/frontend/src/pages/TestPage.tsx.bak +++ /dev/null @@ -1,55 +0,0 @@ -import { K8sModelCommon, useK8sWatchResource } from '@openshift/dynamic-plugin-sdk-utils'; -import { Alert, AlertVariant, Button } from '@patternfly/react-core'; -import * as React from 'react'; -import { ProjectKind } from '~/k8sTypes'; - -const errorMessage = (e: unknown): string => - (typeof e === 'object' ? e?.toString() : typeof e === 'string' ? e : '') || ''; - -const ProjectModel: K8sModelCommon = { - apiVersion: 'v1', - apiGroup: 'project.openshift.io', - kind: 'Project', - plural: 'projects', -}; -const TestPage = () => { - new Object(); - const [limit, setLimit] = React.useState(100); - const [projects, loaded, error] = useK8sWatchResource( - { - groupVersionKind: { - group: 'project.openshift.io', - version: 'v1', - kind: 'Project', - }, - limit, - isList: true, - }, - ProjectModel, - ); - return ( - <> -
- -
- {!loaded ?
Loading...
: null} - {error ? ( - - {errorMessage(error)} - - ) : null} -
    - {Array.isArray(projects) - ? projects.map((p) =>
  1. {p.metadata.name}
    2. ) - : null} -
- - ); -}; - -export default TestPage; diff --git a/frontend/src/__tests__/unit/useGroups.spec.ts b/frontend/src/pages/projects/projectSharing/__tests__/useGroups.spec.ts similarity index 97% rename from frontend/src/__tests__/unit/useGroups.spec.ts rename to frontend/src/pages/projects/projectSharing/__tests__/useGroups.spec.ts index 200531a10f..fcf404f1a4 100644 --- a/frontend/src/__tests__/unit/useGroups.spec.ts +++ b/frontend/src/pages/projects/projectSharing/__tests__/useGroups.spec.ts @@ -2,7 +2,7 @@ import { act } from '@testing-library/react'; import { k8sListResource } from '@openshift/dynamic-plugin-sdk-utils'; import useGroups from '~/pages/projects/projectSharing/useGroups'; import { GroupKind } from '~/k8sTypes'; -import { expectHook, standardUseFetchState, testHook } from './testUtils/hooks'; +import { expectHook, standardUseFetchState, testHook } from '~/__tests__/unit/testUtils/hooks'; jest.mock('@openshift/dynamic-plugin-sdk-utils', () => ({ k8sListResource: jest.fn(), diff --git a/frontend/src/utilities/__tests__/const.spec.ts b/frontend/src/utilities/__tests__/const.spec.ts new file mode 100644 index 0000000000..19c6bd0f9c --- /dev/null +++ b/frontend/src/utilities/__tests__/const.spec.ts @@ -0,0 +1,51 @@ +// https://cloud.google.com/artifact-registry/docs/docker/names +// The full name for a container image is one of the following formats: +// LOCATION-docker.pkg.dev/PROJECT-ID/REPOSITORY/IMAGE +// LOCATION-docker.pkg.dev/PROJECT-ID/REPOSITORY/IMAGE:TAG +// LOCATION-docker.pkg.dev/PROJECT-ID/REPOSITORY/IMAGE@IMAGE-DIGEST + +import { REPOSITORY_URL_REGEX } from '~/utilities/const'; + +describe('REPOSITORY_URL_REGEX', () => { + test('Invalid URL', () => { + const url = 'docker.io'; + const match = url.match(REPOSITORY_URL_REGEX); + expect(match?.[1]).toBe(''); + }); + + test('Docker container URL without tag', () => { + const url = 'docker.io/library/mysql'; + const match = url.match(REPOSITORY_URL_REGEX); + expect(match?.[1]).toBe('docker.io'); + expect(match?.[4]).toBe(undefined); + }); + + test('Docker container URL with tag', () => { + const url = 'docker.io/library/mysql:test-tag'; + const match = url.match(REPOSITORY_URL_REGEX); + expect(match?.[1]).toBe('docker.io'); + expect(match?.[4]).toBe('test-tag'); + }); + + test('OpenShift internal registry URL without tag', () => { + const url = 'image-registry.openshift-image-registry.svc:5000/opendatahub/s2i-minimal-notebook'; + const match = url.match(REPOSITORY_URL_REGEX); + expect(match?.[1]).toBe('image-registry.openshift-image-registry.svc:5000'); + expect(match?.[4]).toBe(undefined); + }); + + test('OpenShift internal registry URL with tag', () => { + const url = + 'image-registry.openshift-image-registry.svc:5000/opendatahub/s2i-minimal-notebook:v0.3.0-py36'; + const match = url.match(REPOSITORY_URL_REGEX); + expect(match?.[1]).toBe('image-registry.openshift-image-registry.svc:5000'); + expect(match?.[4]).toBe('v0.3.0-py36'); + }); + + test('Quay URL with port and tag', () => { + const url = 'quay.io:443/opendatahub/odh-dashboard:main-55e19fa'; + const match = url.match(REPOSITORY_URL_REGEX); + expect(match?.[1]).toBe('quay.io:443'); + expect(match?.[4]).toBe('main-55e19fa'); + }); +}); diff --git a/frontend/src/__tests__/unit/notebookControllerUtils.spec.ts b/frontend/src/utilities/__tests__/notebookControllerUtils.spec.ts similarity index 100% rename from frontend/src/__tests__/unit/notebookControllerUtils.spec.ts rename to frontend/src/utilities/__tests__/notebookControllerUtils.spec.ts diff --git a/frontend/src/__tests__/unit/useFetchState.spec.ts b/frontend/src/utilities/__tests__/useFetchState.spec.ts similarity index 96% rename from frontend/src/__tests__/unit/useFetchState.spec.ts rename to frontend/src/utilities/__tests__/useFetchState.spec.ts index 220e4bccf7..f29f380a21 100644 --- a/frontend/src/__tests__/unit/useFetchState.spec.ts +++ b/frontend/src/utilities/__tests__/useFetchState.spec.ts @@ -1,6 +1,6 @@ import { act } from '@testing-library/react'; import useFetchState from '~/utilities/useFetchState'; -import { expectHook, standardUseFetchState, testHook } from './testUtils/hooks'; +import { expectHook, standardUseFetchState, testHook } from '~/__tests__/unit/testUtils/hooks'; jest.useFakeTimers(); From 38b2f181661b0b94ee6d6c7718eeb2ace48473e3 Mon Sep 17 00:00:00 2001 From: Juntao Wang Date: Mon, 11 Sep 2023 14:30:33 -0400 Subject: [PATCH 2/6] Fix model server expand issue on the project details page --- .../modelServing/ServingRuntimeList.spec.ts | 26 +++++++++++++++---- .../screens/projects/ServingRuntimeTable.tsx | 8 +++--- .../projects/ServingRuntimeTableRow.tsx | 14 +++++++--- 3 files changed, 34 insertions(+), 14 deletions(-) diff --git a/frontend/src/__tests__/integration/pages/modelServing/ServingRuntimeList.spec.ts b/frontend/src/__tests__/integration/pages/modelServing/ServingRuntimeList.spec.ts index f49d3b8068..486bc8233c 100644 --- a/frontend/src/__tests__/integration/pages/modelServing/ServingRuntimeList.spec.ts +++ b/frontend/src/__tests__/integration/pages/modelServing/ServingRuntimeList.spec.ts @@ -45,17 +45,33 @@ test('Legacy Serving Runtime', async ({ page }) => { await page.waitForSelector('text=Add server'); // Check that the legacy serving runtime is shown with the default runtime name - expect(await page.getByText('ovms')).toBeTruthy(); + expect(page.getByText('ovms')).toBeTruthy(); // Check that the legacy serving runtime displays the correct Serving Runtime - expect(await page.getByText('OpenVINO Model Server')).toBeTruthy(); + expect(page.getByText('OpenVINO Model Server')).toBeTruthy(); // Check that the legacy serving runtime has tokens disabled - expect(await page.getByText('Tokens disabled')).toBeTruthy(); + expect(page.getByText('Tokens disabled')).toBeTruthy(); // Check that the serving runtime is shown with the default runtime name - expect(await page.getByText('OVMS Model Serving')).toBeTruthy(); + expect(page.getByText('OVMS Model Serving')).toBeTruthy(); // Check that the serving runtime displays the correct Serving Runtime - expect(await page.getByText('OpenVINO Serving Runtime (Supports GPUs)')).toBeTruthy(); + expect(page.getByText('OpenVINO Serving Runtime (Supports GPUs)')).toBeTruthy(); + + // Get the first and second row + const firstButton = page.getByRole('button', { name: 'ovms', exact: true }); + const secondButton = page.getByRole('button', { name: 'OVMS Model Serving', exact: true }); + const firstRow = page.getByRole('rowgroup').filter({ has: firstButton }); + const secondRow = page.getByRole('rowgroup').filter({ has: secondButton }); + + // Check that both of the rows are not expanded + await expect(firstRow).not.toHaveClass('pf-m-expanded'); + await expect(secondRow).not.toHaveClass('pf-m-expanded'); + + await firstButton.click(); + + // Check that the first row is expanded while the second is not + await expect(firstRow).toHaveClass('pf-m-expanded'); + await expect(secondRow).not.toHaveClass('pf-m-expanded'); }); diff --git a/frontend/src/pages/modelServing/screens/projects/ServingRuntimeTable.tsx b/frontend/src/pages/modelServing/screens/projects/ServingRuntimeTable.tsx index 60a806871e..cae3343da8 100644 --- a/frontend/src/pages/modelServing/screens/projects/ServingRuntimeTable.tsx +++ b/frontend/src/pages/modelServing/screens/projects/ServingRuntimeTable.tsx @@ -2,7 +2,6 @@ import * as React from 'react'; import Table from '~/components/table/Table'; import { AccessReviewResourceAttributes, ServingRuntimeKind } from '~/k8sTypes'; import { ProjectDetailsContext } from '~/pages/projects/ProjectDetailsContext'; -import { ServingRuntimeTableTabs } from '~/pages/modelServing/screens/types'; import { useAccessReview } from '~/api'; import { columns } from './data'; import ServingRuntimeTableRow from './ServingRuntimeTableRow'; @@ -20,7 +19,7 @@ const ServingRuntimeTable: React.FC = () => { const [deployServingRuntime, setDeployServingRuntime] = React.useState(); const [deleteServingRuntime, setDeleteServingRuntime] = React.useState(); const [editServingRuntime, setEditServingRuntime] = React.useState(); - const [expandedColumn, setExpandedColumn] = React.useState(); + const [expandedServingRuntimeName, setExpandedServingRuntimeName] = React.useState(); const { servingRuntimes: { data: modelServers, refresh: refreshServingRuntime }, @@ -52,8 +51,7 @@ const ServingRuntimeTable: React.FC = () => { onDeleteServingRuntime={(obj) => setDeleteServingRuntime(obj)} onEditServingRuntime={(obj) => setEditServingRuntime(obj)} onDeployModel={(obj) => setDeployServingRuntime(obj)} - expandedColumn={expandedColumn} - setExpandedColumn={setExpandedColumn} + expandedServingRuntimeName={expandedServingRuntimeName} allowDelete={allowDelete} /> )} @@ -95,7 +93,7 @@ const ServingRuntimeTable: React.FC = () => { if (submit) { refreshInferenceServices(); refreshDataConnections(); - setExpandedColumn(ServingRuntimeTableTabs.DEPLOYED_MODELS); + setExpandedServingRuntimeName(deployServingRuntime.metadata.name); } }} projectContext={{ diff --git a/frontend/src/pages/modelServing/screens/projects/ServingRuntimeTableRow.tsx b/frontend/src/pages/modelServing/screens/projects/ServingRuntimeTableRow.tsx index 32be3c29a0..fb94afcb0c 100644 --- a/frontend/src/pages/modelServing/screens/projects/ServingRuntimeTableRow.tsx +++ b/frontend/src/pages/modelServing/screens/projects/ServingRuntimeTableRow.tsx @@ -16,8 +16,7 @@ type ServingRuntimeTableRowProps = { onDeleteServingRuntime: (obj: ServingRuntimeKind) => void; onEditServingRuntime: (obj: ServingRuntimeKind) => void; onDeployModel: (obj: ServingRuntimeKind) => void; - expandedColumn?: ServingRuntimeTableTabs; - setExpandedColumn: (column?: ServingRuntimeTableTabs) => void; + expandedServingRuntimeName?: string; allowDelete: boolean; }; @@ -26,8 +25,7 @@ const ServingRuntimeTableRow: React.FC = ({ onDeleteServingRuntime, onEditServingRuntime, onDeployModel, - expandedColumn, - setExpandedColumn, + expandedServingRuntimeName, allowDelete, }) => { const { @@ -40,6 +38,14 @@ const ServingRuntimeTableRow: React.FC = ({ filterTokens, } = React.useContext(ProjectDetailsContext); + const [expandedColumn, setExpandedColumn] = React.useState(); + + React.useEffect(() => { + if (expandedServingRuntimeName === obj.metadata.name) { + setExpandedColumn(ServingRuntimeTableTabs.DEPLOYED_MODELS); + } + }, [expandedServingRuntimeName, obj.metadata.name]); + const tokens = filterTokens(obj.metadata.name); const modelInferenceServices = getInferenceServiceFromServingRuntime(inferenceServices, obj); From ab2614f6aa07f4feaecc5a87689e3d9c1c7f1ab4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Ko=C5=82osowski?= Date: Sat, 2 Sep 2023 19:36:41 +0200 Subject: [PATCH 3/6] Fixed isCpuLimitEqual and isMemoryLimitEqual wrongly comparing null/undefined values. Also added some unit tests for good measure --- .../utilities/__tests__/valueUnits.spec.ts | 27 +++++++++++++++++++ frontend/src/utilities/valueUnits.ts | 8 ++++++ 2 files changed, 35 insertions(+) create mode 100644 frontend/src/utilities/__tests__/valueUnits.spec.ts diff --git a/frontend/src/utilities/__tests__/valueUnits.spec.ts b/frontend/src/utilities/__tests__/valueUnits.spec.ts new file mode 100644 index 0000000000..d16500bdd4 --- /dev/null +++ b/frontend/src/utilities/__tests__/valueUnits.spec.ts @@ -0,0 +1,27 @@ +import { isCpuLimitEqual, isMemoryLimitEqual } from '~/utilities/valueUnits'; + +describe('isCpuLimitEqual', () => { + test('correctly compares non-undefined values', () => { + expect(isCpuLimitEqual('1', '1')).toBe(true); + expect(isCpuLimitEqual('1000m', '1')).toBe(true); + expect(isCpuLimitEqual('1001m', '1')).toBe(false); + }); + test('correctly compares undefined values', () => { + expect(isCpuLimitEqual('1000m', undefined)).toBe(false); + expect(isCpuLimitEqual('1', undefined)).toBe(false); + expect(isCpuLimitEqual(undefined, undefined)).toBe(true); + }); +}); + +describe('isMemoryLimitEqual', () => { + test('correctly compares non-undefined values', () => { + expect(isMemoryLimitEqual('1Gi', '1Gi')).toBe(true); + expect(isMemoryLimitEqual('1Gi', '1024Mi')).toBe(true); + expect(isMemoryLimitEqual('1Gi', '1025Mi')).toBe(false); + }); + test('correctly compares undefined values', () => { + expect(isMemoryLimitEqual('1Gi', undefined)).toBe(false); + expect(isMemoryLimitEqual('1024Mi', undefined)).toBe(false); + expect(isMemoryLimitEqual(undefined, undefined)).toBe(true); + }); +}); diff --git a/frontend/src/utilities/valueUnits.ts b/frontend/src/utilities/valueUnits.ts index 3d6487bac7..55fd54c6ef 100644 --- a/frontend/src/utilities/valueUnits.ts +++ b/frontend/src/utilities/valueUnits.ts @@ -52,6 +52,10 @@ export const isEqual = ( ): boolean => calculateDelta(value1, value2, units) === 0; export const isCpuLimitEqual = (cpu1?: ValueUnitString, cpu2?: ValueUnitString): boolean => { + if (!cpu1 && !cpu2) { + return true; + } + if (!cpu1 || !cpu2) { return false; } @@ -63,6 +67,10 @@ export const isMemoryLimitEqual = ( memory1?: ValueUnitString, memory2?: ValueUnitString, ): boolean => { + if (!memory1 && !memory2) { + return true; + } + if (!memory1 || !memory2) { return false; } From d92d749f6478f0aa23aa196ca29165b73da95d23 Mon Sep 17 00:00:00 2001 From: Dipanshu Gupta Date: Wed, 30 Aug 2023 17:47:54 +0530 Subject: [PATCH 4/6] Divider disappears after create pipeline --- .../detail/ProjectDetailsComponents.tsx | 23 +------- .../data-connections/DataConnectionsList.tsx | 7 ++- .../screens/detail/notebooks/NotebookList.tsx | 54 ++++++++++--------- .../detail/pipelines/PipelinesList.tsx | 12 ++++- .../detail/pipelines/PipelinesSection.tsx | 50 +++++++++-------- .../screens/detail/storage/StorageList.tsx | 7 ++- 6 files changed, 80 insertions(+), 73 deletions(-) diff --git a/frontend/src/pages/projects/screens/detail/ProjectDetailsComponents.tsx b/frontend/src/pages/projects/screens/detail/ProjectDetailsComponents.tsx index 6f0126e7b8..819130f635 100644 --- a/frontend/src/pages/projects/screens/detail/ProjectDetailsComponents.tsx +++ b/frontend/src/pages/projects/screens/detail/ProjectDetailsComponents.tsx @@ -1,12 +1,10 @@ import * as React from 'react'; -import { Divider, PageSection, Stack, StackItem } from '@patternfly/react-core'; +import { PageSection, Stack, StackItem } from '@patternfly/react-core'; import GenericSidebar from '~/components/GenericSidebar'; import { useAppContext } from '~/app/AppContext'; import ServingRuntimeList from '~/pages/modelServing/screens/projects/ServingRuntimeList'; -import { ProjectDetailsContext } from '~/pages/projects/ProjectDetailsContext'; import { featureFlagEnabled } from '~/utilities/utils'; import PipelinesSection from '~/pages/projects/screens/detail/pipelines/PipelinesSection'; -import { usePipelinesAPI } from '~/concepts/pipelines/context'; import NotebooksList from './notebooks/NotebookList'; import { ProjectSectionID } from './types'; import StorageList from './storage/StorageList'; @@ -17,19 +15,10 @@ import useCheckLogoutParams from './useCheckLogoutParams'; type SectionType = { id: ProjectSectionID; component: React.ReactNode; - isEmpty: boolean; }; const ProjectDetailsComponents: React.FC = () => { const { dashboardConfig } = useAppContext(); - const { - notebooks: { data: notebookStates, loaded: notebookStatesLoaded }, - pvcs: { data: pvcs, loaded: pvcsLoaded }, - dataConnections: { data: connections, loaded: connectionsLoaded }, - servingRuntimes: { data: modelServers, loaded: modelServersLoaded }, - } = React.useContext(ProjectDetailsContext); - const { pipelinesServer } = usePipelinesAPI(); - const modelServingEnabled = featureFlagEnabled( dashboardConfig.spec.dashboardConfig.disableModelServing, ); @@ -42,24 +31,20 @@ const ProjectDetailsComponents: React.FC = () => { { id: ProjectSectionID.WORKBENCHES, component: , - isEmpty: notebookStatesLoaded && notebookStates.length === 0, }, { id: ProjectSectionID.CLUSTER_STORAGES, component: , - isEmpty: pvcsLoaded && pvcs.length === 0, }, { id: ProjectSectionID.DATA_CONNECTIONS, component: , - isEmpty: connectionsLoaded && connections.length === 0, }, ...(pipelinesEnabled ? [ { id: ProjectSectionID.PIPELINES, component: , - isEmpty: !pipelinesServer.installed, }, ] : []), @@ -68,7 +53,6 @@ const ProjectDetailsComponents: React.FC = () => { { id: ProjectSectionID.MODEL_SERVER, component: , - isEmpty: modelServersLoaded && modelServers.length === 0, }, ] : []), @@ -82,7 +66,7 @@ const ProjectDetailsComponents: React.FC = () => { maxWidth={175} > - {sections.map(({ id, component, isEmpty }, index) => ( + {sections.map(({ id, component }) => ( { > {component} - {index !== sections.length - 1 && isEmpty && ( - - )} ))} diff --git a/frontend/src/pages/projects/screens/detail/data-connections/DataConnectionsList.tsx b/frontend/src/pages/projects/screens/detail/data-connections/DataConnectionsList.tsx index 18d23c0fe7..92a686590e 100644 --- a/frontend/src/pages/projects/screens/detail/data-connections/DataConnectionsList.tsx +++ b/frontend/src/pages/projects/screens/detail/data-connections/DataConnectionsList.tsx @@ -1,5 +1,5 @@ import * as React from 'react'; -import { Button } from '@patternfly/react-core'; +import { Button, Divider } from '@patternfly/react-core'; import EmptyDetailsList from '~/pages/projects/screens/detail/EmptyDetailsList'; import { ProjectSectionID } from '~/pages/projects/screens/detail/types'; import DetailsSection from '~/pages/projects/screens/detail/DetailsSection'; @@ -15,6 +15,8 @@ const DataConnectionsList: React.FC = () => { } = React.useContext(ProjectDetailsContext); const [open, setOpen] = React.useState(false); + const isDataConnectionsEmpty = connections.length === 0; + return ( <> { , ]} isLoading={!loaded} - isEmpty={connections.length === 0} + isEmpty={isDataConnectionsEmpty} loadError={error} emptyState={ { > + {isDataConnectionsEmpty && } { diff --git a/frontend/src/pages/projects/screens/detail/notebooks/NotebookList.tsx b/frontend/src/pages/projects/screens/detail/notebooks/NotebookList.tsx index 0396239c96..5759aea454 100644 --- a/frontend/src/pages/projects/screens/detail/notebooks/NotebookList.tsx +++ b/frontend/src/pages/projects/screens/detail/notebooks/NotebookList.tsx @@ -1,5 +1,5 @@ import * as React from 'react'; -import { Button } from '@patternfly/react-core'; +import { Button, Divider } from '@patternfly/react-core'; import { useNavigate } from 'react-router-dom'; import EmptyDetailsList from '~/pages/projects/screens/detail/EmptyDetailsList'; import { ProjectSectionID } from '~/pages/projects/screens/detail/types'; @@ -17,6 +17,7 @@ const NotebookList: React.FC = () => { } = React.useContext(ProjectDetailsContext); const navigate = useNavigate(); const projectName = currentProject.metadata.name; + const isNotebooksEmpty = notebookStates.length === 0; React.useEffect(() => { let interval: ReturnType; @@ -27,30 +28,33 @@ const NotebookList: React.FC = () => { }, [notebookStates, refreshNotebooks]); return ( - navigate(`/projects/${projectName}/spawner`)} - variant="secondary" - > - Create workbench - , - ]} - isLoading={!loaded} - loadError={loadError} - isEmpty={notebookStates.length === 0} - emptyState={ - - } - > - - + <> + navigate(`/projects/${projectName}/spawner`)} + variant="secondary" + > + Create workbench + , + ]} + isLoading={!loaded} + loadError={loadError} + isEmpty={isNotebooksEmpty} + emptyState={ + + } + > + + + {isNotebooksEmpty && } + ); }; diff --git a/frontend/src/pages/projects/screens/detail/pipelines/PipelinesList.tsx b/frontend/src/pages/projects/screens/detail/pipelines/PipelinesList.tsx index f038c0d882..4b41778b1e 100644 --- a/frontend/src/pages/projects/screens/detail/pipelines/PipelinesList.tsx +++ b/frontend/src/pages/projects/screens/detail/pipelines/PipelinesList.tsx @@ -9,11 +9,21 @@ import { usePipelinesAPI } from '~/concepts/pipelines/context'; import EmptyStateErrorMessage from '~/components/EmptyStateErrorMessage'; import { TABLE_CONTENT_LIMIT, LIMIT_MAX_ITEM_COUNT } from '~/concepts/pipelines/const'; -const PipelinesList: React.FC = () => { +type PipelinesListProps = { + setIsPipelinesEmpty: (isEmpty: boolean) => void; +}; + +const PipelinesList: React.FC = ({ setIsPipelinesEmpty }) => { const { namespace } = usePipelinesAPI(); const [pipelines, loaded, loadError, refresh] = usePipelines(LIMIT_MAX_ITEM_COUNT); const navigate = useNavigate(); + const isPipelinesEmpty = pipelines.length === 0; + + React.useEffect(() => { + setIsPipelinesEmpty(isPipelinesEmpty); + }, [isPipelinesEmpty, setIsPipelinesEmpty]); + if (loadError) { return ( diff --git a/frontend/src/pages/projects/screens/detail/pipelines/PipelinesSection.tsx b/frontend/src/pages/projects/screens/detail/pipelines/PipelinesSection.tsx index ed36a91cd9..d8039dde92 100644 --- a/frontend/src/pages/projects/screens/detail/pipelines/PipelinesSection.tsx +++ b/frontend/src/pages/projects/screens/detail/pipelines/PipelinesSection.tsx @@ -1,4 +1,5 @@ import * as React from 'react'; +import { Divider } from '@patternfly/react-core'; import { ProjectSectionID } from '~/pages/projects/screens/detail/types'; import { ProjectSectionTitles } from '~/pages/projects/screens/detail/const'; import DetailsSection from '~/pages/projects/screens/detail/DetailsSection'; @@ -13,29 +14,34 @@ const PipelinesSection: React.FC = () => { pipelinesServer: { initializing, installed, timedOut }, } = usePipelinesAPI(); + const [isPipelinesEmpty, setIsPipelinesEmpty] = React.useState(false); + return ( - , - ]} - isLoading={initializing} - isEmpty={!installed} - emptyState={} - > - {timedOut ? ( - - ) : ( - - - - )} - + <> + , + ]} + isLoading={initializing} + isEmpty={!installed} + emptyState={} + > + {timedOut ? ( + + ) : ( + + + + )} + + {(isPipelinesEmpty || !installed) && } + ); }; diff --git a/frontend/src/pages/projects/screens/detail/storage/StorageList.tsx b/frontend/src/pages/projects/screens/detail/storage/StorageList.tsx index 439f8f86b8..3ebdd5c4f8 100644 --- a/frontend/src/pages/projects/screens/detail/storage/StorageList.tsx +++ b/frontend/src/pages/projects/screens/detail/storage/StorageList.tsx @@ -1,5 +1,5 @@ import * as React from 'react'; -import { Button } from '@patternfly/react-core'; +import { Button, Divider } from '@patternfly/react-core'; import EmptyDetailsList from '~/pages/projects/screens/detail/EmptyDetailsList'; import DetailsSection from '~/pages/projects/screens/detail/DetailsSection'; import { ProjectSectionID } from '~/pages/projects/screens/detail/types'; @@ -15,6 +15,8 @@ const StorageList: React.FC = () => { refreshAllProjectData: refresh, } = React.useContext(ProjectDetailsContext); + const isPvcsEmpty = pvcs.length === 0; + return ( <> { , ]} isLoading={!loaded} - isEmpty={pvcs.length === 0} + isEmpty={isPvcsEmpty} loadError={loadError} emptyState={ { > setOpen(true)} /> + {isPvcsEmpty && } { From 92113b81c4a7717913e7773383d4d325c40d355e Mon Sep 17 00:00:00 2001 From: Juntao Wang Date: Wed, 6 Sep 2023 11:51:25 -0400 Subject: [PATCH 5/6] Add shm to ServingRuntime volumes and volumeMounts --- backend/src/types.ts | 2 ++ frontend/src/api/k8s/notebooks.ts | 12 +--------- frontend/src/api/k8s/servingRuntimes.ts | 29 +++++++++++++++++++------ frontend/src/api/k8s/utils.ts | 12 ++++++++++ frontend/src/k8sTypes.ts | 3 +++ 5 files changed, 40 insertions(+), 18 deletions(-) diff --git a/backend/src/types.ts b/backend/src/types.ts index 73fef553b3..8dc3a0cac8 100644 --- a/backend/src/types.ts +++ b/backend/src/types.ts @@ -901,8 +901,10 @@ export type ServingRuntime = K8sResourceCommon & { image: string; name: string; resources: ContainerResources; + volumeMounts?: VolumeMount[]; }[]; supportedModelFormats: SupportedModelFormats[]; replicas: number; + volumes?: Volume[]; }; }; diff --git a/frontend/src/api/k8s/notebooks.ts b/frontend/src/api/k8s/notebooks.ts index 8c1ddb8d80..d7a86ba07e 100644 --- a/frontend/src/api/k8s/notebooks.ts +++ b/frontend/src/api/k8s/notebooks.ts @@ -26,17 +26,7 @@ import { } from '~/concepts/pipelines/elyra/utils'; import { createRoleBinding } from '~/api'; import { Volume, VolumeMount } from '~/types'; -import { assemblePodSpecOptions } from './utils'; - -const getshmVolumeMount = (): VolumeMount => ({ - name: 'shm', - mountPath: '/dev/shm', -}); - -const getshmVolume = (): Volume => ({ - name: 'shm', - emptyDir: { medium: 'Memory' }, -}); +import { assemblePodSpecOptions, getshmVolume, getshmVolumeMount } from './utils'; const assembleNotebook = ( data: StartNotebookData, diff --git a/frontend/src/api/k8s/servingRuntimes.ts b/frontend/src/api/k8s/servingRuntimes.ts index 3842828d48..a8b4699cac 100644 --- a/frontend/src/api/k8s/servingRuntimes.ts +++ b/frontend/src/api/k8s/servingRuntimes.ts @@ -14,7 +14,7 @@ import { getModelServingRuntimeName } from '~/pages/modelServing/utils'; import { getDisplayNameFromK8sResource, translateDisplayNameForK8s } from '~/pages/projects/utils'; import { applyK8sAPIOptions } from '~/api/apiMergeUtils'; import { getModelServingProjects } from './projects'; -import { assemblePodSpecOptions } from './utils'; +import { assemblePodSpecOptions, getshmVolume, getshmVolumeMount } from './utils'; const assembleServingRuntime = ( data: CreatingServingRuntimeObject, @@ -79,12 +79,27 @@ const assembleServingRuntime = ( const { affinity, tolerations, resources } = assemblePodSpecOptions(resourceSettings, gpus); - updatedServingRuntime.spec.containers = servingRuntime.spec.containers.map((container) => ({ - ...container, - resources, - affinity, - tolerations, - })); + const volumes = updatedServingRuntime.spec.volumes || []; + if (!volumes.find((volume) => volume.name === 'shm')) { + volumes.push(getshmVolume('2Gi')); + } + + updatedServingRuntime.spec.volumes = volumes; + + updatedServingRuntime.spec.containers = servingRuntime.spec.containers.map((container) => { + const volumeMounts = container.volumeMounts || []; + if (!volumeMounts.find((volumeMount) => volumeMount.mountPath === '/dev/shm')) { + volumeMounts.push(getshmVolumeMount()); + } + + return { + ...container, + resources, + affinity, + tolerations, + volumeMounts, + }; + }); return updatedServingRuntime; }; diff --git a/frontend/src/api/k8s/utils.ts b/frontend/src/api/k8s/utils.ts index 920415d757..ce2867007c 100644 --- a/frontend/src/api/k8s/utils.ts +++ b/frontend/src/api/k8s/utils.ts @@ -4,6 +4,8 @@ import { PodToleration, TolerationSettings, ContainerResourceAttributes, + VolumeMount, + Volume, } from '~/types'; import { determineTolerations } from '~/utilities/tolerations'; @@ -54,3 +56,13 @@ export const assemblePodSpecOptions = ( const tolerations = determineTolerations(gpus > 0, tolerationSettings); return { affinity, tolerations, resources }; }; + +export const getshmVolumeMount = (): VolumeMount => ({ + name: 'shm', + mountPath: '/dev/shm', +}); + +export const getshmVolume = (sizeLimit?: string): Volume => ({ + name: 'shm', + emptyDir: { medium: 'Memory', ...(sizeLimit && { sizeLimit }) }, +}); diff --git a/frontend/src/k8sTypes.ts b/frontend/src/k8sTypes.ts index 3b4f16524e..2795ef2eed 100644 --- a/frontend/src/k8sTypes.ts +++ b/frontend/src/k8sTypes.ts @@ -12,6 +12,7 @@ import { TolerationSettings, ImageStreamStatusTagItem, ImageStreamStatusTagCondition, + VolumeMount, } from './types'; import { ServingRuntimeSize } from './pages/modelServing/screens/types'; @@ -327,9 +328,11 @@ export type ServingRuntimeKind = K8sResourceCommon & { image: string; name: string; resources: ContainerResources; + volumeMounts?: VolumeMount[]; }[]; supportedModelFormats: SupportedModelFormats[]; replicas: number; + volumes?: Volume[]; }; }; From 27d87fed3ab8b1d48e756a0a1f2dc7aea57539ca Mon Sep 17 00:00:00 2001 From: Christian Vogt Date: Mon, 11 Sep 2023 16:30:11 -0400 Subject: [PATCH 6/6] better error handling when parsing s3 endpoint --- frontend/jest.config.js | 2 +- .../__tests__/utils.spec.ts | 130 ++++++++++++++++++ .../content/configurePipelinesServer/utils.ts | 25 ++-- 3 files changed, 148 insertions(+), 9 deletions(-) create mode 100644 frontend/src/concepts/pipelines/content/configurePipelinesServer/__tests__/utils.spec.ts diff --git a/frontend/jest.config.js b/frontend/jest.config.js index b30cfd2aef..68a37f4e48 100644 --- a/frontend/jest.config.js +++ b/frontend/jest.config.js @@ -26,7 +26,7 @@ module.exports = { testEnvironment: 'jest-environment-jsdom', // include projects from node_modules as required - transformIgnorePatterns: ['node_modules/(?!yaml)'], + transformIgnorePatterns: ['node_modules/(?!yaml|@openshift|lodash-es|uuid)'], // A list of paths to snapshot serializer modules Jest should use for snapshot testing snapshotSerializers: [], diff --git a/frontend/src/concepts/pipelines/content/configurePipelinesServer/__tests__/utils.spec.ts b/frontend/src/concepts/pipelines/content/configurePipelinesServer/__tests__/utils.spec.ts new file mode 100644 index 0000000000..20fe6b4266 --- /dev/null +++ b/frontend/src/concepts/pipelines/content/configurePipelinesServer/__tests__/utils.spec.ts @@ -0,0 +1,130 @@ +import { AWS_KEYS } from '~/pages/projects/dataConnections/const'; +import { PipelineServerConfigType } from '~/concepts/pipelines/content/configurePipelinesServer/types'; +import { createDSPipelineResourceSpec } from '~/concepts/pipelines/content/configurePipelinesServer/utils'; + +describe('configure pipeline server utils', () => { + describe('createDSPipelineResourceSpec', () => { + const createPipelineServerConfig = () => + ({ + database: { + useDefault: true, + value: [], + }, + objectStorage: { + useExisting: true, + existingName: '', + existingValue: [], + }, + } as PipelineServerConfigType); + + type SecretsResponse = Parameters[1]; + + const createSecretsResponse = ( + databaseSecret?: SecretsResponse[0], + objectStorageSecret?: SecretsResponse[1], + ): SecretsResponse => [databaseSecret, objectStorageSecret ?? { secretName: '', awsData: [] }]; + + it('should create resource spec', () => { + const spec = createDSPipelineResourceSpec( + createPipelineServerConfig(), + createSecretsResponse(), + ); + expect(spec).toEqual({ + database: undefined, + objectStorage: { + externalStorage: { + bucket: '', + host: '', + s3CredentialsSecret: { + accessKey: 'AWS_ACCESS_KEY_ID', + secretKey: 'AWS_SECRET_ACCESS_KEY', + secretName: '', + }, + scheme: 'https', + }, + }, + }); + }); + + it('should parse S3 endpoint with scheme', () => { + const secretsResponse = createSecretsResponse(); + secretsResponse[1].awsData = [ + { key: AWS_KEYS.S3_ENDPOINT, value: 'http://s3.amazonaws.com' }, + ]; + const spec = createDSPipelineResourceSpec(createPipelineServerConfig(), secretsResponse); + expect(spec.objectStorage.externalStorage?.scheme).toBe('http'); + expect(spec.objectStorage.externalStorage?.host).toBe('s3.amazonaws.com'); + }); + + it('should parse S3 endpoint without scheme', () => { + const secretsResponse = createSecretsResponse(); + + secretsResponse[1].awsData = [{ key: AWS_KEYS.S3_ENDPOINT, value: 's3.amazonaws.com' }]; + const spec = createDSPipelineResourceSpec(createPipelineServerConfig(), secretsResponse); + expect(spec.objectStorage.externalStorage?.scheme).toBe('https'); + expect(spec.objectStorage.externalStorage?.host).toBe('s3.amazonaws.com'); + }); + + it('should include bucket', () => { + const secretsResponse = createSecretsResponse(); + secretsResponse[1].awsData = [{ key: AWS_KEYS.AWS_S3_BUCKET, value: 'my-bucket' }]; + const spec = createDSPipelineResourceSpec(createPipelineServerConfig(), secretsResponse); + expect(spec.objectStorage.externalStorage?.bucket).toBe('my-bucket'); + }); + + it('should create spec with database object', () => { + const config = createPipelineServerConfig(); + config.database.value = [ + { + key: 'Username', + value: 'test-user', + }, + { + key: 'Port', + value: '8080', + }, + { + key: 'Host', + value: 'test.host.com', + }, + { + key: 'Database', + value: 'db-name', + }, + ]; + const spec = createDSPipelineResourceSpec( + config, + createSecretsResponse({ + key: 'password-key', + name: 'password-name', + }), + ); + expect(spec).toEqual({ + objectStorage: { + externalStorage: { + bucket: '', + host: '', + s3CredentialsSecret: { + accessKey: 'AWS_ACCESS_KEY_ID', + secretKey: 'AWS_SECRET_ACCESS_KEY', + secretName: '', + }, + scheme: 'https', + }, + }, + database: { + externalDB: { + host: 'test.host.com', + passwordSecret: { + key: 'password-key', + name: 'password-name', + }, + pipelineDBName: 'db-name', + port: '8080', + username: 'test-user', + }, + }, + }); + }); + }); +}); diff --git a/frontend/src/concepts/pipelines/content/configurePipelinesServer/utils.ts b/frontend/src/concepts/pipelines/content/configurePipelinesServer/utils.ts index 06c5b00164..d4cb2f6553 100644 --- a/frontend/src/concepts/pipelines/content/configurePipelinesServer/utils.ts +++ b/frontend/src/concepts/pipelines/content/configurePipelinesServer/utils.ts @@ -116,22 +116,22 @@ const createSecrets = (config: PipelineServerConfigType, projectName: string) => .catch(reject); }); -export const configureDSPipelineResourceSpec = ( +export const createDSPipelineResourceSpec = ( config: PipelineServerConfigType, - projectName: string, -): Promise => - createSecrets(config, projectName).then(([databaseSecret, objectStorageSecret]) => { + [databaseSecret, objectStorageSecret]: SecretsResponse, +): DSPipelineKind['spec'] => { + { const awsRecord = dataEntryToRecord(objectStorageSecret.awsData); const databaseRecord = dataEntryToRecord(config.database.value); const [, externalStorageScheme, externalStorageHost] = - awsRecord.AWS_S3_ENDPOINT?.match(/^(\w+):\/\/(.*)/) ?? []; + awsRecord.AWS_S3_ENDPOINT?.match(/^(?:(\w+):\/\/)?(.*)/) ?? []; return { objectStorage: { externalStorage: { - host: externalStorageHost.replace(/\/$/, ''), - scheme: externalStorageScheme, + host: externalStorageHost?.replace(/\/$/, '') || '', + scheme: externalStorageScheme || 'https', bucket: awsRecord.AWS_S3_BUCKET || '', s3CredentialsSecret: { accessKey: AWS_KEYS.ACCESS_KEY_ID, @@ -155,4 +155,13 @@ export const configureDSPipelineResourceSpec = ( } : undefined, }; - }); + } +}; + +export const configureDSPipelineResourceSpec = ( + config: PipelineServerConfigType, + projectName: string, +): Promise => + createSecrets(config, projectName).then((secretsResponse) => + createDSPipelineResourceSpec(config, secretsResponse), + );