Merge branch 'opendatahub-io:main' into task/RHOAIENG-7076-optimize-h…

…ow-we-fetch-for-pipeline-experiments
Gkrumbach07 · Jul 16, 2024 · b47bc73 · b47bc73
2 parents e815d57 + 279d95d
commit b47bc73
Show file tree

Hide file tree

Showing 9 changed files with 130 additions and 112 deletions.
diff --git a/backend/src/routes/api/service/modelregistry/index.ts b/backend/src/routes/api/service/modelregistry/index.ts
@@ -1,32 +1,18 @@
-import httpProxy from '@fastify/http-proxy';
-import { KubeFastifyInstance } from '../../../../types';
-import { DEV_MODE, MODEL_REGISTRY_NAMESPACE } from '../../../../utils/constants';
-import { getParam, setParam } from '../../../../utils/proxy';
+import { MODEL_REGISTRY_NAMESPACE } from '../../../../utils/constants';
+import { proxyService } from '../../../../utils/proxy';
 
-export default async (fastify: KubeFastifyInstance): Promise<void> => {
-  fastify.register(httpProxy, {
-    upstream: '',
-    prefix: '/:name',
-    rewritePrefix: '',
-    replyOptions: {
-      // preHandler must set the `upstream` param
-      getUpstream: (request) => getParam(request, 'upstream'),
-    },
-    preHandler: (request, _, done) => {
-      const name = getParam(request, 'name');
-
-      const upstream = DEV_MODE
-        ? // Use port forwarding for local development:
-          // kubectl port-forward -n <namespace> svc/<service-name> <local.port>:<service.port>
-          `http://${process.env.MODEL_REGISTRY_SERVICE_HOST}:${process.env.MODEL_REGISTRY_SERVICE_PORT}`
-        : // Construct service URL
-          `http://${name}.${MODEL_REGISTRY_NAMESPACE}.svc.cluster.local:8080`;
-
-      // assign the `upstream` param so we can dynamically set the upstream URL for http-proxy
-      setParam(request, 'upstream', upstream);
-
-      fastify.log.info(`Proxy ${request.method} request ${request.url} to ${upstream}`);
-      done();
-    },
-  });
-};
+export default proxyService(
+  null,
+  {
+    port: 8080,
+    namespace: MODEL_REGISTRY_NAMESPACE,
+  },
+  {
+    // Use port forwarding for local development:
+    // kubectl port-forward -n odh-model-registries svc/<service-name> 8085:8080
+    host: process.env.MODEL_REGISTRY_SERVICE_HOST,
+    port: process.env.MODEL_REGISTRY_SERVICE_PORT,
+  },
+  null,
+  false,
+);
diff --git a/backend/src/utils/proxy.ts b/backend/src/utils/proxy.ts
@@ -27,12 +27,13 @@ const notFoundError = (kind: string, name: string, e?: any, overrideMessage?: st
 };
 
 export const proxyService =
