logic for deprecated images #1872

Gkrumbach07 · 2023-09-27T17:49:27Z

closes: #1370

Description

Added a new annotation to workbenches called opendatahub.io/image-display-name which stores the display name of the selected image. this gets used when displaying the image on the notebook table if the image itself could not be found. if it is not found it will also include the wording (deprecated).

When you got to edit this workbench with a deprecated image. the image will be unselected and require you to select a new one

How Has This Been Tested?

create a custom notebook image
create a workbench with that custom image
delete the custom image
verify that deprecated wording is on the workbench table
verify that on edit, the image selection is required
remove the annotation (opendatahub.io/image-display-name) from the notebook resource, and notice that the image becomes unknown on the table row

Test Impact

I added tests to check the use cases of unknown and deprecated. In doing this i felt the need to implement a few helpers for mocks. namely a opts: RecursivePartial<k8sKindType> and then lodash _.merge the base mock with the opts. this gives you the option for fine grained control without having to make a large amount of of options to feed in.

Request review criteria:

Self checklist (all need to be checked):

The developer has manually tested the changes and verified that the changes work
Commits have been squashed into descriptive, self-contained units of work (e.g. 'WIP' and 'Implements feedback' style messages have been removed)
Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
The developer has added tests or explained why testing cannot be added (unit tests & storybook for related changes)

If you have UI changes:

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 (find relevant UX in the SMEs section).

After the PR is posted & before it merges:

The developer has tested their solution on a cluster by using the image produced by the PR to main

Gkrumbach07 · 2023-09-27T17:55:40Z

UX context

This bug fixes the edge case where a user has a already created workbench, but the image tag or the entire image is removed from ODH. an example would be that an admin deleted a custom notebook image but a workbench was using it.

@kywalker-rh @xianli123 See screenshots for how it would look in the row. For edit, the image selector appears as empty and you are required to select a new image.

@kaedward In the issue you signed off on the wording Image Name (deprecated), but just a fyi in case it looks odd to you

kywalker-rh · 2023-09-28T13:00:15Z

Looks good to me, thanks for updating it @Gkrumbach07!

frontend/src/api/k8s/notebooks.ts

DaoDaoNoCode · 2023-09-28T18:05:01Z

I found an issue that's not caused by your PR, but it will affect the result of your PR so you may want to update it as well here.
In useNotebookImage hook, it gets the tagSoftware here, however, this is the wrong function to use. We should use getImageVersionSoftwareString instead of getRelatedVersionDescription. When there is only one version for an image, these 2 functions get the same result. However, if there are more than one version, the second function (getRelatedVersionDescription) could return an undefined because it's only used to show the software string in the image selection dropdown.
So it you want to get a correct and exact software string of a specific image version, you might want to replace it with getImageVersionSoftwareString(imageVersion).
FYI @Gkrumbach07 because if you don't update this hook here, you will get (deprecated) tag for those multi-version images.

Gkrumbach07 · 2023-09-28T19:33:47Z

@DaoDaoNoCode

because if you don't update this hook here, you will get (deprecated) tag for those multi-version images.

I saw this in testing so i used an or operator here to prevent the text from only showing (deprecated). It will fall back to using the display name or in the last case Unknown

const imageDisplayName =
    notebookImage?.imageName ||
    obj.notebook.metadata.annotations['opendatahub.io/image-display-name'] ||
    'Unknown';

However that doesn't fix the case where deprecated will show even if it is not because tag software is undefined. I updated how tag software is fetched based on your comment and adjusted how deprecated is calculated

andrewballantyne · 2023-09-29T22:03:27Z

Needs a rebase

frontend/src/pages/projects/screens/detail/notebooks/NotebookTableRow.tsx

frontend/src/api/k8s/notebooks.ts

frontend/src/__tests__/integration/pages/projects/ProjectDetails.spec.ts

Gkrumbach07 · 2023-10-05T13:00:14Z

Update

I have added a new state for disabled: This required me add an additional includeDisabled option for getting imagestreams because we didnt do that by default.
on edit of a workbench using a disabled image, I allow the user to make edits without forcing them to select a new image: This was a bit odd to do, but i just drilled down an additional prop called oringalImage which will track the disabled image option in the select field for the spawner page

These changes came from conversations in an other issue that is merging with this one

context on the merge: #1520 (comment)

Updated Images (OUTDATED)

xianli123 · 2023-10-07T09:55:32Z

