Model Registry Settings: Add model registries table with db config viewer, delete action and stub for permission management modal #2829

mturley · 2024-05-20T21:25:26Z

Closes RHOAIENG-6970
Closes RHOAIENG-2230 (followup to #2828)

Description

Adds the Model Registries table to the Model Registry Settings page
Adds a stub modal for the "Manage Permissions" button, to be implemented in another story
Adds a "View database configuration" kebab action which opens a modal. All details are displayed immediately except the password, which shows a loading skeleton while the associated secret is fetched.
- For displaying the password in this modal, refactors the backend service so it handles secrets along with modelregistries
- If there is an error fetching the associated secret or decoding the password from it, an inline alert is shown.
Adds a "Delete model registry" kebab action with a modal requiring typing the name for confirmation.
Using the updated backend service, adds databasePassword to the values passed when submitting the CreateModal

Note: Does not include the Database Status column from the mockups, that is blocked by backend work and has been moved into a followup story.

cc @yih-wang

Table:

Kebab menu:

Password loading state:

Password loaded (clicking the eyeball shows it):

Password loading error state:

Delete modal:

Delete modal with name entered correctly:

How Has This Been Tested?

Tested viewing existing model registries that were created on the shared cluster for testing, including viewing database details. Tested the password loading error by creating a model registry referencing a nonexistent secret and trying to view its details. Tested the delete operation.

Test Impact

Cypress tests will be added in a separate PR, they'll conflict with tests @gitdallas has already started on

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 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 (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

codecov · 2024-05-20T21:51:09Z

Codecov Report

Attention: Patch coverage is 67.66917% with 43 lines in your changes missing coverage. Please review.

Project coverage is 78.23%. Comparing base (b33db83) to head (fe007f1).
Report is 10 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2829      +/-   ##
==========================================
+ Coverage   78.16%   78.23%   +0.06%     
==========================================
  Files        1101     1109       +8     
  Lines       23421    23531     +110     
  Branches     5902     5922      +20     
==========================================
+ Hits        18307    18409     +102     
- Misses       5114     5122       +8

Files	Coverage Δ
...nd/src/concepts/dashboard/DashboardModalFooter.tsx	`100.00% <100.00%> (ø)`
...modelRegistrySettings/useModelRegistriesBackend.ts	`100.00% <100.00%> (ø)`
frontend/src/k8sTypes.ts	`100.00% <ø> (ø)`
.../modelRegistrySettings/ViewDatabaseConfigModal.tsx	`100.00% <100.00%> (ø)`
...rontend/src/pages/modelRegistrySettings/columns.ts	`100.00% <100.00%> (ø)`
...RegistrySettings/ModelRegistryDatabasePassword.tsx	`88.88% <88.88%> (ø)`
...nd/src/pages/modelRegistrySettings/CreateModal.tsx	`69.41% <33.33%> (-0.83%)`	⬇️
...ges/modelRegistrySettings/ModelRegistriesTable.tsx	`75.00% <75.00%> (ø)`
...es/modelRegistrySettings/ModelRegistrySettings.tsx	`71.42% <50.00%> (-8.58%)`	⬇️
...s/modelRegistrySettings/ManagePermissionsModal.tsx	`66.66% <66.66%> (ø)`
... and 3 more

... and 1 file 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 b33db83...fe007f1. Read the comment docs.

yih-wang · 2024-05-21T07:01:11Z

@mturley Thanks for all the work! They look really great!

Can you use a plain alert for the case when the password cannot load? Additionally, is it possible for the user to successfully fetch the password with another attempt? If so, it might be beneficial to add a retry button here, like my second screenshot.

backend/src/routes/api/modelRegistrySettings/secrets/index.ts

mturley · 2024-05-21T15:45:00Z

Can you use a plain alert for the case when the password cannot load? Additionally, is it possible for the user to successfully fetch the password with another attempt? If so, it might be beneficial to add a retry button here, like my second screenshot.

@yih-wang I'll change to a plain alert, thanks. I don't think it's possible to succeed on retry here, the only reason an error would happen when loading the password but not the rest of the registry info is if the data is malformed (e.g. the ModelRegistry object refers to a passwordSecret that doesn't exist), which can only happen if the user is manually messing around with the objects on the cluster and would not be fixed until they corrected that mistake. If there's a network issue or other reason for the failure, it would also affect the rest of the data on the page and they wouldn't reach this modal.

mturley · 2024-05-21T15:49:28Z

Pushed the change to switch to plain alert and updated the screenshot in the description.

mturley · 2024-05-21T15:55:59Z

@lucferbux if you look at the Delete modal screenshots, am I correct in assuming that the default permission group shown in the message there (currently "TODO: group here") is something we can't fill in until we figure out RHOAIENG-6636 (RBAC story), or is that maybe known based on the registry name?

mturley · 2024-05-21T20:55:55Z

/hold
let's rebase this after #2828 is merged and implement the Secret logic part of submitting that create modal as part of this PR. That PR will also establish cypress tests that this PR can add to.

backend/src/routes/api/modelRegistries/index.ts

mturley · 2024-06-03T21:08:15Z

/unhold

@lucferbux this is ready for review now, sorry for the delay on the cypress tests!

mturley · 2024-06-04T17:40:17Z

/hold
Dallas found an issue I'm looking into

mturley · 2024-06-04T17:45:47Z

/unhold
False alarm. Need to test with local backend instead of start:dev:ext since the changes aren't on the cluster

mturley · 2024-06-04T19:36:23Z

Pushed changes to address some feedback after a Slack conversation with @gitdallas:

When creating or deleting a registry, the modal does not close until the list of registries has been fully refreshed (so we no longer get a lag between the submit completing and the list of registries changing).
When viewing database details, instead of showing a one-line loading skeleton while the password loads, the whole modal shows a spinner. The skeleton component seemed potentially confusing (if it's taking a while, it could just look like an intentionally obscured password). If there is an error loading the password, the rest of the database details will still be shown. Updated the loading state screenshot in the description above.

cc @yih-wang

…ewer, delete action and stub for permission management modal Signed-off-by: Mike Turley <[email protected]>

lucferbux · 2024-06-06T09:46:08Z

backend/src/routes/api/modelRegistries/modelRegistryUtils.ts

+  // eslint-disable-next-line no-restricted-properties
+  return Buffer.from(value || '').toString('base64');
+};
+const base64decode = (encodedValue?: string): string =>


Mmmm yeah, great idea having the utility function, we've always encoded/decoded without having a declared function in the backend

lucferbux · 2024-06-06T09:54:32Z

frontend/src/pages/modelRegistrySettings/CreateModal.tsx

 };

