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

Refactor BYON images table #1506

Merged
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
47 changes: 20 additions & 27 deletions backend/src/routes/api/images/imageUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,6 @@ import {
ImageStream,
TagContent,
KubeFastifyInstance,
BYONImageCreateRequest,
BYONImageUpdateRequest,
BYONImagePackage,
BYONImage,
} from '../../../types';
Expand Down Expand Up @@ -189,24 +187,19 @@ const packagesToString = (packages: BYONImagePackage[]): string => {
return '[]';
};
const mapImageStreamToBYONImage = (is: ImageStream): BYONImage => ({
id: is.metadata.name,
name: is.metadata.annotations['opendatahub.io/notebook-image-name'],
description: is.metadata.annotations['opendatahub.io/notebook-image-desc'],
id: is.metadata.uid,
name: is.metadata.name,
display_name: is.metadata.annotations['opendatahub.io/notebook-image-name'] || is.metadata.name,
description: is.metadata.annotations['opendatahub.io/notebook-image-desc'] || '',
visible: is.metadata.labels['opendatahub.io/notebook-image'] === 'true',
error: getBYONImageErrorMessage(is),
packages:
is.spec.tags &&
(JSON.parse(
is.spec.tags[0].annotations['opendatahub.io/notebook-python-dependencies'],
) as BYONImagePackage[]),
software:
is.spec.tags &&
(JSON.parse(
is.spec.tags[0].annotations['opendatahub.io/notebook-software'],
) as BYONImagePackage[]),
uploaded: is.metadata.creationTimestamp,
packages: JSON.parse(
is.spec.tags?.[0].annotations['opendatahub.io/notebook-python-dependencies'] || '[]',
),
software: JSON.parse(is.spec.tags?.[0].annotations['opendatahub.io/notebook-software'] || '[]'),
imported_time: is.metadata.creationTimestamp,
url: is.metadata.annotations['opendatahub.io/notebook-image-url'],
user: is.metadata.annotations['opendatahub.io/notebook-image-creator'],
provider: is.metadata.annotations['opendatahub.io/notebook-image-creator'],
});

