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

feat: modify behaviours for the enable button for NIM. #3558

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
97e4f94
Fix the enable button issue for NIM
yzhao583 Dec 9, 2024
7043b7e
Fix lint issue
yzhao583 Dec 9, 2024
80c69c3
Merge branch 'main' into NVPE-37-fix-the-enable-button-loading-issue
yzhao583 Dec 9, 2024
fbe5939
Fix lint issue
yzhao583 Dec 10, 2024
9dcb27f
Merge branch 'main' into NVPE-37-fix-the-enable-button-loading-issue
yzhao583 Dec 10, 2024
75e509b
When enable NIM, check the APIKeyValidation condition, if it is True,…
yzhao583 Dec 16, 2024
86947ef
Merge branch 'NVPE-37-fix-the-enable-button-loading-issue' of https:/…
yzhao583 Dec 16, 2024
c2fb70f
Fix lint issue
yzhao583 Dec 16, 2024
e8e6cc3
Add refreshRate
yzhao583 Dec 16, 2024
509fa1a
Reslove conflict
yzhao583 Dec 18, 2024
919f529
Fix grammar issue
yzhao583 Dec 18, 2024
87b1ce9
Fix account and secret update issue
yzhao583 Dec 18, 2024
e21fd83
Merge branch 'main' into NVPE-37-fix-the-enable-button-loading-issue
yzhao583 Dec 19, 2024
cbfd6d9
Merge branch 'main' into NVPE-37-fix-the-enable-button-loading-issue
yzhao583 Dec 23, 2024
2c52889
Merge branch 'NVPE-37-fix-the-enable-button-loading-issue' of https:/…
yzhao583 Dec 23, 2024
257b83c
Change variablesValidationStatus to enum
yzhao583 Dec 23, 2024
b40e336
Merge branch 'main' into NVPE-37-fix-the-enable-button-loading-issue
yzhao583 Dec 27, 2024
654f82f
Merge branch 'main' into NVPE-37-fix-the-enable-button-loading-issue
yzhao583 Jan 2, 2025
580b6e2
Merge branch 'main' into NVPE-37-fix-the-enable-button-loading-issue
yzhao583 Jan 3, 2025
f5323bd
modify the related logic to ensure the secret is modified before the …
yzhao583 Jan 3, 2025
31af752
Merge branch 'main' into NVPE-37-fix-the-enable-button-loading-issue
yzhao583 Jan 8, 2025
edf7a5c
Compare timestamps to see if backend updated conditions of the accoun…
yzhao583 Jan 8, 2025
41f4197
Make variablesValidationStatus and variablesValidationTimestamp optional
yzhao583 Jan 9, 2025
dd60b29
fix api key update issue and change the initial value of variablesVal…
yzhao583 Jan 10, 2025
d1e042e
Merge branch 'main' into NVPE-37-fix-the-enable-button-loading-issue
yzhao583 Jan 13, 2025
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
66 changes: 44 additions & 22 deletions backend/src/routes/api/integrations/nim/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,15 @@ import { FastifyReply, FastifyRequest } from 'fastify';
import { secureAdminRoute } from '../../../../utils/route-security';
import { KubeFastifyInstance } from '../../../../types';
import { isString } from 'lodash';
import { createNIMAccount, getNIMAccount, isAppEnabled, manageNIMSecret } from './nimUtils';
import { VariablesValidationStatus } from '../../../../types';
import {
createNIMAccount,
manageNIMSecret,
getNIMAccount,
apiKeyValidationStatus,
apiKeyValidationTimestamp,
isAppEnabled,
} from './nimUtils';