-const CreateModal: React.FC<CreateModalProps> = ({ isOpen, onClose }) => {
+const CreateModal: React.FC<CreateModalProps> = ({ isOpen, onClose, refresh }) => {


Mmmmm didn't comment this before, and it's not a bad addition cause in the admin panel we were using old not refactored forms, but we got useGenericObjectState for creating objects and using them for forms.

We should refactor this modal in the near future with a custom hook that feeds from useGenericObjectState

lucferbux · 2024-06-06T09:55:11Z

frontend/src/pages/modelRegistrySettings/CreateModal.tsx

@@ -88,7 +89,8 @@ const CreateModal: React.FC<CreateModalProps> = ({ isOpen, onClose }) => {
      },
    };
    try {
-      await createModelRegistryBackend(data);
+      await createModelRegistryBackend({ modelRegistry: data, databasePassword: password });


Remind me of talking about the posibility of using descriptive names for model registries, we sholud have a display name option, but we should talk with ux first.

lucferbux

/lgtm

openshift-ci · 2024-06-06T09:57:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gitdallas, 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~~ [lucferbux]

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

lucferbux · 2024-06-06T09:58:01Z

Code looks great, there are a few open questions that i wanna discuss with you @mturley so we can do follow ups, but so far, it can be merged!

lucferbux · 2024-06-06T09:58:24Z

(feel free to resolve all comments once you read them so we can proceed merging the pr)

mturley · 2024-06-06T15:16:16Z

Looks like the bot is happy to merge with unresolved conversations now 🤷

@lucferbux should we open followup issues for the 2 points you raised above (refactoring to use useGenericObjectState and supporting display names for registries)? I can do that if you want

openshift-ci bot requested review from DaoDaoNoCode and Gkrumbach07 May 20, 2024 21:25

lucferbux reviewed May 21, 2024

View reviewed changes

backend/src/routes/api/modelRegistrySettings/secrets/index.ts Outdated Show resolved Hide resolved

mturley force-pushed the RHOAIENG-6970-mr-settings-table branch from 9dd6b22 to 30a5b04 Compare May 21, 2024 15:46

openshift-ci bot added the do-not-merge/hold This PR is hold for some reason label May 21, 2024

mturley mentioned this pull request May 29, 2024

feat(mr): create mr modal #2828

Merged

7 tasks

openshift-merge-robot added the needs-rebase PR needs to be rebased label May 29, 2024

mturley force-pushed the RHOAIENG-6970-mr-settings-table branch from 30a5b04 to e55ded1 Compare May 30, 2024 23:36

openshift-merge-robot removed the needs-rebase PR needs to be rebased label May 30, 2024

mturley force-pushed the RHOAIENG-6970-mr-settings-table branch from e55ded1 to c817226 Compare May 31, 2024 16:29

gitdallas reviewed May 31, 2024

View reviewed changes

backend/src/routes/api/modelRegistries/index.ts Show resolved Hide resolved

mturley force-pushed the RHOAIENG-6970-mr-settings-table branch from c817226 to 6820c58 Compare June 3, 2024 21:00

openshift-merge-robot added the needs-rebase PR needs to be rebased label Jun 3, 2024

mturley force-pushed the RHOAIENG-6970-mr-settings-table branch 2 times, most recently from 6017907 to 30cc710 Compare June 3, 2024 21:07

openshift-merge-robot removed the needs-rebase PR needs to be rebased label Jun 3, 2024

openshift-ci bot removed the do-not-merge/hold This PR is hold for some reason label Jun 3, 2024

openshift-ci bot added the do-not-merge/hold This PR is hold for some reason label Jun 4, 2024

openshift-ci bot removed the do-not-merge/hold This PR is hold for some reason label Jun 4, 2024

mturley force-pushed the RHOAIENG-6970-mr-settings-table branch from 30cc710 to 2165cc9 Compare June 4, 2024 19:08

gitdallas approved these changes Jun 4, 2024

View reviewed changes

openshift-ci bot assigned gitdallas Jun 4, 2024

openshift-ci bot added the lgtm label Jun 4, 2024

mturley force-pushed the RHOAIENG-6970-mr-settings-table branch from 2165cc9 to d973ef8 Compare June 4, 2024 19:30

openshift-ci bot removed the lgtm label Jun 4, 2024

Model Registry Settings: Add model registries table with db config vi…

fe007f1

…ewer, delete action and stub for permission management modal Signed-off-by: Mike Turley <[email protected]>

mturley force-pushed the RHOAIENG-6970-mr-settings-table branch from d973ef8 to fe007f1 Compare June 4, 2024 20:35

gitdallas approved these changes Jun 4, 2024

View reviewed changes

openshift-ci bot added the lgtm label Jun 4, 2024

lucferbux reviewed Jun 6, 2024

View reviewed changes

lucferbux approved these changes Jun 6, 2024

View reviewed changes

openshift-ci bot assigned lucferbux Jun 6, 2024

openshift-ci bot added the approved label Jun 6, 2024

openshift-merge-bot bot merged commit f7c9979 into opendatahub-io:main Jun 6, 2024
8 checks passed

mturley deleted the RHOAIENG-6970-mr-settings-table branch June 6, 2024 15:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Model Registry Settings: Add model registries table with db config viewer, delete action and stub for permission management modal #2829

Model Registry Settings: Add model registries table with db config viewer, delete action and stub for permission management modal #2829

mturley commented May 20, 2024 •

edited

Loading

codecov bot commented May 20, 2024 •

edited

Loading

yih-wang commented May 21, 2024

mturley commented May 21, 2024 •

edited

Loading

mturley commented May 21, 2024 •

edited

Loading

mturley commented May 21, 2024

mturley commented May 21, 2024 •

edited

Loading

mturley commented Jun 3, 2024

mturley commented Jun 4, 2024

mturley commented Jun 4, 2024

mturley commented Jun 4, 2024 •

edited

Loading

lucferbux Jun 6, 2024

lucferbux Jun 6, 2024

lucferbux Jun 6, 2024

lucferbux left a comment

openshift-ci bot commented Jun 6, 2024

lucferbux commented Jun 6, 2024

lucferbux commented Jun 6, 2024

mturley commented Jun 6, 2024 •

edited

Loading

Model Registry Settings: Add model registries table with db config viewer, delete action and stub for permission management modal #2829

Model Registry Settings: Add model registries table with db config viewer, delete action and stub for permission management modal #2829

Conversation

mturley commented May 20, 2024 • edited Loading

Description

How Has This Been Tested?

Test Impact

Request review criteria:

codecov bot commented May 20, 2024 • edited Loading

Codecov Report

yih-wang commented May 21, 2024

mturley commented May 21, 2024 • edited Loading

mturley commented May 21, 2024 • edited Loading

mturley commented May 21, 2024

mturley commented May 21, 2024 • edited Loading

mturley commented Jun 3, 2024

mturley commented Jun 4, 2024

mturley commented Jun 4, 2024

mturley commented Jun 4, 2024 • edited Loading

lucferbux Jun 6, 2024

Choose a reason for hiding this comment

lucferbux Jun 6, 2024

Choose a reason for hiding this comment

lucferbux Jun 6, 2024

Choose a reason for hiding this comment

lucferbux left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Jun 6, 2024

lucferbux commented Jun 6, 2024

lucferbux commented Jun 6, 2024

mturley commented Jun 6, 2024 • edited Loading

mturley commented May 20, 2024 •

edited

Loading

codecov bot commented May 20, 2024 •

edited

Loading

mturley commented May 21, 2024 •

edited

Loading

mturley commented May 21, 2024 •

edited

Loading

mturley commented May 21, 2024 •

edited

Loading

mturley commented Jun 4, 2024 •

edited

Loading

mturley commented Jun 6, 2024 •

edited

Loading