feat: modify behaviours for the enable button for NIM. #3558

yzhao583 · 2024-12-09T05:00:24Z

Jira: https://issues.redhat.com/browse/NVPE-37
Jira: https://issues.redhat.com/browse/NVPE-72

Description

Show Skeleton when visibility of the enable button is determined.
Disable the submit button if the API key is not provided by user.
Hide the link to remove the NIM app from the enabled apps page when NIM is disabled.
Hide the enable button after NIM app is enabled.

How Has This Been Tested?

Tested locally.

Test Impact

Go to the Explore page, select the NVIDIA NIM tile, and should see the Skeletonwhen loading. If NIM is enabled, the Enable button should be hidden, if NIM is disabled but can be enabled, the Enable button should be displayed, if NIM cannot be enabled, the Enable button should be displayed but the button should be disabled, hover the button, the tooltip should be displayed.
Click the Enable button, and the modal to enable NIM should be displayed. If no API key is provided, the Submit button should be disabled, otherwise, it should be enabled.
Provide API key, and click the Submit button. If validation success, the modal should be closed and the Enable button on the Explore page should be hidden.
Disable NIM and go to the Enable page -> click the disable on the top-right corner of the NIM tile, the link to remove NIM from this page should NOT be seen.

Request review criteria:

Self checklist (all need to be checked):

The developer has manually tested the changes and verified that the changes work
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 or cypress tests 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.

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

openshift-ci · 2024-12-09T05:00:36Z

Hi @yzhao583. Thanks for your PR.

I'm waiting for a opendatahub-io member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

andrewballantyne · 2024-12-09T17:53:23Z

/ok-to-test

codecov · 2024-12-10T08:25:45Z

Codecov Report

Attention: Patch coverage is 18.75000% with 52 lines in your changes missing coverage. Please review.

Project coverage is 85.06%. Comparing base (8d3338c) to head (dd60b29).
Report is 13 commits behind head on main.

Files with missing lines	Patch %	Lines
frontend/src/utilities/useEnableApplication.tsx	5.26%	36 Missing ⚠️
frontend/src/components/OdhAppCard.tsx	0.00%	5 Missing ⚠️
...ntend/src/pages/exploreApplication/EnableModal.tsx	37.50%	5 Missing ⚠️
...d/src/pages/exploreApplication/GetStartedPanel.tsx	28.57%	5 Missing ⚠️
...nd/src/utilities/useWatchIntegrationComponents.tsx	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3558      +/-   ##
==========================================
+ Coverage   85.01%   85.06%   +0.05%     
==========================================
  Files        1404     1406       +2     
  Lines       32239    32332      +93     
  Branches     9038     9085      +47     
==========================================
+ Hits        27407    27503      +96     
+ Misses       4832     4829       -3

Files with missing lines	Coverage Δ
...pages/exploreApplication/useIntegratedAppStatus.ts	`88.88% <100.00%> (+1.38%)`	⬆️
frontend/src/types.ts	`100.00% <100.00%> (ø)`
...nd/src/utilities/useWatchIntegrationComponents.tsx	`37.73% <0.00%> (ø)`
frontend/src/components/OdhAppCard.tsx	`61.90% <0.00%> (-2.03%)`	⬇️
...ntend/src/pages/exploreApplication/EnableModal.tsx	`33.92% <37.50%> (+1.85%)`	⬆️
...d/src/pages/exploreApplication/GetStartedPanel.tsx	`62.06% <28.57%> (-2.22%)`	⬇️
frontend/src/utilities/useEnableApplication.tsx	`24.34% <5.26%> (-3.92%)`	⬇️

... and 18 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8d3338c...dd60b29. Read the comment docs.

frontend/src/components/OdhAppCard.tsx

frontend/src/pages/exploreApplication/EnableModal.tsx

… show success banner, if it is false, show error banner

…/github.com/yzhao583/odh-dashboard into NVPE-37-fix-the-enable-button-loading-issue

frontend/src/pages/exploreApplication/EnableModal.tsx

emilys314 · 2024-12-18T14:29:09Z

/lgtm

openshift-ci · 2024-12-19T05:06:05Z

New changes are detected. LGTM label has been removed.

frontend/src/utilities/useEnableApplication.tsx

emilys314 · 2024-12-19T16:48:47Z

frontend/src/utilities/useEnableApplication.tsx