-  <K extends K8sResourceCommon>(
-    model: { apiGroup: string; apiVersion: string; plural: string; kind: string },
+  <K extends K8sResourceCommon = never>(
+    model: { apiGroup: string; apiVersion: string; plural: string; kind: string } | null,
     service: {
       port: number | string;
       prefix?: string;
       suffix?: string;
+      namespace?: string;
     },
     local: {
       host: string;
@@ -44,7 +45,7 @@ export const proxyService =
   async (fastify: KubeFastifyInstance): Promise<void> => {
     fastify.register(httpProxy, {
       upstream: '',
-      prefix: '/:namespace/:name',
+      prefix: service.namespace ? ':name' : '/:namespace/:name',
       rewritePrefix: '',
       replyOptions: {
         // preHandler must set the `upstream` param
@@ -65,52 +66,60 @@ export const proxyService =
           );
           return;
         }
-        const kc = fastify.kube.config;
-        const cluster = kc.getCurrentCluster();
-
         // see `prefix` for named params
-        const namespace = getParam(request, 'namespace');
+        const namespace = service.namespace ?? getParam(request, 'namespace');
         const name = getParam(request, 'name');
 
-        // retreive the gating resource by name and namespace
-        passThroughResource<K>(fastify, request, {
-          url: `${cluster.server}/apis/${model.apiGroup}/${model.apiVersion}/namespaces/${namespace}/${model.plural}/${name}`,
-          method: 'GET',
-        })
-          .then((resource) => {
-            return getDirectCallOptions(fastify, request, request.url).then((requestOptions) => {
-              if (isK8sStatus(resource)) {
-                done(notFoundError(model.kind, name));
-              } else if (!statusCheck || statusCheck(resource)) {
-                if (tls) {
-                  const token = getAccessToken(requestOptions);
-                  request.headers.authorization = `Bearer ${token}`;
-                }
+        const doServiceRequest = () => {
+          const scheme = tls ? 'https' : 'http';
 
-                const scheme = tls ? 'https' : 'http';
+          const upstream = DEV_MODE
+            ? // Use port forwarding for local development:
+              // kubectl port-forward -n <namespace> svc/<service-name> <local.port>:<service.port>
+              `${scheme}://${local.host}:${local.port}`
+            : // Construct service URL
+              `${scheme}://${service.prefix || ''}${name}${
+                service.suffix ?? ''
+              }.${namespace}.svc.cluster.local:${service.port}`;
 
-                const upstream = DEV_MODE
-                  ? // Use port forwarding for local development:
-                    // kubectl port-forward -n <namespace> svc/<service-name> <local.port>:<service.port>
-                    `${scheme}://${local.host}:${local.port}`
-                  : // Construct service URL
-                    `${scheme}://${service?.prefix || ''}${resource.metadata.name}${
-                      service?.suffix ?? ''
-                    }.${resource.metadata.namespace}.svc.cluster.local:${service.port}`;
+          // assign the `upstream` param so we can dynamically set the upstream URL for http-proxy
+          setParam(request, 'upstream', upstream);
 
-                // assign the `upstream` param so we can dynamically set the upstream URL for http-proxy
-                setParam(request, 'upstream', upstream);
+          fastify.log.info(`Proxy ${request.method} request ${request.url} to ${upstream}`);
+          done();
+        };
 
-                fastify.log.info(`Proxy ${request.method} request ${request.url} to ${upstream}`);
-                done();
-              } else {
-                done(notFoundError(model.kind, name, undefined, 'service unavailable'));
-              }
-            });
+        if (model) {
+          const kc = fastify.kube.config;
+          const cluster = kc.getCurrentCluster();
+
+          // retreive the gating resource by name and namespace
+          passThroughResource<K>(fastify, request, {
+            url: `${cluster.server}/apis/${model.apiGroup}/${model.apiVersion}/namespaces/${namespace}/${model.plural}/${name}`,
+            method: 'GET',
           })
-          .catch((e) => {
-            done(notFoundError(model.kind, name, e));
-          });
+            .then((resource) => {
+              return getDirectCallOptions(fastify, request, request.url).then((requestOptions) => {
+                if (isK8sStatus(resource)) {
+                  done(notFoundError(model.kind, name));
+                } else if (!statusCheck || statusCheck(resource)) {
+                  if (tls) {
+                    const token = getAccessToken(requestOptions);
+                    request.headers.authorization = `Bearer ${token}`;
+                  }
+
+                  doServiceRequest();
+                } else {
+                  done(notFoundError(model.kind, name, undefined, 'service unavailable'));
+                }
+              });
+            })
+            .catch((e) => {
+              done(notFoundError(model.kind, name, e));
+            });
+        } else {
+          doServiceRequest();
+        }
       },
     });
   };
diff --git a/frontend/src/__tests__/cypress/cypress/pages/modelRegistrySettings.ts b/frontend/src/__tests__/cypress/cypress/pages/modelRegistrySettings.ts
@@ -2,6 +2,7 @@ import { appChrome } from './appChrome';
 
 export enum FormFieldSelector {
   NAME = '#mr-name',
+  RESOURCENAME = '#resource-mr-name',
   HOST = '#mr-host',
   PORT = '#mr-port',
   USERNAME = '#mr-username',
@@ -10,7 +11,6 @@ export enum FormFieldSelector {
 }
 
 export enum FormErrorTestId {
-  NAME = 'mr-name-error',
   HOST = 'mr-host-error',
   PORT = 'mr-port-error',
   USERNAME = 'mr-username-error',

diff --git a/.../__tests__/cypress/cypress/tests/mocked/modelRegistrySettings/modelRegistrySettings.cy.ts b/.../__tests__/cypress/cypress/tests/mocked/modelRegistrySettings/modelRegistrySettings.cy.ts
@@ -3,7 +3,6 @@ import { mockDsciStatus } from '~/__mocks__/mockDsciStatus';
 import { StackCapability, StackComponent } from '~/concepts/areas/types';
 import {
   FormFieldSelector,
-  FormErrorTestId,
   modelRegistrySettings,
   DatabaseDetailsTestId,
 } from '~/__tests__/cypress/cypress/pages/modelRegistrySettings';
@@ -106,14 +105,6 @@ describe('CreateModal', () => {
     modelRegistrySettings.shouldHaveAllErrors();
   });
 
-  it('should display error when name is not empty but is invalid', () => {
-    modelRegistrySettings.visit(true);
-    modelRegistrySettings.findCreateButton().click();
-    modelRegistrySettings.findFormField(FormFieldSelector.NAME).type('Invalid Name');
-    modelRegistrySettings.findFormField(FormFieldSelector.NAME).blur();
-    modelRegistrySettings.findFormError(FormErrorTestId.NAME).should('exist');
-  });
-
   it('should enable submit button if fields are valid', () => {
     modelRegistrySettings.visit(true);
     cy.findByText('Create model registry').click();

diff --git a/frontend/src/__tests__/cypress/test-variables.yml.example b/frontend/src/__tests__/cypress/test-variables.yml.example
@@ -0,0 +1,9 @@
+ODH_DASHBOARD_URL: "http://odh-dashboard-opendatahub.apps.my-cluster.test.redhat.com"
+TEST_USER:
+  AUTH_TYPE: foo-auth
+  USERNAME: foo-user
+  PASSWORD: foo-passwd
+OCP_ADMIN_USER:
+  AUTH_TYPE: adm-auth
+  USERNAME: adminuser
+  PASSWORD: adminuser-passwd
diff --git a/frontend/src/k8sTypes.ts b/frontend/src/k8sTypes.ts
@@ -1302,6 +1302,7 @@ export type ModelRegistryKind = K8sResourceCommon & {
   metadata: {
     name: string;
     namespace: string;
+    annotations?: DisplayNameAnnotations;
   };
   spec: {
     grpc: {

diff --git a/frontend/src/pages/home/projects/ProjectsLoading.tsx b/frontend/src/pages/home/projects/ProjectsLoading.tsx
@@ -1,9 +1,20 @@
+import {
+  EmptyState,
+  EmptyStateVariant,
+  Spinner,
+  EmptyStateHeader,
+  Bullseye,
+} from '@patternfly/react-core';
 import * as React from 'react';
-import { Skeleton } from '@patternfly/react-core';
 
 const ProjectsLoading: React.FC = () => (
   <div style={{ height: '230px' }}>
-    <Skeleton height="75%" width="100%" screenreaderText="Loading projects" />
+    <Bullseye>
+      <EmptyState variant={EmptyStateVariant.lg} data-id="loading-empty-state">
+        <Spinner size="xl" />
+        <EmptyStateHeader titleText="Loading" headingLevel="h1" />
+      </EmptyState>
+    </Bullseye>
   </div>
 );
 

diff --git a/frontend/src/pages/modelRegistrySettings/CreateModal.tsx b/frontend/src/pages/modelRegistrySettings/CreateModal.tsx
@@ -17,8 +17,10 @@ import { ModelRegistryKind } from '~/k8sTypes';
 import { MODEL_REGISTRY_DEFAULT_NAMESPACE } from '~/concepts/modelRegistry/const';
 import { ModelRegistryModel } from '~/api';
 import { createModelRegistryBackend } from '~/services/modelRegistrySettingsService';
-import { isValidK8sName } from '~/concepts/k8s/utils';
+import { isValidK8sName, translateDisplayNameForK8s } from '~/concepts/k8s/utils';
 import './CreateModal.scss';
+import NameDescriptionField from '~/concepts/k8s/NameDescriptionField';
+import { NameDescType } from '~/pages/projects/types';
 
 type CreateModalProps = {
   isOpen: boolean;
@@ -29,13 +31,16 @@ type CreateModalProps = {
 const CreateModal: React.FC<CreateModalProps> = ({ isOpen, onClose, refresh }) => {
   const [isSubmitting, setIsSubmitting] = React.useState(false);
   const [error, setError] = React.useState<Error>();
-  const [name, setName] = React.useState('');
+  const [nameDesc, setNameDesc] = React.useState<NameDescType>({
+    name: '',
+    k8sName: undefined,
+    description: '',
+  });
   const [host, setHost] = React.useState('');
   const [port, setPort] = React.useState('');
   const [username, setUsername] = React.useState('');
   const [password, setPassword] = React.useState('');
   const [database, setDatabase] = React.useState('');
-  const [isNameTouched, setIsNameTouched] = React.useState(false);
   const [isHostTouched, setIsHostTouched] = React.useState(false);
   const [isPortTouched, setIsPortTouched] = React.useState(false);
   const [isUsernameTouched, setIsUsernameTouched] = React.useState(false);
@@ -46,13 +51,16 @@ const CreateModal: React.FC<CreateModalProps> = ({ isOpen, onClose, refresh }) =
   const onBeforeClose = () => {
     setIsSubmitting(false);
     setError(undefined);
-    setName('');
+    setNameDesc({
+      name: '',
+      k8sName: undefined,
+      description: '',
+    });
     setHost('');
     setPort('');
     setUsername('');
     setPassword('');
     setDatabase('');
-    setIsNameTouched(false);
     setIsHostTouched(false);
     setIsPortTouched(false);
     setIsUsernameTouched(false);
@@ -69,8 +77,12 @@ const CreateModal: React.FC<CreateModalProps> = ({ isOpen, onClose, refresh }) =
       apiVersion: `${ModelRegistryModel.apiGroup}/${ModelRegistryModel.apiVersion}`,
       kind: 'ModelRegistry',
       metadata: {
-        name,
+        name: nameDesc.k8sName || translateDisplayNameForK8s(nameDesc.name),
         namespace: MODEL_REGISTRY_DEFAULT_NAMESPACE,
+        annotations: {
+          'openshift.io/description': nameDesc.description,
+          'openshift.io/display-name': nameDesc.name.trim(),
+        },
       },
       spec: {
         grpc: {
@@ -104,7 +116,7 @@ const CreateModal: React.FC<CreateModalProps> = ({ isOpen, onClose, refresh }) =
 
   const canSubmit = () =>
     !isSubmitting &&
-    isValidK8sName(name) &&
+    isValidK8sName(nameDesc.k8sName || translateDisplayNameForK8s(nameDesc.name)) &&
     hasContent(host) &&
     hasContent(password) &&
     hasContent(port) &&
@@ -138,26 +150,15 @@ const CreateModal: React.FC<CreateModalProps> = ({ isOpen, onClose, refresh }) =
       }
     >
       <Form>
-        <FormGroup label="Name" isRequired fieldId="mr-name">
-          <TextInput
-            isRequired
-            type="text"
-            id="mr-name"
-            name="mr-name"
-            value={name}
-            onBlur={() => setIsNameTouched(true)}
-            onChange={(_e, value) => setName(value)}
-            validated={isNameTouched && !isValidK8sName(name) ? 'error' : 'default'}
-          />
-          {isNameTouched && !isValidK8sName(name) && (
-            <HelperText>
-              <HelperTextItem variant="error" data-testid="mr-name-error">
-                {`Must consist of lower case alphanumeric characters or '-', and must start and end
-                with an alphanumeric character`}
-              </HelperTextItem>
-            </HelperText>
-          )}
-        </FormGroup>
+        <NameDescriptionField
+          nameFieldId="mr-name"
+          descriptionFieldId="mr-description"
+          data={nameDesc}
+          showK8sName
+          setData={(value) => {
+            setNameDesc(value);
+          }}
+        />
         <FormSection
           title={
             <>

diff --git a/frontend/src/pages/modelRegistrySettings/ModelRegistriesTableRow.tsx b/frontend/src/pages/modelRegistrySettings/ModelRegistriesTableRow.tsx
@@ -2,6 +2,7 @@ import React from 'react';
 import { ActionsColumn, Td, Tr } from '@patternfly/react-table';
 import { Link } from 'react-router-dom';
 import { ModelRegistryKind } from '~/k8sTypes';
+import ResourceNameTooltip from '~/components/ResourceNameTooltip';
 import ViewDatabaseConfigModal from './ViewDatabaseConfigModal';
 import DeleteModelRegistryModal from './DeleteModelRegistryModal';
 
@@ -19,7 +20,16 @@ const ModelRegistriesTableRow: React.FC<ModelRegistriesTableRowProps> = ({
   return (
     <>
       <Tr>
-        <Td dataLabel="Model registry name">{mr.metadata.name}</Td>
+        <Td dataLabel="Model registry name">
+          <ResourceNameTooltip resource={mr}>
+            <strong>
+              {mr.metadata.annotations?.['openshift.io/display-name'] || mr.metadata.name}
+            </strong>
+          </ResourceNameTooltip>
+          {mr.metadata.annotations?.['openshift.io/description'] && (
+            <p>{mr.metadata.annotations['openshift.io/description']}</p>
+          )}
+        </Td>
         <Td modifier="fitContent">
           <Link
             aria-label={`Manage permissions for model registry ${mr.metadata.name}`}