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

Added accelerator column and field #2077

Conversation

Gkrumbach07
Copy link
Member

@Gkrumbach07 Gkrumbach07 commented Nov 6, 2023

closes: #1439
closes: #2056

Description

  • Added accelerators column to image admin page
    • each row matches to what accelerator profiles exist in the cluster
    • pending UX issue for when an image does not have the "recommended accelerators" annotation
    • a button that navigates with state of identifier to the create accelerator profile page
  • multi select input value for accelerator identifiers
    • this uses the identifiers from all accelerator profiles found to populate the dropdown
  • navigate to create page when clicking "Create accelerator"
image image image image

Screenshot 2023-11-06 at 4 56 29 PM
Screenshot 2023-11-06 at 4 56 39 PM
Screenshot 2023-11-06 at 4 57 30 PM
Screenshot 2023-11-06 at 4 57 41 PM
Screenshot 2023-11-06 at 4 58 41 PM
Screenshot 2023-11-06 at 4 58 59 PM
Screenshot 2023-11-06 at 4 59 14 PM

When clicking on "Create profile"
multiple identifiers
image
one identifier
image

How Has This Been Tested?

  1. create a accelerator profile CR
  2. open BYON import modal
  3. fill in required fields
  4. open dropdown for accelerators
  5. The accelerator profile identifier you just added should be there. select it
  6. start typing a new identifier
  7. select create
  8. save form
  9. edit new image just created
  10. see that identifiers are there
  11. remove and add a new identifier
  12. repeat steps 9 and 10

Test Impact

Tests are added to the existing image admin page test suite. Specifically testing

  • ability to see accelerator profile display names when there is a match in the table
  • able to sort accelerator column
  • import modal's accelerator identifier multi select can select, create, and remove from the input
  • edit modal has identifier info when present in the image annotations

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

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress This PR is in WIP state label Nov 6, 2023
@Gkrumbach07
Copy link
Member Author

@yannnz This does not look like the mocks completely because of the following

  • BYON enhancements phase 2 has a lot of overlap with this PR and @andrewballantyne is looking into the impact of adding some of the BYON features to this PR. Examples would be: including OOTB images, import/edit page layout
  • specifically in the import/edit modal for a image. You used multiple single select fields with chips. so i used the patternfly multiselect. Let me know if you intended for there to be multiple single selects and i can change it to that flow instead

@Gkrumbach07
Copy link
Member Author

@kaedward I think you looked at most of the wording already. but just in case i will give you a tag

@Gkrumbach07 Gkrumbach07 linked an issue Nov 6, 2023 that may be closed by this pull request
@yannnz
Copy link

yannnz commented Nov 7, 2023

@yannnz This does not look like the mocks completely because of the following

  • BYON enhancements phase 2 has a lot of overlap with this PR and @andrewballantyne is looking into the impact of adding some of the BYON features to this PR. Examples would be: including OOTB images, import/edit page layout
  • specifically in the import/edit modal for a image. You used multiple single select fields with chips. so i used the patternfly multiselect. Let me know if you intended for there to be multiple single selects and i can change it to that flow instead
    @Gkrumbach07
  1. Is it saying that when we are working on adding the BYON features, we will adapt to the new import/edit page layout? e.g. uses page rather than model for the import/edit.

  2. In the screenshot, the identifier multiselect dropdown looks good to me. Is there a way to display a long identifier? I think PF default it as truncated chip and allow a tooltip when mouse hover.

Screenshot 2023-11-07 at 15 58 03 .
  1. Same comment for the tooltip next to the question icon. I think we will need popover for this case. Screenshot 2023-11-07 at 16 24 54

@Gkrumbach07
Copy link
Member Author

@yannnz

  1. Thats unclear to me. I dont know whats in byon 2 and whats not. The goal of this feature was to not cause any blockers for that future work
  2. Already supported
    Screenshot 2023-11-07 at 7 12 11 AM
  3. I switched this over to tooltip

@Gkrumbach07 Gkrumbach07 force-pushed the admin-image-page branch 3 times, most recently from 9573e4b to 9199855 Compare November 10, 2023 14:43
@Gkrumbach07 Gkrumbach07 linked an issue Nov 13, 2023 that may be closed by this pull request
@Gkrumbach07
Copy link
Member Author

@yannnz one more thing now that the other PR merged. This is what you had in the mocks. Look alright still?

When clicking on "Create profile"
multiple identifiers
image
one identifier
image

@Gkrumbach07 Gkrumbach07 changed the title [WIP] Added accelerator column and field Added accelerator column and field Nov 14, 2023
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress This PR is in WIP state label Nov 14, 2023
@Gkrumbach07 Gkrumbach07 force-pushed the admin-image-page branch 4 times, most recently from 3242b8c to 782173e Compare November 15, 2023 19:01
@Gkrumbach07 Gkrumbach07 force-pushed the admin-image-page branch 2 times, most recently from 0d51627 to 801299c Compare November 15, 2023 19:16
added tests

tooltip to popover

naming fix, and new ux

added new disbaled state

make label compact

added navigate to create

rerender fix

add comment

remove span

changed icons

added loading
@christianvogt
Copy link
Contributor

/approve

Tested various scenarios.

Copy link
Contributor

openshift-ci bot commented Nov 15, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christianvogt

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

@christianvogt
Copy link
Contributor

/lgtm

@dpanshug
Copy link
Contributor

/lgtm

@openshift-merge-bot openshift-merge-bot bot merged commit 8aac844 into opendatahub-io:f/accelerator-admin-support Nov 15, 2023
4 checks passed
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.

[Story]: Display lack of accelerator recommendations in column Map accelerators to custom notebook images
4 participants