Parse BYON repository URL correctly #1249

DaoDaoNoCode · 2023-05-17T13:56:30Z

Description

The PR did the following things:

Use regex to match the repository URL, which could help correctly parse the hostname and the tag name
If the user doesn't input the tag name, we will set the tag name default to latest, because it will automatically fetch the latest tag if you create an image stream without specifying the tag name
If the user inputs the tag name, we will set the tag name of the BYON image to the same as the tag name user is importing

Because some docker container images might not have the latest tag or might not have the tag user specifies, it could throw an error if the tag cannot be found. In this situation, we show the error in the BYON image table to the admin users and hide the image from the spawner page of both the KFNBC and DS projects.

UX/UI change: Add an error state and tooltip to invalid BYON images

How Has This Been Tested?

Try to create some custom images, you can use the images here https://quay.io/repository/opendatahub/workbench-images?tab=tags with the tag starting with jupyter, also you can go to search the OpenShift internal image streams like image-registry.openshift-image-registry.svc:5000/opendatahub/s2i-minimal-notebook. Try to create some with and without the tags
Go to the spawner page to see if you can create a notebook/workbench with the custom image
Try to find an image without the latest tag and import that image without specifying the tag
Check whether there is an error state in the table for that image
Check whether the error image is not visible in the spawner page image selections

Test Impact

Add some jest tests to make sure the regex could parse the URL correctly.

Request review criteria:

The commits have meaningful messages (squashes happen on merge by the bot).
Included any necessary screenshots or gifs if it was a UI change.
Included tags to the UX team if it was a UI/UX change.
Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
The developer has manually tested the changes and verified that the changes work
The developer has added tests or explained why testing cannot be added (unit tests & storybook for related changes)

DaoDaoNoCode · 2023-05-17T13:58:45Z

backend/src/utils/constants.ts

@@ -130,3 +130,6 @@ export const DEFAULT_NOTEBOOK_SIZES: NotebookSize[] = [
    },
  },
 ];
+
+export const imageUrlRegex =
+  /^([\w.\-_]+((?::\d+|)(?=\/[a-z0-9._-]+\/[a-z0-9._-]+))|)(?:\/|)([a-z0-9.\-_]+(?:\/[a-z0-9.\-_]+|))(?::([\w.\-_]{1,127})|)/;


This regex won't match @sha256... as the tag because if use that, we cannot set it as the tag name. So in the custom image, the tag name will be set to latest but the repo URL will be set to the correct one with @sha256.

Oh my gosh -- we need you to comment this in some fashion haha... wowzers, that's a regex and a half haha.

DaoDaoNoCode · 2023-05-17T14:01:58Z

@vconzola Please check the UI change for the error state of the custom images as listed in the description.

backend/src/routes/api/images/imageUtils.ts

frontend/src/__tests__/dockerRepositoryURL.spec.ts

frontend/src/pages/projects/screens/spawner/spawnerUtils.ts

vconzola · 2023-05-18T15:03:47Z

@DaoDaoNoCode As long as invalid images are hidden from non-admin users in the spawner dropdowns, the UX LGTM.

lucferbux · 2023-06-01T13:08:37Z

backend/src/routes/api/images/imageUtils.ts