+              setEnableStatus({
+                status:
+                  response.variablesValidationStatus === 'True'
+                    ? EnableApplicationStatus.SUCCESS
+                    : EnableApplicationStatus.FAILED,
+                error:
+                  response.variablesValidationStatus === 'True' ? '' : 'Variables are not valid',
+              });
+              dispatchResults(
+                response.variablesValidationStatus === 'True'
+                  ? undefined
+                  : 'Variables are not valid',
+              );


Also the first time I enabled NIM with the modal by entering a key, it will tell me it failed with "Variables are not valid". I believe this is because the response.variablesValidationStatus is 'Unknown'

But this code will think it failed unless it's True. So we should update this part to account for Unknown somehow

@emilys314 The logic here is to check if response.variablesValidationStatus is true. If yes, dispatchResults. If the response.variablesValidationStatus is Unknown, it should loop again to check the status of the application.

Oh okay so it depends on how you test it. If I just delete the nvidia-nim-access your PR will work fine and not give an error.

But if the key is invalid (deleted/incorrect/expired) then it will say it's erroring when I submit

The response I get back is {isInstalled: true, isEnabled: false, variablesValidationStatus: 'False', canInstall: true, error: ''}

That's because the getIntegrationAppEnablementStatus() is polling the existing secret which is invalid via the GET. Meanwhile the initial pending value of the POST enableIntegrationApp() quickly gets overwritten by the poller to invalid

@emilys314 Good catch! I am having hard time fixing this.... Do you have any suggestion?

@emilys314 I have a solution but it is not very ideal.

@emilys314
Yes the conditions status will be reset.

There are two conditions for this to work:

The secrest should be managed by ODH, indicated by a target label. See here.

The secret is referenced by an Account. See here.

As long as these conditions stand, a reconcialtion will start with the initial "Unknown" status. See here.

cc: @swati-kale @yzhao583

I see thank you for the info Tomer, so I guess the issue is there will be a delay when the new api key is set and the validation status in the account. And this is pretty hard to get rid of.

@yzhao583 I think there is a solution we can implement in the UI to work around this. When we start polling the status, we can also return a variablesValidationTimestamp to check if we have a new value. So on submit, we store a timestamp in the UI, and while polling we will only update the status if the variablesValidationTimestamp is newer than the onSubmit timestamp one. I think you can also revert / replace your latest commit with this

@emilys314
We're also working on a way to speed up the status reporting. Currently, we report it at the end of the reconciliation; we'll change this behaviour to report more frequently while the reconciliation is still running. Regardless, this will remain an async operation. While it will be faster, checking the timestamp is better, as you suggested.

@emilys314 @TomerFi Thanks for the suggestions, I will implement the workaround to compare the time stamp.

@emilys314 @TomerFi @swati-kale Implemented the Timestamp workaround, please review again, thanks!

frontend/src/components/OdhAppCard.tsx

frontend/src/types.ts

frontend/src/pages/exploreApplication/useIntegratedAppStatus.ts

…/github.com/yzhao583/odh-dashboard into NVPE-37-fix-the-enable-button-loading-issue

openshift-ci · 2024-12-23T07:59:34Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from andrewballantyne. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

…account is changed, and remove the interval to check the enablement status of the app in useIntegratedAppStatus hook

…t CR

emilys314 · 2025-01-08T14:04:37Z

It looks like there is a typescript error with the most recent rebasing to include Olga's pr in frontend/src/__tests__/cypress/cypress/utils/nimUtils.ts

I think we may want to make the properties variablesValidationStatus and variablesValidationTimestamp optional in type IntegrationAppStatus in frontend/src/types.ts

yzhao583 · 2025-01-09T03:08:07Z

@emilys314 I changed variablesValidationTimestamp and variablesValidationStatus to optional

emilys314

Also make sure to run the linter / formatter

emilys314 · 2025-01-09T19:08:01Z

frontend/src/utilities/useEnableApplication.tsx

+              if (shouldContinueWatching(response)) {
                watchHandle = setTimeout(watchStatus, 10 * 1000);
                return;
              }