@Gkrumbach07 Thanks for invloving me. I left some comments in the issue #1520 , please refer to my comments.

DaoDaoNoCode · 2023-11-02T14:45:24Z

When the notebook image is deleted, should we also disable the toggle button so the users cannot toggle it anymore? It doesn't make sense when we tell the user they need to select another image to start the notebook while we still allow them to start it and get Error: ImagePullBackOff later.

DaoDaoNoCode

Overall it's working as expected. Just left some small comments as well as the question above. Otherwise it looks good!

frontend/src/__tests__/integration/pages/projects/ProjectDetails.spec.ts

frontend/src/pages/projects/screens/detail/notebooks/useNotebookImageData.ts

kywalker-rh · 2023-11-02T19:20:55Z

When the notebook image is deleted, should we also disable the toggle button so the users cannot toggle it anymore? It doesn't make sense when we tell the user they need to select another image to start the notebook while we still allow them to start it and get Error: ImagePullBackOff later.

I think that's a great idea... good catch @DaoDaoNoCode !

Gkrumbach07 · 2023-11-06T13:49:18Z

@DaoDaoNoCode everything should be addressed

DaoDaoNoCode

/lgtm

Gkrumbach07 · 2023-11-06T19:29:08Z

@christianvogt this PR is ready for your final review

