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

Add Accelerator section in Admin Panel and feature flag #2002

Conversation

dpanshug
Copy link
Contributor

@dpanshug dpanshug commented Oct 23, 2023

Closes: #1372

Description

Added new feature flag disableAccelerators and new section in the admin panel for accelerators.

Screenshot from 2023-10-26 13-08-20

How Has This Been Tested?

  1. Add accelerator profile crd to your cluster, from odh-dasboard/manifests/crd/acceleratorprofiles.opendatahub.io.crd.yaml
  2. Open ODH dashboard with clusteradmin login
  3. In the sidebar, under setting section you can find Accelerator Profiles.

Note: This PR only provides empty state of the page, so make sure you don't have any AcceleratorProfile operand in your cluster.

Test Impact

Integration testing: To test Empty State of no Accelerator profile and created storybook too.

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

cc @yannnz

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress This PR is in WIP state label Oct 23, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 23, 2023

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@dpanshug
Copy link
Contributor Author

/test all

@dpanshug dpanshug changed the base branch from main to f/accelerator-admin-support October 23, 2023 08:01
@dpanshug dpanshug force-pushed the accelerator-section branch 4 times, most recently from 0c62def to aeb54fc Compare October 26, 2023 07:36
@dpanshug dpanshug marked this pull request as ready for review October 26, 2023 07:40
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress This PR is in WIP state label Oct 26, 2023
@openshift-ci openshift-ci bot requested review from DaoDaoNoCode and pnaik1 October 26, 2023 07:40
@dpanshug dpanshug added do-not-merge/work-in-progress This PR is in WIP state and removed do-not-merge/work-in-progress This PR is in WIP state labels Oct 26, 2023
@dpanshug dpanshug requested a review from Gkrumbach07 October 26, 2023 08:11
@dpanshug dpanshug force-pushed the accelerator-section branch from aeb54fc to d162244 Compare October 26, 2023 12:05
@Gkrumbach07
Copy link
Member

@kaedward In the mocks you gave concern on the wording of the empty state, The empty state is for admins to add accelerator profile CRs (gpus). Accelerator profiles are used to sync the use of gpus in the UI to the physical hardware in the cluster. Essentially an admin would go add an accelerator profile after they configured actual accelerators in the openshift console.

manifests/crd/odhdashboardconfigs.opendatahub.io.crd.yaml Outdated Show resolved Hide resolved
frontend/src/app/AppRoutes.tsx Outdated Show resolved Hide resolved
frontend/src/app/AppRoutes.tsx Outdated Show resolved Hide resolved
@Gkrumbach07
Copy link
Member

please add tests too @dpanshug

@dpanshug dpanshug force-pushed the accelerator-section branch 3 times, most recently from 5f8c511 to 4586d5f Compare October 27, 2023 15:57
frontend/src/__mocks__/mockAcceleratorProfile.ts Outdated Show resolved Hide resolved
frontend/src/types.ts Outdated Show resolved Hide resolved
Copy link
Member

@Gkrumbach07 Gkrumbach07 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few things and then should be good to go

@dpanshug dpanshug requested a review from Gkrumbach07 October 30, 2023 13:51
@dpanshug dpanshug force-pushed the accelerator-section branch from db98e21 to cf619d7 Compare October 30, 2023 15:08
@dpanshug dpanshug force-pushed the accelerator-section branch from cf619d7 to 7f8e230 Compare October 30, 2023 15:57
@dpanshug dpanshug requested a review from Gkrumbach07 October 30, 2023 15:57
Copy link
Member

@Gkrumbach07 Gkrumbach07 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@andrewballantyne this is ready for your review/approval

@openshift-ci openshift-ci bot added the lgtm label Oct 30, 2023
Copy link
Member

@Gkrumbach07 Gkrumbach07 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/approve

Copy link
Contributor

openshift-ci bot commented Oct 31, 2023

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by: Gkrumbach07

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:

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

@openshift-ci openshift-ci bot merged commit d15f8aa into opendatahub-io:f/accelerator-admin-support Oct 31, 2023
3 checks passed
@kaedward
Copy link

kaedward commented Nov 2, 2023

@kaedward In the mocks you gave concern on the wording of the empty state, The empty state is for admins to add accelerator profile CRs (gpus). Accelerator profiles are used to sync the use of gpus in the UI to the physical hardware in the cluster. Essentially an admin would go add an accelerator profile after they configured actual accelerators in the openshift console.

Thanks for the details, @Gkrumbach07 ! I think my problem was more with the phrasing - usually we wouldn't tell a user to go ask someone about something if we don't know for certain who that person is or if it is the current user. It's different if we're asking them to go ask their admin for some permission, but here we're not sure. This is based on @yannnz's comment in the mocks, which was 4 months ago so is probably outdated: " I believe they need to check the availability of accelerators in cluster, although it’s unclear who they should check with."

LMK if this is outdated and the info in the screenshot above is correct, and I can write another suggestion. For now though, I would recommend something more like this:

"No accelerator profiles yet
To get started, create an accelerator profile that corresponds with an available accelerator in your OpenShift cluster.
[Create accelerator profile]"

This is more straight-forward and allows the user to figure out who they need to talk to (if anyone). I also removed the "please" which we try to avoid, and updated the button to "Create" instead of "Add" since we refer to this as "Create" everywhere else. Please LMK what you think!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Accelerator section in Admin Panel and feature flag
4 participants