Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prefill URI connections and fix prefill deploy model form from version #3590

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
/* eslint-disable camelcase */
import {
mockCustomSecretK8sResource,
mockDscStatus,
mockK8sResourceList,
mockProjectK8sResource,
Expand Down Expand Up @@ -32,7 +33,10 @@ import { ServingRuntimePlatform } from '~/types';
import { kserveModal } from '~/__tests__/cypress/cypress/pages/modelServing';
import { mockModelArtifact } from '~/__mocks__/mockModelArtifact';
import { mockNimAccount } from '~/__mocks__/mockNimAccount';
import { mockConnectionTypeConfigMap } from '~/__mocks__/mockConnectionType';
import {
mockConnectionTypeConfigMap,
mockModelServingFields,
} from '~/__mocks__/mockConnectionType';

const MODEL_REGISTRY_API_VERSION = 'v1alpha3';

Expand Down Expand Up @@ -61,6 +65,7 @@ const initIntercepts = ({
modelVersions = [
mockModelVersion({ id: '1', name: 'test model version' }),
mockModelVersion({ id: '2', name: modelVersionMocked2.name }),
mockModelVersion({ id: '3', name: 'test model version 2' }),
],
modelMeshInstalled = true,
kServeInstalled = true,
Expand Down Expand Up @@ -172,6 +177,20 @@ const initIntercepts = ({
mockModelArtifactList({}),
);

cy.interceptOdh(
`GET /api/service/modelregistry/:serviceName/api/model_registry/:apiVersion/model_versions/:modelVersionId/artifacts`,
{
path: {
serviceName: 'modelregistry-sample',
apiVersion: MODEL_REGISTRY_API_VERSION,
modelVersionId: 3,
},
},
mockModelArtifactList({
items: [mockModelArtifact({ uri: 'https://demo-models/some-path.zip' })],
}),
);

cy.interceptOdh(
`GET /api/service/modelregistry/:serviceName/api/model_registry/:apiVersion/model_versions/:modelVersionId/artifacts`,
{
Expand Down Expand Up @@ -227,6 +246,13 @@ const initIntercepts = ({
},
],
}),
mockConnectionTypeConfigMap({
name: 's3',
displayName: 'S3 compatible object storage - v1',
description: 'description 2',
category: ['existing-category'],
fields: mockModelServingFields,
}),
]);

cy.interceptK8sList(NIMAccountModel, mockK8sResourceList([mockNimAccount({})]));
Expand Down Expand Up @@ -274,7 +300,7 @@ describe('Deploy model version', () => {
cy.findByText('Cannot deploy the model until you configure a model server').should('exist');
});

it('Pre-fill deployment information on KServe modal', () => {
it('Selects Create Connection in case of no matching connections', () => {
initIntercepts({});
cy.interceptK8sList(
SecretModel,
Expand Down Expand Up @@ -309,11 +335,14 @@ describe('Deploy model version', () => {
).should('exist');

// Validate connection section
kserveModal.findExistingConnectionOption().should('be.checked');
kserveModal.findLocationPathInput().should('exist');
kserveModal.findNewConnectionOption().should('be.checked');
kserveModal.findLocationBucketInput().should('have.value', 'test-bucket');
kserveModal.findLocationEndpointInput().should('have.value', 'test-endpoint');
kserveModal.findLocationRegionInput().should('have.value', 'test-region');
kserveModal.findLocationPathInput().should('have.value', 'demo-models/test-path');
});

it('One match connection on KServe modal', () => {
it('Prefills when there is one s3 matching connection', () => {
initIntercepts({});
cy.interceptK8sList(
SecretModel,
Expand Down Expand Up @@ -342,12 +371,78 @@ describe('Deploy model version', () => {

// Validate connection section
kserveModal.findExistingConnectionOption().should('be.checked');
kserveModal.findExistingConnectionSelectValueField().click();
kserveModal.selectExistingConnectionSelectOptionByResourceName();
kserveModal.findLocationPathInput().type('test-model/');
kserveModal.findNewConnectionOption().click();
kserveModal.findExistingConnectionSelect().should('have.attr', 'disabled');
kserveModal.findConnectionNameInput().type('Test Name');
kserveModal.findConnectionFieldInput().type('https://test');
kserveModal.findExistingConnectionSelectValueField().should('have.value', 'Test Secret');
kserveModal.findLocationPathInput().should('have.value', 'demo-models/test-path');
});

it('Prefills when there is one URI matching connection', () => {
initIntercepts({});
cy.interceptK8sList(
SecretModel,
mockK8sResourceList([
mockCustomSecretK8sResource({
namespace: 'kserve-project',
name: 'test-secret',
annotations: {
'opendatahub.io/connection-type': 'uri-v1',
'openshift.io/display-name': 'Test Secret',
},
data: { URI: 'aHR0cHM6Ly9kZW1vLW1vZGVscy9zb21lLXBhdGguemlw' },
}),
]),
);

cy.visit(`/modelRegistry/modelregistry-sample/registeredModels/1/versions`);
const modelVersionRow = modelRegistry.getModelVersionRow('test model version 2');
modelVersionRow.findKebabAction('Deploy').click();
modelVersionDeployModal.selectProjectByName('KServe project');

// Validate connection section
kserveModal.findExistingConnectionOption().should('be.checked');
kserveModal.findExistingConnectionSelectValueField().should('have.value', 'Test Secret');
});

it('Selects existing connection when there are 2 matching connections', () => {
initIntercepts({});
cy.interceptK8sList(
SecretModel,
mockK8sResourceList([
mockCustomSecretK8sResource({
namespace: 'kserve-project',
name: 'test-secret',
annotations: {
'opendatahub.io/connection-type': 'uri-v1',
'openshift.io/display-name': 'Test Secret',
},
data: { URI: 'aHR0cHM6Ly9kZW1vLW1vZGVscy9zb21lLXBhdGguemlw' },
}),
mockCustomSecretK8sResource({
namespace: 'kserve-project',
name: 'test-secret-2',
annotations: {
'opendatahub.io/connection-type': 'uri-v1',
'openshift.io/display-name': 'Test Secret Match 2',
},
data: { URI: 'aHR0cHM6Ly9kZW1vLW1vZGVscy9zb21lLXBhdGguemlw' },
}),
]),
);

cy.visit(`/modelRegistry/modelregistry-sample/registeredModels/1/versions`);
const modelVersionRow = modelRegistry.getModelVersionRow('test model version 2');
modelVersionRow.findKebabAction('Deploy').click();
modelVersionDeployModal.selectProjectByName('KServe project');

// Validate connection section
kserveModal.findExistingConnectionOption().should('be.checked');
kserveModal.findExistingConnectionSelectValueField().should('be.empty');
kserveModal
.findExistingConnectionSelectValueField()
.findSelectOption('Test Secret Recommended Type: URI - v1')
.should('exist');
kserveModal
.findExistingConnectionSelectValueField()
.findSelectOption('Test Secret Match 2 Recommended Type: URI - v1')
.should('exist');
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -175,9 +175,9 @@ describe('Register model page', () => {
registerModelPage.findObjectStorageAutofillButton().click();
registerModelPage
.findConnectionSelector()
.contains('Select a project to view its available data connections');
.contains('Select a project to view its available connections');
registerModelPage.projectDropdown.openAndSelectItem('Test Project', true);
registerModelPage.findConnectionSelector().contains('No available data connections');
registerModelPage.findConnectionSelector().contains('No available connections');
});

it('Project selection with connections displays connections and fills form', () => {
Expand All @@ -190,9 +190,9 @@ describe('Register model page', () => {
registerModelPage.findObjectStorageAutofillButton().click();
registerModelPage
.findConnectionSelector()
.contains('Select a project to view its available data connections');
.contains('Select a project to view its available connections');
registerModelPage.projectDropdown.openAndSelectItem('Test Project', true);
registerModelPage.findConnectionSelector().contains('Select data connection');
registerModelPage.findConnectionSelector().contains('Select connection');
registerModelPage.findConnectionSelector().findDropdownItem('Test Secret').click();
registerModelPage.findAutofillButton().click();
registerModelPage.findConnectionAutofillModal().should('not.exist');
Expand Down
10 changes: 8 additions & 2 deletions frontend/src/components/TypeaheadSelect.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ import {
FormHelperText,
HelperTextItem,
HelperText,
FlexItem,
Flex,
} from '@patternfly/react-core';
import { TimesIcon } from '@patternfly/react-icons';
import TruncatedText from '~/components/TruncatedText';
Expand All @@ -30,6 +32,7 @@ export interface TypeaheadSelectOption extends Omit<SelectOptionProps, 'content'
value: string | number;
/** Indicator for option being selected */
isSelected?: boolean;
dropdownLabel?: React.ReactNode;
}

export interface TypeaheadSelectProps extends Omit<SelectProps, 'toggle' | 'onSelect'> {
Expand Down Expand Up @@ -438,15 +441,18 @@ const TypeaheadSelect: React.FunctionComponent<TypeaheadSelectProps> = ({
>
<SelectList>
{filteredSelections.map((option, index) => {
const { content, value, ...optionProps } = option;
const { content, value, dropdownLabel, ...optionProps } = option;
return (
<SelectOption
key={value}
value={value}
isFocused={focusedItemIndex === index}
{...optionProps}
>
{content}
<Flex>
<FlexItem>{content}</FlexItem>
<FlexItem>{dropdownLabel}</FlexItem>
</Flex>
</SelectOption>
);
})}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
import { act } from 'react';
import { standardUseFetchState, testHook } from '~/__tests__/unit/testUtils/hooks';
import { fetchConnectionType } from '~/services/connectionTypesService';
import { useConnectionType } from '~/concepts/connectionTypes/useConnectionType';
import { mockConnectionTypeConfigMapObj } from '~/__mocks__/mockConnectionType';

jest.mock('~/services/connectionTypesService', () => ({
fetchConnectionType: jest.fn(),
}));

const mockFetchConnectionType = jest.mocked(fetchConnectionType);

describe('useConnectionType', () => {
it('should return connection type config map object when name is given', async () => {
const configMapObjMock = mockConnectionTypeConfigMapObj({ name: 'test-connection-type' });
mockFetchConnectionType.mockResolvedValue(configMapObjMock);
const renderResult = testHook(useConnectionType)('test-connection-type');
expect(mockFetchConnectionType).toHaveBeenCalledTimes(1);
expect(renderResult).hookToStrictEqual(standardUseFetchState(undefined, false, undefined));
expect(renderResult).hookToHaveUpdateCount(1);

// wait for update
await renderResult.waitForNextUpdate();
expect(mockFetchConnectionType).toHaveBeenCalledTimes(1);
expect(renderResult).hookToStrictEqual(standardUseFetchState(configMapObjMock, true));
expect(renderResult).hookToHaveUpdateCount(2);
expect(renderResult).hookToBeStable([false, false, true, true]);

// refresh
await act(() => renderResult.result.current[3]());
expect(mockFetchConnectionType).toHaveBeenCalledTimes(2);
expect(renderResult).hookToHaveUpdateCount(3);
expect(renderResult).hookToBeStable([false, true, true, true]);
});

it('should handle errors when name is empty', async () => {
mockFetchConnectionType.mockRejectedValue(new Error('No connection type name'));
const renderResult = testHook(useConnectionType)('test');
expect(renderResult).hookToStrictEqual(standardUseFetchState(undefined));
expect(renderResult).hookToHaveUpdateCount(1);
// wait for update
await renderResult.waitForNextUpdate();
expect(renderResult).hookToStrictEqual(
standardUseFetchState(undefined, false, new Error('No connection type name')),
);
expect(mockFetchConnectionType).toHaveBeenCalledTimes(1);
expect(renderResult).hookToHaveUpdateCount(2);
expect(renderResult).hookToBeStable([true, true, false, true]);
// refresh
mockFetchConnectionType.mockRejectedValue(new Error('No connection type name-error2'));
await act(() => renderResult.result.current[3]());
expect(mockFetchConnectionType).toHaveBeenCalledTimes(2);
expect(renderResult).hookToStrictEqual(
standardUseFetchState(undefined, false, new Error('No connection type name-error2')),
);
expect(renderResult).hookToHaveUpdateCount(3);
expect(renderResult).hookToBeStable([true, true, false, true]);
});
});
44 changes: 44 additions & 0 deletions frontend/src/concepts/connectionTypes/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ import {
ConnectionTypeValueType,
} from '~/concepts/connectionTypes/types';
import { enumIterator } from '~/utilities/utils';
import { AWSDataEntry, EnvVariableDataEntry } from '~/pages/projects/types';
import { AwsKeys } from '~/pages/projects/dataConnections/const';

export const isConnectionTypeDataFieldType = (
type: ConnectionTypeFieldTypeUnion | string,
Expand Down Expand Up @@ -222,6 +224,27 @@ export const getDefaultValues = (
return defaults;
};

export const getMRConnectionValues = (
connectionValues: EnvVariableDataEntry[] | string,
): { [key: string]: ConnectionTypeValueType } => {
const defaults: {
[key: string]: ConnectionTypeValueType;
} = {};
if (typeof connectionValues !== 'string') {
connectionValues.map((connectionValue) => {
if (connectionValue.key !== 'Name') {
defaults[connectionValue.key] = connectionValue.value;
}
return defaults;
});
return defaults;
}
if (typeof connectionValues === 'string') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: This would be easier for the next person to grok if it was an if / else branch IMO

defaults.URI = connectionValues;
}
return defaults;
};

export const withRequiredFields = (
connectionType?: ConnectionTypeConfigMapObj,
envVars?: string[],
Expand Down Expand Up @@ -390,3 +413,24 @@ export const validateEnvVarName = (name: string): string | undefined => {
}
return undefined;
};

export const convertObjectStorageSecretData = (dataConnection: Connection): AWSDataEntry => {
let convertedData: { key: AwsKeys; value: string }[] = [];
const secretData = dataConnection.data;
if (secretData) {
convertedData = Object.values(AwsKeys)
.filter((key) => key !== AwsKeys.NAME)
.map((key: AwsKeys) => ({
key,
value: secretData[key] ? window.atob(secretData[key]) : '',
}));
}
const convertedSecret: AWSDataEntry = [
{
key: AwsKeys.NAME,
value: getDisplayNameFromK8sResource(dataConnection),
},
...convertedData,
];
return convertedSecret;
};
Loading