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

Apply hardware profile to notebook image settings table #3645

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

DaoDaoNoCode
Copy link
Member

@DaoDaoNoCode DaoDaoNoCode commented Jan 14, 2025

JIRA: RHOAIENG-16258

Description

Apply the hardware profiles to the notebook image settings page. This remains unchanged when the hardware profile flag is disabled. When it's enabled, do the following changes:

  1. Replace the identifiers selector in the import/edit modal of the notebook image, and map all the hardware profile identifiers in the node resources table (except 'cpu' and 'memory' as they are the default ones)
Screenshot 2025-01-14 at 3 51 47 PM
  1. Replace the column Recommended identifiers with Recommended hardware profiles and put all the hardware profiles that has the selected identifier (as long as at least one identifier is included in the hardware profile node resources, we think that hardware profile is recommended)
Screenshot 2025-01-14 at 3 57 30 PM
  1. When creating the hardware profile from the column, navigate it to the create hardware profile page and pre-fill the node resources table with the current recommended identifier(s)
Screenshot 2025-01-14 at 3 52 04 PM

How Has This Been Tested?

  1. Turn on hardware profile feature flag
  2. Create some hardware profiles with different custom identifiers in the node resources table
  3. Go to notebook image settings page
  4. Edit a notebook image, choose the identifier(s)
  5. Make sure the hardware profiles that contain the identifier(s) are listed on the recommended column
  6. Try to create a profile from this page, you will see the identifiers are filled in the search params in the URL
  7. The node resources table should be pre-filled with the information in the URL

Test Impact

Added a unit test to test the recommended hardware profile filter function, and added a cypress test to test the pre-fill node resources table based on the search params in the URL.

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 openshift-ci bot requested review from alexcreasy and dpanshug January 14, 2025 21:04
Copy link
Contributor

openshift-ci bot commented Jan 14, 2025

[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 assign andrewballantyne for approval. 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:

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

Copy link

codecov bot commented Jan 14, 2025

Codecov Report

Attention: Patch coverage is 87.69231% with 8 lines in your changes missing coverage. Please review.

Project coverage is 85.71%. Comparing base (0ce1fbb) to head (d8f7675).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
...src/pages/BYONImages/BYONImageHardwareProfiles.tsx 73.68% 5 Missing ⚠️
...BYONImages/BYONImageModal/ManageBYONImageModal.tsx 87.50% 1 Missing ⚠️
...ontend/src/pages/BYONImages/BYONImagesTableRow.tsx 75.00% 1 Missing ⚠️
frontend/src/pages/BYONImages/tableData.tsx 50.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3645      +/-   ##
==========================================
- Coverage   85.79%   85.71%   -0.08%     
==========================================
  Files        1419     1421       +2     
  Lines       32688    32750      +62     
  Branches     9195     9212      +17     
==========================================
+ Hits        28044    28071      +27     
- Misses       4644     4679      +35     
Files with missing lines Coverage Δ
...mageModal/HardwareProfileIdentifierMultiselect.tsx 100.00% <100.00%> (ø)
frontend/src/pages/BYONImages/BYONImagesTable.tsx 93.75% <100.00%> (+0.56%) ⬆️
frontend/src/pages/BYONImages/utils.ts 100.00% <100.00%> (ø)
...ages/hardwareProfiles/HardwareProfilesTableRow.tsx 84.37% <ø> (ø)
...dwareProfiles/manage/ManageNodeResourceSection.tsx 97.22% <100.00%> (+1.06%) ⬆️
frontend/src/types.ts 100.00% <ø> (ø)
...BYONImages/BYONImageModal/ManageBYONImageModal.tsx 93.84% <87.50%> (-0.99%) ⬇️
...ontend/src/pages/BYONImages/BYONImagesTableRow.tsx 84.00% <75.00%> (-1.72%) ⬇️
frontend/src/pages/BYONImages/tableData.tsx 80.00% <50.00%> (-7.50%) ⬇️
...src/pages/BYONImages/BYONImageHardwareProfiles.tsx 73.68% <73.68%> (ø)

... and 11 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 0ce1fbb...d8f7675. Read the comment docs.

@pnaik1
Copy link
Contributor

pnaik1 commented Jan 15, 2025

@DaoDaoNoCode , I created notebook image with a new option(ex: gpu) for Hardware profile identifier and then clicking on create profile in the Recommended hardware profiles column , navigate it to the create hardware profile page and pre-fill the node resources table with the current recommended identifier(s)(ex: gpu) , I created the hardware profile with recommended identifiers, editing the newly created hardware profile doesnt show up the recomended identifier(ex: gpu)

rest of the functionality mentioned are working as expected

@pnaik1
Copy link
Contributor

pnaik1 commented Jan 16, 2025

@DaoDaoNoCode , thanks for the changes, that works well
/lgtm

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

Successfully merging this pull request may close these issues.

2 participants