module.exports = async (fastify: KubeFastifyInstance) => {
const PAGE_NOT_FOUND_MESSAGE = '404 page not found';
Expand All @@ -15,11 +23,27 @@ module.exports = async (fastify: KubeFastifyInstance) => {
if (response) {
// Installed
const isEnabled = isAppEnabled(response);
reply.send({ isInstalled: true, isEnabled: isEnabled, canInstall: false, error: '' });
const keyValidationStatus: string = apiKeyValidationStatus(response);
const keyValidationTimestamp: string = apiKeyValidationTimestamp(response);
reply.send({
isInstalled: true,
isEnabled: isEnabled,
variablesValidationStatus: keyValidationStatus,
variablesValidationTimestamp: keyValidationTimestamp,
canInstall: !isEnabled,
error: '',
});
} else {
// Not installed
fastify.log.info(`NIM account does not exist`);
reply.send({ isInstalled: false, isEnabled: false, canInstall: true, error: '' });
reply.send({
isInstalled: false,
isEnabled: false,
variablesValidationStatus: VariablesValidationStatus.UNKNOWN,
variablesValidationTimestamp: '',
canInstall: true,
error: '',
});
}
})
.catch((e) => {
Expand All @@ -33,6 +57,8 @@ module.exports = async (fastify: KubeFastifyInstance) => {
reply.send({
isInstalled: false,
isEnabled: false,
variablesValidationStatus: VariablesValidationStatus.UNKNOWN,
variablesValidationTimestamp: '',
canInstall: false,
error: 'NIM not installed',
});
Expand All @@ -41,7 +67,9 @@ module.exports = async (fastify: KubeFastifyInstance) => {
fastify.log.error(`An unexpected error occurred: ${e.response.body?.message}`);
reply.send({
isInstalled: false,
isAppEnabled: false,
isEnabled: false,
variablesValidationStatus: VariablesValidationStatus.UNKNOWN,
variablesValidationTimestamp: '',
canInstall: false,
error: 'An unexpected error occurred. Please try again later.',
});
Expand All @@ -66,24 +94,18 @@ module.exports = async (fastify: KubeFastifyInstance) => {
// Ensure the account exists
try {
const account = await getNIMAccount(fastify);
if (!account) {
const response = await createNIMAccount(fastify);
const isEnabled = isAppEnabled(response);
reply.send({
isInstalled: true,
isEnabled: isEnabled,
canInstall: false,
error: '',
});
} else {
const isEnabled = isAppEnabled(account);
reply.send({
isInstalled: true,
isEnabled: isEnabled,
canInstall: false,
error: '',
});
}
const nimAccount = !account ? await createNIMAccount(fastify) : account;
const isEnabled = isAppEnabled(nimAccount);
const keyValidationStatus: string = apiKeyValidationStatus(nimAccount);
const keyValidationTimeStamp: string = apiKeyValidationTimestamp(nimAccount);
reply.send({
isInstalled: true,
isEnabled: isEnabled,
variablesValidationStatus: keyValidationStatus,
variablesValidationTimestamp: keyValidationTimeStamp,
canInstall: !isEnabled,
error: '',
});
} catch (accountError: any) {
const message = `Failed to create or retrieve NIM account: ${accountError.response?.body?.message}`;
fastify.log.error(message);
Expand Down
12 changes: 12 additions & 0 deletions backend/src/routes/api/integrations/nim/nimUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,18 @@ import { KubeFastifyInstance, NIMAccountKind, SecretKind } from '../../../../typ
const NIM_SECRET_NAME = 'nvidia-nim-access';
const NIM_ACCOUNT_NAME = 'odh-nim-account';

export const apiKeyValidationTimestamp = (app: NIMAccountKind): string => {
const conditions = app?.status?.conditions || [];
const apiKeyCondition = conditions.find((condition) => condition.type === 'APIKeyValidation');
return apiKeyCondition?.lastTransitionTime || '';
};

export const apiKeyValidationStatus = (app: NIMAccountKind): string => {
const conditions = app?.status?.conditions || [];
const apiKeyCondition = conditions.find((condition) => condition.type === 'APIKeyValidation');
return apiKeyCondition?.status || 'Unknown';
};

export const isAppEnabled = (app: NIMAccountKind): boolean => {
const conditions = app?.status?.conditions || [];
return (
Expand Down
6 changes: 6 additions & 0 deletions backend/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1249,6 +1249,12 @@ export enum ServiceAddressAnnotation {
EXTERNAL_GRPC = 'routing.opendatahub.io/external-address-grpc',
}

export enum VariablesValidationStatus {
UNKNOWN = 'Unknown',
FAILED = 'False',
SUCCESS = 'True',
}

export type NIMAccountKind = K8sResourceCommon & {
metadata: {
name: string;
Expand Down
31 changes: 18 additions & 13 deletions frontend/src/components/OdhAppCard.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import { useAppContext } from '~/app/AppContext';
import { useAppDispatch } from '~/redux/hooks';
import { SupportedArea, useIsAreaAvailable } from '~/concepts/areas';
import { isInternalRouteIntegrationsApp } from '~/utilities/utils';
import { useQuickStartCardSelected } from './useQuickStartCardSelected';
import SupportedAppTitle from './SupportedAppTitle';
import BrandImage from './BrandImage';
Expand Down Expand Up @@ -149,20 +150,24 @@
setEnableOpen(true);
}}
>
here
here.
</Button>
. To remove card click&nbsp;
<Button
isInline
variant="link"
onClick={() => {
hide();
removeApplication();
}}
>
here
</Button>
.
{!isInternalRouteIntegrationsApp(odhApp.spec.internalRoute) ? (
emilys314 marked this conversation as resolved.
Show resolved Hide resolved
<>

Check warning on line 156 in frontend/src/components/OdhAppCard.tsx

View check run for this annotation

Codecov / codecov/patch

frontend/src/components/OdhAppCard.tsx#L156

Added line #L156 was not covered by tests
To remove card click&nbsp;
<Button
isInline
variant="link"
onClick={() => {
hide();
removeApplication();

Check warning on line 163 in frontend/src/components/OdhAppCard.tsx

View check run for this annotation

Codecov / codecov/patch

frontend/src/components/OdhAppCard.tsx#L161-L163

Added lines #L161 - L163 were not covered by tests
}}
>
here
</Button>
.
</>
) : null}

Check warning on line 170 in frontend/src/components/OdhAppCard.tsx

View check run for this annotation

Codecov / codecov/patch

frontend/src/components/OdhAppCard.tsx#L170

Added line #L170 was not covered by tests
</div>
);

Expand Down
30 changes: 20 additions & 10 deletions frontend/src/pages/exploreApplication/EnableModal.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
import { Alert, Button, Form, FormAlert, Spinner, TextInputTypes } from '@patternfly/react-core';
import { Modal, ModalVariant } from '@patternfly/react-core/deprecated';
import { ExternalLinkAltIcon } from '@patternfly/react-icons';
import { isEmpty, values } from 'lodash-es';
import { OdhApplication } from '~/types';
import { EnableApplicationStatus, useEnableApplication } from '~/utilities/useEnableApplication';
import { asEnumMember } from '~/utilities/utils';
Expand All @@ -18,6 +19,10 @@
const [postError, setPostError] = React.useState('');
const [validationInProgress, setValidationInProgress] = React.useState(false);
const [enableValues, setEnableValues] = React.useState<{ [key: string]: string }>({});
const isEnableValuesHasEmptyValue = React.useMemo(
() => isEmpty(enableValues) || values(enableValues).some((val) => isEmpty(val)),
[enableValues],
);
const [validationStatus, validationErrorMessage] = useEnableApplication(
validationInProgress,
selectedApp.metadata.name,
Expand All @@ -44,6 +49,12 @@
setValidationInProgress(true);
};

const handleClose = React.useCallback(() => {
setEnableValues({});
setPostError('');
onClose();

Check warning on line 55 in frontend/src/pages/exploreApplication/EnableModal.tsx

View check run for this annotation

Codecov / codecov/patch

frontend/src/pages/exploreApplication/EnableModal.tsx#L53-L55

Added lines #L53 - L55 were not covered by tests
}, [onClose]);

React.useEffect(() => {
if (validationInProgress && validationStatus === EnableApplicationStatus.SUCCESS) {
setValidationInProgress(false);
Expand All @@ -53,13 +64,19 @@
selectedApp.spec.shownOnEnabledPage = true;
/* eslint-enable no-param-reassign */

onClose();
handleClose();

Check warning on line 67 in frontend/src/pages/exploreApplication/EnableModal.tsx

View check run for this annotation

Codecov / codecov/patch

frontend/src/pages/exploreApplication/EnableModal.tsx#L67

Added line #L67 was not covered by tests
}
if (validationInProgress && validationStatus === EnableApplicationStatus.FAILED) {
setValidationInProgress(false);
setPostError(validationErrorMessage);
}
}, [onClose, selectedApp.spec, validationErrorMessage, validationInProgress, validationStatus]);
}, [
handleClose,
selectedApp.spec,
validationErrorMessage,
validationInProgress,
validationStatus,
]);

React.useEffect(() => {
if (shown) {
Expand All @@ -71,13 +88,6 @@
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [shown]);

const handleClose = () => {
if (!validationInProgress) {
setEnableValues({});
}
onClose();
};

if (!selectedApp.spec.enable || !shown) {
return null;
}
Expand All @@ -97,7 +107,7 @@
key="confirm"
variant="primary"
onClick={onDoEnableApp}
isDisabled={validationInProgress}
isDisabled={validationInProgress || isEnableValuesHasEmptyValue}

Check warning on line 110 in frontend/src/pages/exploreApplication/EnableModal.tsx

View check run for this annotation

Codecov / codecov/patch

frontend/src/pages/exploreApplication/EnableModal.tsx#L110

Added line #L110 was not covered by tests
>
{enable.actionLabel}
</Button>,
Expand Down
26 changes: 16 additions & 10 deletions frontend/src/pages/exploreApplication/GetStartedPanel.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
DrawerPanelBody,
DrawerPanelContent,
Tooltip,
Skeleton,
Content,
} from '@patternfly/react-core';
import { ExternalLinkAltIcon } from '@patternfly/react-icons';
Expand All @@ -38,35 +39,40 @@
const GetStartedPanel: React.FC<GetStartedPanelProps> = ({ selectedApp, onClose, onEnable }) => {
const { dashboardConfig } = useAppContext();
const { enablement } = dashboardConfig.spec.dashboardConfig;
const [{ isInstalled, canInstall, error }, loaded] = useIntegratedAppStatus(selectedApp);
const [{ isEnabled, canInstall, error }, loaded] = useIntegratedAppStatus(selectedApp);
const { isAdmin } = useUser();

if (!selectedApp) {
return null;
}

const renderEnableButton = () => {
if (!selectedApp.spec.enable || selectedApp.spec.isEnabled || isInstalled || !isAdmin) {
if (!selectedApp.spec.enable || selectedApp.spec.isEnabled || isEnabled || !isAdmin) {
return null;
}

if (!loaded && !error) {
return <Skeleton style={{ minWidth: 100 }} fontSize="3xl" />;

Check warning on line 55 in frontend/src/pages/exploreApplication/GetStartedPanel.tsx

View check run for this annotation

Codecov / codecov/patch

frontend/src/pages/exploreApplication/GetStartedPanel.tsx#L54-L55

Added lines #L54 - L55 were not covered by tests
}

const button = (
<Button
variant={ButtonVariant.secondary}
onClick={onEnable}
isDisabled={!enablement || !canInstall}
isLoading={!loaded && !error}
>
Enable
</Button>
);
if (enablement) {
return button;

if (!enablement || !canInstall) {
return (

Check warning on line 69 in frontend/src/pages/exploreApplication/GetStartedPanel.tsx

View check run for this annotation

Codecov / codecov/patch

frontend/src/pages/exploreApplication/GetStartedPanel.tsx#L68-L69

Added lines #L68 - L69 were not covered by tests
<Tooltip content="This feature has been disabled by an administrator.">
<span>{button}</span>
</Tooltip>
);
}
return (
<Tooltip content="This feature has been disabled by an administrator.">
<span>{button}</span>
</Tooltip>
);
return button;

Check warning on line 75 in frontend/src/pages/exploreApplication/GetStartedPanel.tsx

View check run for this annotation

Codecov / codecov/patch

frontend/src/pages/exploreApplication/GetStartedPanel.tsx#L75

Added line #L75 was not covered by tests
};

return (
Expand Down
15 changes: 12 additions & 3 deletions frontend/src/pages/exploreApplication/useIntegratedAppStatus.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,13 @@
import * as React from 'react';
import { IntegrationAppStatus, OdhApplication } from '~/types';
import { IntegrationAppStatus, OdhApplication, VariablesValidationStatus } from '~/types';
import useFetchState, { FetchState, NotReadyError } from '~/utilities/useFetchState';
import { getIntegrationAppEnablementStatus } from '~/services/integrationAppService';
import { isIntegrationApp } from '~/utilities/utils';
import { useAppSelector } from '~/redux/hooks';

export const useIntegratedAppStatus = (app?: OdhApplication): FetchState<IntegrationAppStatus> => {
const forceUpdate = useAppSelector((state) => state.forceComponentsUpdate);
yzhao583 marked this conversation as resolved.
Show resolved Hide resolved

const callback = React.useCallback(() => {
if (!app) {
return Promise.reject(new NotReadyError('Need an app to check'));
Expand All @@ -15,21 +18,27 @@ export const useIntegratedAppStatus = (app?: OdhApplication): FetchState<Integra
isInstalled: false,
isEnabled: false,
canInstall: true,
variablesValidationStatus: VariablesValidationStatus.UNKNOWN,
variablesValidationTimestamp: '',
error: '',
});
}

return getIntegrationAppEnablementStatus(app.spec.internalRoute);
}, [app]);
}, [app, forceUpdate]); // eslint-disable-line react-hooks/exhaustive-deps

return useFetchState(
callback,
{
isInstalled: false,
isEnabled: false,
canInstall: false,
variablesValidationStatus: VariablesValidationStatus.UNKNOWN,
variablesValidationTimestamp: '',
error: '',
},
{ initialPromisePurity: true },
{
initialPromisePurity: true,
},
);
};
8 changes: 8 additions & 0 deletions frontend/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -668,9 +668,17 @@ export type KeyValuePair = {
value: string;
};

export enum VariablesValidationStatus {
UNKNOWN = 'Unknown',
FAILED = 'False',
SUCCESS = 'True',
}

export type IntegrationAppStatus = {
isInstalled: boolean;
isEnabled: boolean;
canInstall: boolean;
variablesValidationStatus?: VariablesValidationStatus;
variablesValidationTimestamp?: string;
error: string;
};
Loading
Loading