-              if (response.isInstalled) {
-                setEnableStatus({
-                  status: EnableApplicationStatus.SUCCESS,
-                  error: '',
-                });
-                dispatchResults(undefined);
-              }
+              setLastVariablesValidationTimestamp(response.variablesValidationTimestamp || 'Unknown');
+              setEnableStatus({
+                status:
+                  response.variablesValidationStatus === VariablesValidationStatus.SUCCESS
+                    ? EnableApplicationStatus.SUCCESS
+                    : EnableApplicationStatus.FAILED,
+                error:
+                  response.variablesValidationStatus === VariablesValidationStatus.SUCCESS
+                    ? ''
+                    : 'Variables are not valid',


I'm facing an issue where the first time I submit the api key, it will say it's invalid.

Delete the nim secret or change it to be invalid

Reload the page (important! otherwise the modal is saving the timestamp state after being closed because this modal isn't great)

Enter and submit new key

See invalid error

I believe this is happening because the initial value of the timestamp is set to Unknown, after the onSubmit POST, it will return the last timestamp which is stale. The watcher sees the timestamp changes and will check the status and see the stale invalid one.

@emilys314 Thanks! Fixed it.

frontend/src/pages/exploreApplication/useIntegratedAppStatus.ts

emilys314 · 2025-01-09T19:18:44Z

backend/src/routes/api/integrations/nim/index.ts

+              variablesValidationStatus: VariablesValidationStatus.UNKNOWN,
+              variablesValidationTimestamp: 'Unknown',


Nitpick: I'm not sure how I feel about having "Unknown" as the return value for the timestamp. It looks a little strange next to the other enum, and timestamp can have many values, not different enum states, also passing in "Unknown" is a magic string. In my opinion returning an empty value or no timestamp is a more universal sign that there is no timestamp

@emilys314 Thanks for your suggestion. Are you suggesting replacing Unknown with '' ?

@emilys314 Replaced Unknow to ''

…idationTimestamp to empty string

Fix the enable button issue for NIM

97e4f94

openshift-ci bot requested review from DaoDaoNoCode and manaswinidas December 9, 2024 05:00

openshift-ci bot added the needs-ok-to-test The openshift bot needs to label PRs from non members to avoid strain on the CI label Dec 9, 2024

yzhao583 force-pushed the NVPE-37-fix-the-enable-button-loading-issue branch from 39448bd to 97e4f94 Compare December 9, 2024 05:22

Fix lint issue

7043b7e

yzhao583 force-pushed the NVPE-37-fix-the-enable-button-loading-issue branch 2 times, most recently from 1bed0fd to 7043b7e Compare December 9, 2024 05:49

Merge branch 'main' into NVPE-37-fix-the-enable-button-loading-issue

80c69c3

openshift-ci bot added ok-to-test The openshift bot needs `ok-to-test` to allow non member PRs to run the tests. and removed needs-ok-to-test The openshift bot needs to label PRs from non members to avoid strain on the CI labels Dec 9, 2024

Fix lint issue

fbe5939

Merge branch 'main' into NVPE-37-fix-the-enable-button-loading-issue

9dcb27f

emilys314 reviewed Dec 11, 2024

View reviewed changes

frontend/src/components/OdhAppCard.tsx Outdated Show resolved Hide resolved

frontend/src/pages/exploreApplication/EnableModal.tsx Outdated Show resolved Hide resolved

yzhao583 added 4 commits December 16, 2024 17:08

When enable NIM, check the APIKeyValidation condition, if it is True,…

75e509b

… show success banner, if it is false, show error banner

Merge branch 'NVPE-37-fix-the-enable-button-loading-issue' of https:/…

86947ef

…/github.com/yzhao583/odh-dashboard into NVPE-37-fix-the-enable-button-loading-issue

Fix lint issue

c2fb70f

Add refreshRate

e8e6cc3

emilys314 reviewed Dec 17, 2024

View reviewed changes

frontend/src/pages/exploreApplication/EnableModal.tsx Outdated Show resolved Hide resolved

yzhao583 added 3 commits December 18, 2024 14:31

Reslove conflict

509fa1a

Fix grammar issue

919f529

Fix account and secret update issue

87b1ce9

openshift-ci bot assigned emilys314 Dec 18, 2024

openshift-ci bot added the lgtm label Dec 18, 2024

yzhao583 requested a review from emilys314 December 19, 2024 05:05

Merge branch 'main' into NVPE-37-fix-the-enable-button-loading-issue

e21fd83

openshift-ci bot removed the lgtm label Dec 19, 2024

emilys314 reviewed Dec 19, 2024

View reviewed changes

frontend/src/utilities/useEnableApplication.tsx Outdated Show resolved Hide resolved

emilys314 reviewed Dec 19, 2024

View reviewed changes

andrewballantyne requested changes Dec 19, 2024

View reviewed changes

frontend/src/components/OdhAppCard.tsx Show resolved Hide resolved

frontend/src/types.ts Outdated Show resolved Hide resolved

frontend/src/pages/exploreApplication/useIntegratedAppStatus.ts Outdated Show resolved Hide resolved

openshift-ci bot assigned andrewballantyne Dec 19, 2024

yzhao583 added 2 commits December 23, 2024 15:58

Merge branch 'main' into NVPE-37-fix-the-enable-button-loading-issue

cbfd6d9

Merge branch 'NVPE-37-fix-the-enable-button-loading-issue' of https:/…

2c52889

…/github.com/yzhao583/odh-dashboard into NVPE-37-fix-the-enable-button-loading-issue

Change variablesValidationStatus to enum

257b83c

yzhao583 requested review from andrewballantyne and emilys314 December 23, 2024 09:56

yzhao583 added 6 commits December 27, 2024 09:24

Merge branch 'main' into NVPE-37-fix-the-enable-button-loading-issue

b40e336

Merge branch 'main' into NVPE-37-fix-the-enable-button-loading-issue

654f82f

Merge branch 'main' into NVPE-37-fix-the-enable-button-loading-issue

580b6e2

modify the related logic to ensure the secret is modified before the …

f5323bd

…account is changed, and remove the interval to check the enablement status of the app in useIntegratedAppStatus hook

Merge branch 'main' into NVPE-37-fix-the-enable-button-loading-issue

31af752

Compare timestamps to see if backend updated conditions of the accoun…

edf7a5c

…t CR

yzhao583 requested review from swati-kale and TomerFi January 8, 2025 07:09

Make variablesValidationStatus and variablesValidationTimestamp optional

41f4197

emilys314 reviewed Jan 9, 2025

View reviewed changes

fix api key update issue and change the initial value of variablesVal…

dd60b29

…idationTimestamp to empty string

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: modify behaviours for the enable button for NIM. #3558

feat: modify behaviours for the enable button for NIM. #3558

yzhao583 commented Dec 9, 2024 •

edited

Loading

openshift-ci bot commented Dec 9, 2024

andrewballantyne commented Dec 9, 2024

codecov bot commented Dec 10, 2024 •

edited

Loading

emilys314 commented Dec 18, 2024

openshift-ci bot commented Dec 19, 2024

emilys314 Dec 19, 2024 •

edited

Loading

yzhao583 Dec 23, 2024

emilys314 Jan 2, 2025 •

edited

Loading

yzhao583 Jan 3, 2025

yzhao583 Jan 3, 2025

TomerFi Jan 7, 2025

emilys314 Jan 7, 2025

TomerFi Jan 7, 2025

yzhao583 Jan 8, 2025

yzhao583 Jan 8, 2025

openshift-ci bot commented Dec 23, 2024

emilys314 commented Jan 8, 2025

yzhao583 commented Jan 9, 2025

emilys314 left a comment

emilys314 Jan 9, 2025

yzhao583 Jan 10, 2025

emilys314 Jan 9, 2025

yzhao583 Jan 10, 2025

yzhao583 Jan 10, 2025

		variablesValidationStatus: VariablesValidationStatus.UNKNOWN,
		variablesValidationTimestamp: 'Unknown',

feat: modify behaviours for the enable button for NIM. #3558

Are you sure you want to change the base?

feat: modify behaviours for the enable button for NIM. #3558

Conversation

yzhao583 commented Dec 9, 2024 • edited Loading

Description

How Has This Been Tested?

Test Impact

Request review criteria:

openshift-ci bot commented Dec 9, 2024

andrewballantyne commented Dec 9, 2024

codecov bot commented Dec 10, 2024 • edited Loading

Codecov Report

emilys314 commented Dec 18, 2024

openshift-ci bot commented Dec 19, 2024

emilys314 Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emilys314 Jan 2, 2025 • edited Loading

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

openshift-ci bot commented Dec 23, 2024

emilys314 commented Jan 8, 2025

yzhao583 commented Jan 9, 2025

emilys314 left a 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

yzhao583 commented Dec 9, 2024 •

edited

Loading

codecov bot commented Dec 10, 2024 •

edited

Loading

emilys314 Dec 19, 2024 •

edited

Loading

emilys314 Jan 2, 2025 •

edited

Loading