+  // check if the host is valid
+  if (!matchArray[1]) {
+    fastify.log.error('Invalid repository URL unable to add notebook image');
+    return { success: false, error: 'Invalid repository URL: ' + fullUrl };


I guess that's the way we had the endpoint configure, but I'm not a fan of returning a 200 with a failing state, rather than using the proper error code.

Yeah, there is a lot of uncomfortable stuff with this area of the code -- I'm viewing the UI and noticing a lot of inconsistencies to the other parts of our app... we will need to rework this.

lucferbux

/lgtm

I left a little nit, but at this point I don't think we should do major refactors to the backend so it's ok.

andrewballantyne · 2023-06-02T14:00:52Z

frontend/src/__tests__/dockerRepositoryURL.spec.ts

+test('OpenShift internal registry URL with tag', () => {
+  const url =
+    'image-registry.openshift-image-registry.svc:5000/opendatahub/s2i-minimal-notebook:v0.3.0-py36';
+  const match = url.match(REPOSITORY_URL_REGEX);


I think this should have been a function wrapping to give 1/4 a real name instead of magic-ness... these tests showcase that just in reading of the test lines.

andrewballantyne · 2023-06-02T14:08:43Z

frontend/src/pages/BYONImages/ImageErrorStatus.tsx

+  }
+  return (
+    <Tooltip removeFindDomNode role="none" content={<Text>{image.error}</Text>}>
+      <Icon role="button" aria-label="error icon" status="danger" isInline tabIndex={0}>


I feel this is not the best way to make it tab-enabled... because you're explicitly setting the tab index to 0. We did this elsewhere with a button: https://github.com/opendatahub-io/odh-dashboard/blob/main/frontend/src/components/SupportedAppTitle.tsx#L22-L30

andrewballantyne

Will log a tech debt to clean up some of these things... this is bad for maintenance, but this works as-is... so we'll want to clean it up next sprint.

EDIT: for posterity, #1326

openshift-ci · 2023-06-02T14:19:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewballantyne, lucferbux

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [andrewballantyne]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Parse BYON repository URL correctly

2892f97

openshift-ci bot requested review from alexcreasy and lucferbux May 17, 2023 13:56

DaoDaoNoCode requested a review from andrewballantyne May 17, 2023 13:56

DaoDaoNoCode commented May 17, 2023

View reviewed changes

lucferbux requested changes May 18, 2023

View reviewed changes

backend/src/routes/api/images/imageUtils.ts Outdated Show resolved Hide resolved

frontend/src/__tests__/dockerRepositoryURL.spec.ts Outdated Show resolved Hide resolved

frontend/src/pages/projects/screens/spawner/spawnerUtils.ts Show resolved Hide resolved

openshift-ci bot assigned lucferbux May 18, 2023

address comments

5aa1889

lucferbux reviewed Jun 1, 2023

View reviewed changes

lucferbux approved these changes Jun 1, 2023

View reviewed changes

openshift-ci bot added the lgtm label Jun 1, 2023

andrewballantyne reviewed Jun 2, 2023

View reviewed changes

andrewballantyne approved these changes Jun 2, 2023

View reviewed changes

openshift-ci bot assigned andrewballantyne Jun 2, 2023

openshift-ci bot added the approved label Jun 2, 2023

openshift-merge-robot merged commit a80041d into opendatahub-io:main Jun 2, 2023

andrewballantyne mentioned this pull request Jun 2, 2023

[Tech Debt]: Cleanup Image Regex #1326

Closed

andrewballantyne mentioned this pull request Jun 16, 2023

[Feature Request]: Report results of BYON URL validation in the modal #1348

Closed

DaoDaoNoCode deleted the upstream-main branch August 29, 2023 18:37

DaoDaoNoCode restored the upstream-main branch August 29, 2023 18:37

spolti mentioned this pull request Oct 19, 2023

[Snyk] Upgrade @kubernetes/client-node from 0.12.3 to 0.19.0 spolti/odh-dashboard#1

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parse BYON repository URL correctly #1249

Parse BYON repository URL correctly #1249

DaoDaoNoCode commented May 17, 2023 •

edited

Loading

DaoDaoNoCode May 17, 2023

andrewballantyne Jun 2, 2023

DaoDaoNoCode commented May 17, 2023

vconzola commented May 18, 2023

lucferbux Jun 1, 2023

andrewballantyne Jun 2, 2023

lucferbux left a comment

andrewballantyne Jun 2, 2023

andrewballantyne Jun 2, 2023

andrewballantyne left a comment •

edited

Loading

openshift-ci bot commented Jun 2, 2023

Parse BYON repository URL correctly #1249

Parse BYON repository URL correctly #1249

Conversation

DaoDaoNoCode commented May 17, 2023 • edited Loading

Description

How Has This Been Tested?

Test Impact

Request review criteria:

DaoDaoNoCode May 17, 2023

Choose a reason for hiding this comment

andrewballantyne Jun 2, 2023

Choose a reason for hiding this comment

DaoDaoNoCode commented May 17, 2023

vconzola commented May 18, 2023

lucferbux Jun 1, 2023

Choose a reason for hiding this comment

andrewballantyne Jun 2, 2023

Choose a reason for hiding this comment

lucferbux left a comment

Choose a reason for hiding this comment

andrewballantyne Jun 2, 2023

Choose a reason for hiding this comment

andrewballantyne Jun 2, 2023

Choose a reason for hiding this comment

andrewballantyne left a comment • edited Loading

Choose a reason for hiding this comment

openshift-ci bot commented Jun 2, 2023

DaoDaoNoCode commented May 17, 2023 •

edited

Loading

andrewballantyne left a comment •

edited

Loading