christianvogt · 2023-11-06T20:06:48Z

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

 ): [imageStreams: ImageStreamKind[], loaded: boolean, loadError: Error | undefined] => {
  const [imageStreams, setImageStreams] = React.useState<ImageStreamKind[]>([]);
  const [loaded, setLoaded] = React.useState(false);
  const [loadError, setLoadError] = React.useState<Error | undefined>(undefined);

  React.useEffect(() => {
    if (namespace) {
-      getNotebookImageStreams(namespace)
+      getNotebookImageStreams(namespace, includeDisabled)


If an error occurs when includeDisabled changes and we have previously loaded data, setImageStreams will remain the old value as the catch clause does not reset the data. Also I don't think we should be setting loaded to true if we have an error. The typical pattern here sets loaded to true when data is available. Loaded state should be unaffected by error state.

previous logic set loaded to true regardless of the outcome. This was so we can have a state where the api is settled or not. and then error and data represent the outcome. If we only set loaded to true on success, then loaded has not value because we no longer can use it to determine if we are loading or not.

I can however set the state back to an empty array to stop previous loaded data from lingering on error

actually i am just going to switch this over to useFetchState if i can so it uses that common logic

christianvogt · 2023-11-06T20:09:28Z

frontend/src/pages/projects/screens/detail/notebooks/useNotebookImageData.ts

-    if (!loaded) {
-      return [null, false];
+    if (loadError) {
+      return [null, true, loadError];


I don't think loaded should be set to true here because this pattern would suggests that data is available.

Suggested change

return [null, true, loadError];

return [null, false, loadError];

christianvogt · 2023-11-06T20:09:42Z

frontend/src/pages/projects/screens/detail/notebooks/useNotebookImageData.ts

+    if (!notebook || !loaded) {
+      return [null, false, undefined];
    }

-    if (!loaded) {
-      return [null, false];
+    if (loadError) {
+      return [null, true, loadError];


I think you should handle error state before loaded state or combine all together if you allow error state to occur when loaded is false.

now handling like this

if (!loaded || !notebook) { return [null, false, loadError]; }

christianvogt · 2023-11-06T20:10:29Z

frontend/src/pages/projects/screens/detail/notebooks/useNotebookImage.ts

+    return [null, false, undefined];
+  }
+  if (loadError) {
+    return [null, true, loadError];


Similar comment elsewhere:

Suggested change

return [null, true, loadError];

return [null, false, loadError];

christianvogt · 2023-11-06T20:10:42Z

frontend/src/pages/projects/screens/detail/notebooks/useNotebookImage.ts

+  if (!notebook || !loaded) {
+    return [null, false, undefined];
+  }
+  if (loadError) {
+    return [null, true, loadError];


Should handle error state before loaded state. But likely can combine all together if you allow error state to occur when loaded is false.

christianvogt · 2023-11-06T20:15:11Z

frontend/src/pages/projects/screens/detail/notebooks/useNotebookImage.ts

+  | [notebookImage: null, loaded: false, loadError: undefined]
+  | [notebookImage: null, loaded: true, loadError: Error]
+  | [notebookImage: NotebookImage, loaded: true, loadError: undefined] => {


The typical pattern suggests that loaded=true implies data is available, but error state is not related to loaded state. In more generic cases, error can be present while data is available if an error occurred on an update.

Suggested change

| [notebookImage: null, loaded: false, loadError: undefined]

| [notebookImage: null, loaded: true, loadError: Error]

| [notebookImage: NotebookImage, loaded: true, loadError: undefined] => {

| [notebookImage: null, loaded: false, loadError: Error | undefined]

| [notebookImage: NotebookImage, loaded: true, loadError: undefined] => {

christianvogt · 2023-11-06T20:17:18Z

frontend/src/pages/projects/screens/detail/notebooks/types.ts

+  | [data: null, loaded: false, loadError: undefined]
+  | [data: null, loaded: true, loadError: Error]


I see that this traces down to useImageSteams which sets loaded to true along with an error. So the logic around this tyoe would work but I believe the pattern is to typically return an error with any value of loaded. useFetchState can return a subsequent error on refresh while loaded was previously set to true.

Suggested change

| [data: null, loaded: false, loadError: undefined]

| [data: null, loaded: true, loadError: Error]

| [data: null, loaded: false, loadError: Error | undefined]

Gkrumbach07 · 2023-11-07T16:54:53Z

@christianvogt i switched the logic around to use useFetchState and have loaded = true to mean there is always some data defined

christianvogt · 2023-11-08T19:06:13Z

latest changes to use useFetchState look good.
/lgtm
/approve

openshift-ci · 2023-11-08T19:06:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christianvogt, DaoDaoNoCode

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~~ [christianvogt]

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

logic for deprecated images added tests fix annotations made annotations land labels optional supported disabled wording unknown state will not use a label Updated tests, updated UI fix tests small fixes added deleted disabled loaded always signifies data exists moved popover

openshift-ci · 2023-11-08T19:09:38Z

New changes are detected. LGTM label has been removed.

openshift-ci bot requested review from manaswinidas and pnaik1 September 27, 2023 17:49

DaoDaoNoCode reviewed Sep 28, 2023

View reviewed changes

frontend/src/api/k8s/notebooks.ts Outdated Show resolved Hide resolved

Gkrumbach07 force-pushed the notebook-name-store branch from ab9f5b5 to 89f1a1f Compare September 28, 2023 16:22

Gkrumbach07 force-pushed the notebook-name-store branch from 89f1a1f to 8e9f02c Compare September 28, 2023 19:32

DaoDaoNoCode mentioned this pull request Sep 29, 2023

Disabling a custom Notebook Image from view makes its Workbenches display an Unknown image #1520

Closed

openshift-merge-robot added the needs-rebase PR needs to be rebased label Sep 29, 2023

Gkrumbach07 force-pushed the notebook-name-store branch from 8e9f02c to 9e17a8a Compare October 2, 2023 21:27

openshift-merge-robot removed the needs-rebase PR needs to be rebased label Oct 2, 2023

Gkrumbach07 force-pushed the notebook-name-store branch from 9e17a8a to c2552f6 Compare October 2, 2023 21:28

christianvogt reviewed Oct 4, 2023

View reviewed changes

frontend/src/pages/projects/screens/detail/notebooks/NotebookTableRow.tsx Outdated Show resolved Hide resolved

Gkrumbach07 force-pushed the notebook-name-store branch 2 times, most recently from 398ee66 to 784fb70 Compare October 4, 2023 19:19

christianvogt reviewed Oct 4, 2023

View reviewed changes

frontend/src/api/k8s/notebooks.ts Outdated Show resolved Hide resolved

frontend/src/__tests__/integration/pages/projects/ProjectDetails.spec.ts Outdated Show resolved Hide resolved

Gkrumbach07 force-pushed the notebook-name-store branch from 784fb70 to 046f699 Compare October 5, 2023 12:55

Gkrumbach07 force-pushed the notebook-name-store branch 3 times, most recently from 4b6b423 to e0da178 Compare October 12, 2023 13:34

Gkrumbach07 requested review from DaoDaoNoCode, christianvogt and andrewballantyne October 12, 2023 13:35

xianli123 mentioned this pull request Oct 13, 2023

[Bug]: Displaying Image version for OOTB Notebook images in Project View would be useful to have #1917

Closed

1 task

DaoDaoNoCode reviewed Nov 2, 2023

View reviewed changes

Gkrumbach07 force-pushed the notebook-name-store branch 2 times, most recently from 84d0ed1 to 926d69b Compare November 6, 2023 13:48

DaoDaoNoCode approved these changes Nov 6, 2023

View reviewed changes

openshift-ci bot assigned DaoDaoNoCode Nov 6, 2023

openshift-ci bot added the lgtm label Nov 6, 2023

christianvogt requested changes Nov 6, 2023

View reviewed changes

openshift-ci bot assigned christianvogt Nov 6, 2023

openshift-ci bot removed the lgtm label Nov 6, 2023

Gkrumbach07 force-pushed the notebook-name-store branch from 926d69b to fac5edb Compare November 7, 2023 16:53

openshift-ci bot added the lgtm label Nov 8, 2023

openshift-ci bot added the approved label Nov 8, 2023

christianvogt removed the approved label Nov 8, 2023

Gkrumbach07 force-pushed the notebook-name-store branch from fac5edb to cd79472 Compare November 8, 2023 19:09

openshift-ci bot removed the lgtm label Nov 8, 2023

openshift-ci bot added the approved label Nov 8, 2023

christianvogt added the lgtm label Nov 8, 2023

openshift-merge-bot bot merged commit 152b672 into opendatahub-io:main Nov 8, 2023
6 checks passed

andrewballantyne linked an issue Nov 9, 2023 that may be closed by this pull request

Disabling a custom Notebook Image from view makes its Workbenches display an Unknown image #1520

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

logic for deprecated images #1872

logic for deprecated images #1872

Gkrumbach07 commented Sep 27, 2023 •

edited

Loading

Gkrumbach07 commented Sep 27, 2023 •

edited

Loading

kywalker-rh commented Sep 28, 2023

DaoDaoNoCode commented Sep 28, 2023

Gkrumbach07 commented Sep 28, 2023

andrewballantyne commented Sep 29, 2023

Gkrumbach07 commented Oct 5, 2023 •

edited

Loading

xianli123 commented Oct 7, 2023

DaoDaoNoCode commented Nov 2, 2023

DaoDaoNoCode left a comment

kywalker-rh commented Nov 2, 2023

Gkrumbach07 commented Nov 6, 2023

DaoDaoNoCode left a comment

Gkrumbach07 commented Nov 6, 2023

christianvogt Nov 6, 2023

Gkrumbach07 Nov 7, 2023

Gkrumbach07 Nov 7, 2023

Gkrumbach07 Nov 7, 2023

christianvogt Nov 6, 2023

christianvogt Nov 6, 2023

Gkrumbach07 Nov 7, 2023

christianvogt Nov 6, 2023

christianvogt Nov 6, 2023

christianvogt Nov 6, 2023

christianvogt Nov 6, 2023

Gkrumbach07 commented Nov 7, 2023

christianvogt commented Nov 8, 2023

openshift-ci bot commented Nov 8, 2023

openshift-ci bot commented Nov 8, 2023

	return [null, true, loadError];
	return [null, false, loadError];

		\| [data: null, loaded: false, loadError: undefined]
		\| [data: null, loaded: true, loadError: Error]

logic for deprecated images #1872

logic for deprecated images #1872

Conversation

Gkrumbach07 commented Sep 27, 2023 • edited Loading

Description

How Has This Been Tested?

Test Impact

Request review criteria:

Gkrumbach07 commented Sep 27, 2023 • edited Loading

UX context

kywalker-rh commented Sep 28, 2023

DaoDaoNoCode commented Sep 28, 2023

Gkrumbach07 commented Sep 28, 2023

andrewballantyne commented Sep 29, 2023

Gkrumbach07 commented Oct 5, 2023 • edited Loading

Update

Updated Images (OUTDATED)

xianli123 commented Oct 7, 2023

DaoDaoNoCode commented Nov 2, 2023

DaoDaoNoCode left a comment

Choose a reason for hiding this comment

kywalker-rh commented Nov 2, 2023

Gkrumbach07 commented Nov 6, 2023

DaoDaoNoCode left a comment

Choose a reason for hiding this comment

Gkrumbach07 commented Nov 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gkrumbach07 commented Nov 7, 2023

christianvogt commented Nov 8, 2023

openshift-ci bot commented Nov 8, 2023

openshift-ci bot commented Nov 8, 2023

Gkrumbach07 commented Sep 27, 2023 •

edited

Loading

Gkrumbach07 commented Sep 27, 2023 •

edited

Loading

Gkrumbach07 commented Oct 5, 2023 •

edited

Loading