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

Improve username logic and add navbar elements #641

Merged
merged 5 commits into from
Dec 13, 2024
Merged
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
34 changes: 29 additions & 5 deletions clients/ui/api/openapi/mod-arch.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,15 @@ paths:
operationId: healthcheck
summary: HealthCheck
description: HealthCheck endpoint.
/api/v1/config:
summary: Path used to manage Configuration information regarding the UI.
/api/v1/user:
summary: Path used to Retrieve a user based on the header.
description: >-
The REST endpoint/path used pass all the config information needed for the UI.
get:
tags:
- K8SOperation
parameters:
- $ref: "#/components/parameters/kubeflowUserId"
responses:
"200":
$ref: "#/components/responses/ConfigResponse"
Expand All @@ -49,6 +51,8 @@ paths:
get:
tags:
- K8SOperation
parameters:
- $ref: "#/components/parameters/kubeflowUserId"
responses:
"200":
$ref: "#/components/responses/ModelRegistryRespone"
Expand All @@ -68,6 +72,7 @@ paths:
- ModelRegistryService
parameters:
- $ref: "#/components/parameters/modelRegistryName"
- $ref: "#/components/parameters/kubeflowUserId"
responses:
"200":
$ref: "#/components/responses/ModelVersionResponse"
Expand Down Expand Up @@ -112,6 +117,7 @@ paths:
description: Updates an existing `ModelVersion`.
parameters:
- $ref: "#/components/parameters/modelRegistryName"
- $ref: "#/components/parameters/kubeflowUserId"
- name: modelversionId
description: A unique identifier for a `ModelVersion`.
schema:
Expand All @@ -131,6 +137,7 @@ paths:
- $ref: "#/components/parameters/orderBy"
- $ref: "#/components/parameters/sortOrder"
- $ref: "#/components/parameters/nextPageToken"
- $ref: "#/components/parameters/kubeflowUserId"
responses:
"200":
$ref: "#/components/responses/RegisteredModelListResponse"
Expand Down Expand Up @@ -159,6 +166,7 @@ paths:
- ModelRegistryService
parameters:
- $ref: "#/components/parameters/modelRegistryName"
- $ref: "#/components/parameters/kubeflowUserId"
responses:
"201":
$ref: "#/components/responses/RegisteredModelResponse"
Expand All @@ -180,6 +188,7 @@ paths:
- ModelRegistryService
parameters:
- $ref: "#/components/parameters/modelRegistryName"
- $ref: "#/components/parameters/kubeflowUserId"
responses:
"200":
$ref: "#/components/responses/RegisteredModelResponse"
Expand Down Expand Up @@ -224,6 +233,7 @@ paths:
description: Updates an existing `RegisteredModel`.
parameters:
- $ref: "#/components/parameters/modelRegistryName"
- $ref: "#/components/parameters/kubeflowUserId"
- name: registeredmodelId
description: A unique identifier for a `RegisteredModel`.
schema:
Expand All @@ -245,6 +255,7 @@ paths:
- $ref: "#/components/parameters/orderBy"
- $ref: "#/components/parameters/sortOrder"
- $ref: "#/components/parameters/nextPageToken"
- $ref: "#/components/parameters/kubeflowUserId"
responses:
"200":
$ref: "#/components/responses/ArtifactListResponse"
Expand Down Expand Up @@ -274,6 +285,7 @@ paths:
- ModelRegistryService
parameters:
- $ref: "#/components/parameters/modelRegistryName"
- $ref: "#/components/parameters/kubeflowUserId"
responses:
"200":
$ref: "#/components/responses/ArtifactResponse"
Expand Down Expand Up @@ -311,6 +323,7 @@ paths:
- $ref: "#/components/parameters/orderBy"
- $ref: "#/components/parameters/sortOrder"
- $ref: "#/components/parameters/nextPageToken"
- $ref: "#/components/parameters/kubeflowUserId"
responses:
"200":
$ref: "#/components/responses/ModelVersionListResponse"
Expand Down Expand Up @@ -355,6 +368,7 @@ paths:
description: Creates a new instance of a `ModelVersion`.
parameters:
- $ref: "#/components/parameters/modelRegistryName"
- $ref: "#/components/parameters/kubeflowUserId"
- name: registeredmodelId
description: A unique identifier for a `RegisteredModel`.
schema:
Expand All @@ -365,12 +379,16 @@ components:
schemas:
Config:
required:
- username
- userId
- clusterAdmin
type: object
properties:
username:
userId:
type: string
example: username-1
example: [email protected]
clusterAdmin:
type: boolean
example: true
ModelRegistry:
required:
- name
Expand Down Expand Up @@ -1193,6 +1211,12 @@ components:
$ref: "#/components/schemas/ServeModel"
description: A response containing a `ServeModel` entity.
parameters:
kubeflowUserId:
in: header
name: kubeflow-userid
schema:
type: string
required: true
modelRegistryName:
name: modelRegistryName
description: Name of the Model Registry selected.
Expand Down
2 changes: 1 addition & 1 deletion clients/ui/bff/internal/models/health_check.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,5 +7,5 @@ type SystemInfo struct {
type HealthCheckModel struct {
Status string `json:"status"`
SystemInfo SystemInfo `json:"system_info"`
UserID string `json:"user-id"`
UserID string `json:"userId"`
}
4 changes: 2 additions & 2 deletions clients/ui/bff/internal/models/user.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
package models

type User struct {
UserID string `json:"user-id"`
ClusterAdmin bool `json:"cluster-admin"`
UserID string `json:"userId"`
ClusterAdmin bool `json:"clusterAdmin"`
}
14 changes: 14 additions & 0 deletions clients/ui/frontend/src/__mocks__/mockUserSettings.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
import { UserSettings } from '~/shared/types';

type MockUserSettingsType = {
userId?: string;
clusterAdmin?: boolean;
};

export const mockUserSettings = ({
userId = '[email protected]',
clusterAdmin = true,
}: MockUserSettingsType): UserSettings => ({
userId,
clusterAdmin,
});
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import type {
RegisteredModel,
RegisteredModelList,
} from '~/app/types';
import type { UserSettings } from '~/shared/types';

const MODEL_REGISTRY_API_VERSION = 'v1';
export { MODEL_REGISTRY_API_VERSION };
Expand Down Expand Up @@ -102,6 +103,11 @@ declare global {
type: 'GET /api/:apiVersion/model_registry',
options: { path: { apiVersion: string } },
response: ApiResponse<ModelRegistry[]>,
) => Cypress.Chainable<null>) &
((
type: 'GET /api/:apiVersion/user',
options: { path: { apiVersion: string } },
response: ApiResponse<UserSettings>,
) => Cypress.Chainable<null>);
}
}
Expand Down
12 changes: 12 additions & 0 deletions clients/ui/frontend/src/__tests__/cypress/cypress/support/e2e.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,10 @@