export const postImage = async (
Expand All @@ -215,7 +208,7 @@ export const postImage = async (
): Promise<{ success: boolean; error: string }> => {
const customObjectsApi = fastify.kube.customObjectsApi;
const namespace = fastify.kube.namespace;
const body = request.body as BYONImageCreateRequest;
const body = request.body as BYONImage;
const fullUrl = body.url;
const matchArray = fullUrl.match(imageUrlRegex);
// check if the host is valid
Expand All @@ -234,18 +227,18 @@ export const postImage = async (

if (validName.length > 0) {
fastify.log.error('Duplicate name unable to add notebook image');
return { success: false, error: 'Unable to add notebook image: ' + body.name };
return { success: false, error: 'Unable to add notebook image: ' + body.display_name };
}

const payload: ImageStream = {
kind: 'ImageStream',
apiVersion: 'image.openshift.io/v1',
metadata: {
annotations: {
'opendatahub.io/notebook-image-desc': body.description ? body.description : '',
'opendatahub.io/notebook-image-name': body.name,
'opendatahub.io/notebook-image-desc': body.description || '',
'opendatahub.io/notebook-image-name': body.display_name,
'opendatahub.io/notebook-image-url': fullUrl,
'opendatahub.io/notebook-image-creator': body.user,
'opendatahub.io/notebook-image-creator': body.provider,
},
name: `byon-${Date.now()}`,
namespace: namespace,
Expand Down Expand Up @@ -325,7 +318,7 @@ export const updateImage = async (
const customObjectsApi = fastify.kube.customObjectsApi;
const namespace = fastify.kube.namespace;
const params = request.params as { image: string };
const body = request.body as BYONImageUpdateRequest;
const body = request.body as BYONImage;
const labels = {
'app.kubernetes.io/created-by': 'byon',
'opendatahub.io/notebook-image': 'true',
Expand All @@ -334,8 +327,8 @@ export const updateImage = async (
const imageStreams = await getImageStreams(fastify, labels);
const validName = imageStreams.filter(
(is) =>
is.metadata.annotations['opendatahub.io/notebook-image-name'] === body.name &&
is.metadata.name !== body.id,
is.metadata.annotations['opendatahub.io/notebook-image-name'] === body.display_name &&
is.metadata.name !== body.name,
);

if (validName.length > 0) {
Expand Down Expand Up @@ -375,8 +368,8 @@ export const updateImage = async (
imageStream.metadata.labels['opendatahub.io/notebook-image'] = 'false';
}
}
if (body.name) {
imageStream.metadata.annotations['opendatahub.io/notebook-image-name'] = body.name;
if (body.display_name) {
imageStream.metadata.annotations['opendatahub.io/notebook-image-name'] = body.display_name;
}

if (body.description !== undefined) {
Expand Down
34 changes: 10 additions & 24 deletions backend/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -466,36 +466,23 @@ export type ODHSegmentKey = {

export type BYONImage = {
id: string;
user?: string;
uploaded?: Date;
error?: string;
} & BYONImageCreateRequest &
BYONImageUpdateRequest;

export type BYONImageCreateRequest = {
provider: string;
imported_time: string;
error: string;
name: string;
url: string;
description?: string;
// FIXME: This shouldn't be a user defined value consumed from the request payload but should be a controlled value from an authentication middleware.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this taken care of already?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not yet, I believe this part will be taken care of after we support OOTB images

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if we should be removing this comment then. If we are, we should be tracking it somewhere. Are we already tracking this?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, i think we should just keep the comment here until we get to refactor the backend in phase 2.

Copy link
Contributor

Choose a reason for hiding this comment

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

please keep the comment then, thanks

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry my bad, I thought I didn't remove the comment 😞 I will add it back in the following PR, thanks!

user: string;
software?: BYONImagePackage[];
packages?: BYONImagePackage[];
display_name: string;
description: string;
visible: boolean;
software: BYONImagePackage[];
packages: BYONImagePackage[];
};

export type ImageTag = {
image: ImageInfo | undefined;
tag: ImageTagInfo | undefined;
};

export type BYONImageUpdateRequest = {
id: string;
name?: string;
description?: string;
visible?: boolean;
software?: BYONImagePackage[];
packages?: BYONImagePackage[];
};

export type BYONImagePackage = {
name: string;
version: string;
Expand Down Expand Up @@ -546,6 +533,7 @@ export type ImageStream = {
namespace: string;
labels?: { [key: string]: string };
annotations?: { [key: string]: string };
creationTimestamp?: string;
};
spec: {
lookupPolicy?: {
Expand Down Expand Up @@ -826,7 +814,6 @@ export type TemplateParameter = {
required: boolean;
};


export type Template = K8sResourceCommon & {
metadata: {
annotations?: Partial<{
Expand Down Expand Up @@ -870,7 +857,6 @@ export type ContainerResources = {
};
};


export type ServingRuntime = K8sResourceCommon & {
metadata: {
annotations?: DisplayNameAnnotations & ServingRuntimeAnnotations;
Expand All @@ -893,4 +879,4 @@ export type ServingRuntime = K8sResourceCommon & {
supportedModelFormats: SupportedModelFormats[];
replicas: number;
};
};
};
36 changes: 36 additions & 0 deletions frontend/src/concepts/dashboard/DashboardEmptyTableView.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
import * as React from 'react';
import {
Bullseye,
Button,
EmptyState,
EmptyStateBody,
EmptyStateIcon,
EmptyStatePrimary,
Title,
} from '@patternfly/react-core';
import { SearchIcon } from '@patternfly/react-icons';

type DashboardEmptyTableViewProps = {
onClearFilters: () => void;
};

const DashboardEmptyTableView: React.FC<DashboardEmptyTableViewProps> = ({ onClearFilters }) => (
<Bullseye>
<EmptyState>
<EmptyStateIcon icon={SearchIcon} />
<Title size="lg" headingLevel="h2">
No results found
</Title>
<EmptyStateBody>
No results match the filter criteria. Clear all filters and try again.
</EmptyStateBody>
<EmptyStatePrimary>
<Button variant="link" onClick={() => onClearFilters()}>
Clear all filters
</Button>
</EmptyStatePrimary>
</EmptyState>
</Bullseye>
);

export default DashboardEmptyTableView;
69 changes: 69 additions & 0 deletions frontend/src/concepts/dashboard/DashboardSearchField.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
import * as React from 'react';
import { InputGroup, SearchInput, Select, SelectOption } from '@patternfly/react-core';

// List all the possible search fields here
export enum SearchType {
NAME = 'Name',
DESCRIPTION = 'Description',
USER = 'User',
PROJECT = 'Project',
METRIC = 'Metric',
PROTECTED_ATTRIBUTE = 'Protected attribute',
PRIVILEGED_VALUE = 'Privileged value',
UNPRIVILEGED_VALUE = 'Unprivileged value',
OUTPUT = 'Output',
OUTPUT_VALUE = 'Output value',
PROVIDER = 'Provider',
}

type DashboardSearchFieldProps = {
types: string[];
searchType: SearchType;
onSearchTypeChange: (searchType: SearchType) => void;
searchValue: string;
onSearchValueChange: (searchValue: string) => void;
};

const DashboardSearchField: React.FC<DashboardSearchFieldProps> = ({
types,
searchValue,
searchType,
onSearchValueChange,
onSearchTypeChange,
}) => {
const [typeOpen, setTypeOpen] = React.useState(false);

return (
<InputGroup>
<Select
removeFindDomNode
isOpen={typeOpen}
onToggle={() => setTypeOpen(!typeOpen)}
selections={searchType}
onSelect={(e, key) => {
if (typeof key === 'string') {
onSearchTypeChange(SearchType[key]);
setTypeOpen(false);
}
}}
>
{types.map((key) => (
<SelectOption key={key} value={key}>
{SearchType[key]}
</SelectOption>
))}
</Select>
<SearchInput
placeholder={`Find by ${searchType.toLowerCase()}`}
value={searchValue}
onChange={(_, newSearch) => {
onSearchValueChange(newSearch);
}}
onClear={() => onSearchValueChange('')}
style={{ minWidth: '200px' }}
/>
</InputGroup>
);
};

export default DashboardSearchField;
48 changes: 48 additions & 0 deletions frontend/src/pages/BYONImages/BYONImageStatusToggle.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
import * as React from 'react';
import { Switch } from '@patternfly/react-core';
import { BYONImage } from '~/types';
import { updateBYONImage } from '~/services/imagesService';
import useNotification from '~/utilities/useNotification';

type BYONImageStatusToggleProps = {
image: BYONImage;
};

const BYONImageStatusToggle: React.FC<BYONImageStatusToggleProps> = ({ image }) => {
const [isLoading, setLoading] = React.useState(false);
const [isEnabled, setEnabled] = React.useState(image.visible);
const notification = useNotification();
const handleChange = (checked: boolean) => {
setLoading(true);
updateBYONImage({
name: image.name,
visible: checked,
packages: image.packages,
})
.then(() => {
setEnabled(checked);
})
.catch((e) => {
notification.error(
`Error ${checked ? 'enable' : 'disable'} the serving runtime`,
e.message,
);
setEnabled(!checked);
})
.finally(() => {
setLoading(false);
});
};

return (
<Switch
aria-label={`Enable Switch ${image.name}`}
data-id={`enabled-disable-${image.id}`}
isChecked={isEnabled}
onChange={handleChange}
isDisabled={isLoading}
/>
);
};

export default BYONImageStatusToggle;
Loading
Loading