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

Open
wants to merge 24 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
24 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
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
35 changes: 29 additions & 6 deletions backend/src/routes/api/integrations/nim/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,13 @@ 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 {
createNIMAccount,
manageNIMSecret,
getNIMAccount,
apiKeyValidationStatus,
isAppEnabled,
} from './nimUtils';

module.exports = async (fastify: KubeFastifyInstance) => {
const PAGE_NOT_FOUND_MESSAGE = '404 page not found';
Expand All @@ -15,11 +21,24 @@ 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);
reply.send({
isInstalled: true,
isEnabled: isEnabled,
variablesValidationStatus: keyValidationStatus,
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: '',
canInstall: true,
error: '',
});
}
})
.catch((e) => {
Expand All @@ -33,6 +52,7 @@ module.exports = async (fastify: KubeFastifyInstance) => {
reply.send({
isInstalled: false,
isEnabled: false,
variablesValidationStatus: '',
canInstall: false,
error: 'NIM not installed',
});
Expand All @@ -41,7 +61,8 @@ 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: '',
canInstall: false,
error: 'An unexpected error occurred. Please try again later.',
});
Expand Down Expand Up @@ -72,15 +93,17 @@ module.exports = async (fastify: KubeFastifyInstance) => {
reply.send({
isInstalled: true,
isEnabled: isEnabled,
canInstall: false,
variablesValidationStatus: '',
canInstall: !isEnabled,
error: '',
});
} else {
const isEnabled = isAppEnabled(account);
reply.send({
isInstalled: true,
isEnabled: isEnabled,
canInstall: false,
variablesValidationStatus: '',
canInstall: !isEnabled,
error: '',
});
}
Expand Down
6 changes: 6 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,12 @@ import { KubeFastifyInstance, NIMAccountKind, SecretKind } from '../../../../typ
const NIM_SECRET_NAME = 'nvidia-nim-access';
const NIM_ACCOUNT_NAME = 'odh-nim-account';

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

export const isAppEnabled = (app: NIMAccountKind): boolean => {
const conditions = app?.status?.conditions || [];
return (
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 @@ -154,20 +155,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 161 in frontend/src/components/OdhAppCard.tsx

View check run for this annotation

Codecov / codecov/patch

frontend/src/components/OdhAppCard.tsx#L161

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

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

View check run for this annotation

Codecov / codecov/patch

frontend/src/components/OdhAppCard.tsx#L166-L168

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

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

View check run for this annotation

Codecov / codecov/patch

frontend/src/components/OdhAppCard.tsx#L175

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

Expand Down
32 changes: 22 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,14 @@
setValidationInProgress(true);
};

const handleClose = React.useCallback(() => {
if (!validationInProgress) {
setEnableValues({});
setPostError('');

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();

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

View check run for this annotation

Codecov / codecov/patch

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

Added line #L57 was not covered by tests
}, [validationInProgress, onClose]);

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

onClose();
handleClose();

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

View check run for this annotation

Codecov / codecov/patch

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

Added line #L69 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 +90,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 +109,7 @@
key="confirm"
variant="primary"
onClick={onDoEnableApp}
isDisabled={validationInProgress}
isDisabled={validationInProgress || isEnableValuesHasEmptyValue}

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

View check run for this annotation

Codecov / codecov/patch

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

Added line #L112 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
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ export const useIntegratedAppStatus = (app?: OdhApplication): FetchState<Integra
isInstalled: false,
isEnabled: false,
canInstall: true,
variablesValidationStatus: '',
error: '',
});
}
Expand All @@ -28,8 +29,9 @@ export const useIntegratedAppStatus = (app?: OdhApplication): FetchState<Integra
isInstalled: false,
isEnabled: false,
canInstall: false,
variablesValidationStatus: '',
error: '',
},
{ initialPromisePurity: true },
{ refreshRate: 5000, initialPromisePurity: true },
yzhao583 marked this conversation as resolved.
Show resolved Hide resolved
);
};
1 change: 1 addition & 0 deletions frontend/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -666,5 +666,6 @@ export type IntegrationAppStatus = {
isInstalled: boolean;
isEnabled: boolean;
canInstall: boolean;
variablesValidationStatus: string;
yzhao583 marked this conversation as resolved.
Show resolved Hide resolved
error: string;
};
Loading
Loading