import chaiSubset from 'chai-subset';
import '@cypress/code-coverage/support';
import { mockUserSettings } from '~/__mocks__/mockUserSettings';
import 'cypress-mochawesome-reporter/register';
import './commands';
import { MODEL_REGISTRY_API_VERSION } from './commands/api';

chai.use(chaiSubset);

Expand All @@ -28,5 +30,15 @@ beforeEach(() => {
if (Cypress.env('MOCK')) {
// fallback: return 404 for all api requests
cy.intercept({ pathname: '/api/**' }, { statusCode: 404 });

cy.interceptApi(
'GET /api/:apiVersion/user',
{
path: {
apiVersion: MODEL_REGISTRY_API_VERSION,
},
},
mockUserSettings({}),
);
}
});
36 changes: 22 additions & 14 deletions clients/ui/frontend/src/app/App.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,6 @@ import {
Alert,
Bullseye,
Button,
Masthead,
MastheadContent,
MastheadMain,
Page,
PageSection,
Spinner,
Expand All @@ -16,11 +13,12 @@ import {
} from '@patternfly/react-core';
import ToastNotifications from '~/shared/components/ToastNotifications';
import { useSettings } from '~/shared/hooks/useSettings';
import { isMUITheme, Theme } from '~/shared/utilities/const';
import { isMUITheme, Theme, USER_ID } from '~/shared/utilities/const';
import NavSidebar from './NavSidebar';
import AppRoutes from './AppRoutes';
import { AppContext } from './AppContext';
import { ModelRegistrySelectorContextProvider } from './context/ModelRegistrySelectorContext';
import NavBar from './NavBar';

const App: React.FC = () => {
const {
Expand All @@ -30,6 +28,8 @@ const App: React.FC = () => {
loadError: configError,
} = useSettings();

const username = userSettings?.userId;

React.useEffect(() => {
// Apply the theme based on the value of STYLE_THEME
if (isMUITheme()) {
Expand All @@ -39,6 +39,16 @@ const App: React.FC = () => {
}
}, []);

React.useEffect(() => {
// Add the user to localStorage if in PoC
// TODO: [Env Handling] Just add this logic in PoC mode
if (username) {
localStorage.setItem(USER_ID, username);
} else {
localStorage.removeItem(USER_ID);
}
}, [username]);

const contextValue = React.useMemo(
() =>
configSettings && userSettings
Expand Down Expand Up @@ -82,15 +92,6 @@ const App: React.FC = () => {
// Waiting on the API to finish
const loading = !configLoaded || !userSettings || !configSettings || !contextValue;

const masthead = (
<Masthead>
<MastheadMain />
<MastheadContent>
{/* TODO: [Auth-enablement] Add logout and user status once we enable itNavigates to register page from table toolbar */}
</MastheadContent>
</Masthead>
);

return loading ? (
<Bullseye>
<Spinner />
Expand All @@ -99,7 +100,14 @@ const App: React.FC = () => {
<AppContext.Provider value={contextValue}>
<Page
mainContainerId="primary-app-container"
masthead={masthead}
masthead={
<NavBar
username={username}
onLogout={() => {
//TODO: [Auth-enablement] Logout when auth is enabled
}}
/>
}
isManagedSidebar
sidebar={<NavSidebar />}
>
Expand Down
20 changes: 7 additions & 13 deletions clients/ui/frontend/src/app/AppRoutes.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { Navigate, Route, Routes } from 'react-router-dom';
import { NotFound } from './pages/notFound/NotFound';
import ModelRegistrySettingsRoutes from './pages/settings/ModelRegistrySettingsRoutes';
import ModelRegistryRoutes from './pages/modelRegistry/ModelRegistryRoutes';
import { useAppContext } from './AppContext';

export const isNavDataGroup = (navItem: NavDataItem): navItem is NavDataGroup =>
'children' in navItem;
Expand All @@ -22,12 +23,9 @@ export type NavDataGroup = NavDataCommon & {
type NavDataItem = NavDataHref | NavDataGroup;

export const useAdminSettings = (): NavDataItem[] => {
// get auth access for example set admin as true
const isAdmin = true; //this should be a call to getting auth / role access
const { clusterAdmin } = useAppContext().user;

// TODO: [Auth-enablement] Remove the linter skip when we implement authentication
// eslint-disable-next-line @typescript-eslint/no-unnecessary-condition
if (!isAdmin) {
if (!clusterAdmin) {
return [];
}

Expand All @@ -48,20 +46,16 @@ export const useNavData = (): NavDataItem[] => [
];

const AppRoutes: React.FC = () => {
const isAdmin = true;
const { clusterAdmin } = useAppContext().user;

return (
<Routes>
<Route path="/" element={<Navigate to="/modelRegistry" replace />} />
<Route path="/modelRegistry/*" element={<ModelRegistryRoutes />} />
<Route path="*" element={<NotFound />} />
{
// TODO: [Auth-enablement] Remove the linter skip when we implement authentication
// eslint-disable-next-line @typescript-eslint/no-unnecessary-condition
isAdmin && (
<Route path="/modelRegistrySettings/*" element={<ModelRegistrySettingsRoutes />} />
)
}
{clusterAdmin && (
<Route path="/modelRegistrySettings/*" element={<ModelRegistrySettingsRoutes />} />
)}
</Routes>
);
};
Expand Down
Loading
Loading