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

[Tech Debt]: Cleanup Image Regex #1326

Closed
andrewballantyne opened this issue Jun 2, 2023 · 2 comments · Fixed by #1529
Closed

[Tech Debt]: Cleanup Image Regex #1326

andrewballantyne opened this issue Jun 2, 2023 · 2 comments · Fixed by #1529
Assignees
Labels
feature/byon Bring Your Own Notebook Feature kind/tech-debt A technical debt item for the development team. E.g. Refactors / Tests / etc priority/normal An issue with the product; fix when possible

Comments

@andrewballantyne
Copy link
Member

andrewballantyne commented Jun 2, 2023

Feature description

We implemented a Image regex in #1249 and it's got little explanation. We use it string.match(regexp) and the result is manually match[4] -- this is nightmarish for the next dev to read into this. Looking at the complex regexp is nasty too...

Also, to tie it all off, the reg exp is duped in both back and frontend due to having no test runner in the backend and that's where the code usage is at.

Changes needed to clean this up:

  • Wrap the usage of the reg expression in a function -- document through code
    • eg. parseImageURL(imageString) and put the constant inside inside this function
    • Return out of this function an object { fullURL: string; tag: string; host: string; ... } Using [1], [4] should only be used in this function and thus documented as you put into a variable... ideally don't have groups you don't care about, or put them all in the object -- ie, what is [2] and [3] in the match string?
  • Add Jest on the backend + Test this function (we'll move it from the backend to the frontend with the code)
    • Delete tests in frontend (as they are copied/rewritten to use the function)
    • Delete copied regexp in frontend

Describe alternatives you've considered

No response

Anything else?

No response

@andrewballantyne andrewballantyne added untriaged Indicates the newly create issue has not been triaged yet kind/tech-debt A technical debt item for the development team. E.g. Refactors / Tests / etc labels Jun 2, 2023
@andrewballantyne andrewballantyne added this to the Upcoming Release milestone Jun 2, 2023
@shalberd
Copy link
Contributor

shalberd commented Jun 3, 2023

Here is Mr. airgapped / on-prem again :-)

What about typing in a specific image by digest, not by tag, in BYON? As of now, that is not possible, is it?

e.g. { fullURL: string; digest: string; imagestreamtagname: string; ... } or so

https://github.com/opendatahub-io/odh-manifests/blob/v1.6/notebook-images/base/jupyter-datascience-notebook-imagestream.yaml#L24

quay.io/opendatahub/workbench-images@sha256:af8da57b334a334475d9404ed42f7b535ca27ad7c9e0278d28b47b9e83002493

tag for imagestream / dashboard GUI when selecting available notebook images to start. For the image above, I'd want that to appear as jupyter-datascience-ubi9-python-3.9-2023a-20230515 later on, being imagestream tag.name

Bildschirmfoto 2023-06-03 um 07 17 58
Bildschirmfoto 2023-06-03 um 07 18 10

On the other hand, the whole BYON image import functionality only works with the internal docker registry probably anyways, so no need to also make this sha256 digest format compatible?

On import, is only an imagestream def created? If yes, that whole above idea is not necessary, unless someone absoiutely want to make sure to get an image by digest and not by tag from the external source registry.

Related: Do think about using the image trigger annotation when assembling notebooks, though for making assembly of notebooks possibible on clusters without and with openshift internal registry, leading to the image-field of the StatefulSet derived from the Notebook being populated based on imagestream name and tag.

Because the neat thing is: Even if tags.from.name is in tag-format, e.g. a usual quai.ui tag-based full path the the image, it resolves in the end to sha256 digest for the image-field.

I am still in coordination with that with Joo Hoo Lee and Vaishnavi, though.

@andrewballantyne
Copy link
Member Author

andrewballantyne commented Jun 5, 2023

Hey @shalberd -- love the engagement! I do believe we will be correcting this in #1266 when we cycle through to clean up BYON based on other customer feedback.

Snippet from the WIP mocks:
image

Things still need to be ironed out, but it should cover SHA references at that point.

@Gkrumbach07 Gkrumbach07 added priority/normal An issue with the product; fix when possible feature/byon Bring Your Own Notebook Feature and removed untriaged Indicates the newly create issue has not been triaged yet labels Jun 7, 2023
@andrewballantyne andrewballantyne removed this from the Current Release milestone Jul 12, 2023
@DaoDaoNoCode DaoDaoNoCode linked a pull request Jul 25, 2023 that will close this issue
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature/byon Bring Your Own Notebook Feature kind/tech-debt A technical debt item for the development team. E.g. Refactors / Tests / etc priority/normal An issue with the product; fix when possible
Projects
No open projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants