-
Notifications
You must be signed in to change notification settings - Fork 178
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 create/edit/duplicate hardware profile page #3604
base: main
Are you sure you want to change the base?
Add create/edit/duplicate hardware profile page #3604
Conversation
@vconzola Could you check the screenshots? Thanks! |
35c7a0d
to
b61a5ce
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3604 +/- ##
==========================================
+ Coverage 85.02% 85.45% +0.43%
==========================================
Files 1403 1415 +12
Lines 32213 32499 +286
Branches 9032 9122 +90
==========================================
+ Hits 27389 27772 +383
+ Misses 4824 4727 -97
... and 7 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
@DaoDaoNoCode A couple comments...
Otherwise, lgtm. |
@vconzola Just saw your comments in another PR and was about to ask you for the button positions 😄 thanks, I will update them. |
b61a5ce
to
9f0cc9d
Compare
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
frontend/src/pages/hardwareProfiles/manage/ManageHardwareProfile.tsx
Outdated
Show resolved
Hide resolved
56d6c3d
to
39a71f6
Compare
frontend/src/pages/hardwareProfiles/manage/ManageHardwareProfile.tsx
Outdated
Show resolved
Hide resolved
...end/src/__tests__/cypress/cypress/tests/mocked/hardwareProfiles/manageHardwareProfiles.cy.ts
Show resolved
Hide resolved
frontend/src/pages/hardwareProfiles/manage/ManageHardwareProfileWrapper.tsx
Outdated
Show resolved
Hide resolved
frontend/src/pages/hardwareProfiles/manage/ManageHardwareProfileWrapper.tsx
Outdated
Show resolved
Hide resolved
39a71f6
to
911c403
Compare
911c403
to
1e00112
Compare
@DaoDaoNoCode , I am unsure if this is related to your PR or has been discussed earlier, but while adding what if I want a scenario like this to have unit dropdown?? |
@pnaik1 its good that you asked, @Gkrumbach07 can you clarify on this? Earlier we only discussed having unit dropdowns for CPU and Memory identifier, but in the mocks we can see units with other identifiers also. |
@dpanshug Where in the mocks do you see units on identifiers other than CPU and memory? They shouldn't be there. If you can point me to the mockups where I show this, I'll update them. |
@dpanshug Ugh! So this is a bit of a problem. I guess I assumed because the label is RAM, you could tell it's a memory resource. But the label is just a label, right, so you ignore it? Is there any way for you to tell from the identifier what type of resource something is? Because for something like memory, without units, the numerical values are kind of meaningless. I originally had a "Resource type" dropdown field as part of the form, but @Gkrumbach07 decided we didn't need it. Maybe we need to bring it back? |
@vconzola hi, the identifier is unique and cannot be changeable, so for the preset |
@DaoDaoNoCode That's not how @Gkrumbach07 explained it to me. (At least that's not what I understood.) The idea is that if a type of RAM is used that has a unique identifier (e.g., samsung.com/gddr7) that will replace the memory identifier, not be a new identifier. |
Ok I did a quick search and while technically not required, So @vconzola
|
@Gkrumbach07 My gut tells me we should not allow deleting resources with cpu or ram identifiers. But will that create a conflict if a user adds a new resource (e.g. samsung.com/mem) that's also a memory resource instead of editing the existing resource with ram identifier? |
@vconzola So the issue is as follows
we can still block them from deleting, but if we allow changing the identifier, they have a work around to deleting it. For ultimate flexibility we can allow them to delete all but maybe adding a special warning if they try to delete cpu or ram |
@Gkrumbach07 I like that idea. Any idea what happens if they don't have a cpu or ram identifier? That will help inform what the warning should say. |
@pnaik1 OK, after discussion on Slack https://redhat-internal.slack.com/archives/C07C0FNHVPT/p1736352212866699, we will let k8s throw the error when submitting if there is a duplicated k8s name. As for the display name, we allow duplication, and will show a popover next to the name in the table to indicate the k8s name on the cluster for users to better tell different hardware profiles, just like other k8s resources. |
JIRA: RHOAIENG-16255
Description
Added the page to create/edit/duplicate hardware profile, which allows you to add/edit/remove node selectors and tolerations. The work for the identifiers(node resources) is implemented in #3565 so this PR doesn't include that part.
On create:
On update:
On duplicate:
All the other fields will get duplicated except the name description fields
Node selector modal:
Toleration modal:
Added last modified annotation:
How Has This Been Tested?
Test Impact
Added cypress tests for the create/edit page
Request review criteria:
Self checklist (all need to be checked):
If you have UI changes:
After the PR is posted & before it merges:
main