-
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 custom container size to workbenches table #2944
Add custom container size to workbenches table #2944
Conversation
@kywalker-rh Is there a need to state the resource limits and requests below |
@manaswinidas It would be helpful for the user to see what the limits and requests are, but as it states in the other comment, this can be shown when the row is expanded. |
4a23b19
to
11f4652
Compare
11f4652
to
dbcd95d
Compare
frontend/src/pages/projects/screens/detail/notebooks/NotebookTableRow.tsx
Outdated
Show resolved
Hide resolved
3a02eec
to
ab7f4dd
Compare
@kywalker-rh Yes, this is there but the limits and requests are not in the expanded row now that this container size is deleted. You can check this in the screen recording attached in the PR description. I have added screenshots to show the container size dropdown for this "Custom" state. Can you check them when you have time? As shown in the screenshot, I'm adding "Custom" as the first option in the dropdown and it doesn't show limits and requests in the option description coz it's all deleted. If I show them, it will look something like this: Let me know if the screenshots and screen recording look fine. |
@manaswinidas can we not show whatever is in the notebook? It's only on edit we show custom, right? |
ab7f4dd
to
3704712
Compare
Are you referring to the limits and requests? We are fetching the sizes directly from
Oh yes, missed it. Updated to show "Custom" dropdown option only on Edit. @kywalker-rh Also, is there a need to change the content in this alert? |
When creating a new workbench the alert pops up. Screen.Recording.2024-07-03.at.2.16.02.PM.mov |
3704712
to
da7d2d9
Compare
@dpanshug fixed! |
@manaswinidas I'm sorry I misunderstood what you were asking before. With more context I'm okay moving forward as is. |
da7d2d9
to
199b257
Compare
@kywalker-rh We now have the limits and requests in the expanded row. The screenshots and screen recording have been updated in the PR description. |
3bccbf8
to
232309f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a small nit, otherwise lgtm
frontend/src/pages/projects/screens/detail/notebooks/NotebookTableRow.tsx
Outdated
Show resolved
Hide resolved
232309f
to
15aaefe
Compare
45be5e2
to
66a7bb4
Compare
frontend/src/pages/projects/screens/spawner/useNotebookSizeState.ts
Outdated
Show resolved
Hide resolved
frontend/src/pages/projects/screens/spawner/useNotebookSizeState.ts
Outdated
Show resolved
Hide resolved
66a7bb4
to
976db7e
Compare
frontend/src/pages/projects/screens/spawner/useNotebookSizeState.ts
Outdated
Show resolved
Hide resolved
976db7e
to
c4e9e60
Compare
frontend/src/__tests__/cypress/cypress/tests/mocked/projects/workbench.cy.ts
Show resolved
Hide resolved
frontend/src/pages/projects/screens/detail/notebooks/useNotebookDeploymentSize.ts
Show resolved
Hide resolved
frontend/src/pages/projects/screens/spawner/useNotebookSizeState.ts
Outdated
Show resolved
Hide resolved
frontend/src/pages/projects/screens/spawner/useNotebookSizeState.ts
Outdated
Show resolved
Hide resolved
/uncc Christian is on the review side of things now. I'll step out. Tag me if you need me. |
c4e9e60
to
952d5c1
Compare
@christianvogt made the requested changes and the build passed. |
Retested the previous failing scenarios I noted in comments and they are now working correctly. /approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: christianvogt, dpanshug, gitdallas 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 |
Closes: https://issues.redhat.com/browse/RHOAIENG-578
Description
When an administrator updates the notebook sizes, the container size now changes to
Custom
instead ofUnknown
.Workbenches table(not expanded):
Workbenches table(expanded):
Edit workbenches for deleted notebook sizes(expanded):
How Has This Been Tested?
notebookSizes
inOdhDashboardConfig
(for example)Custom
in theContainer size
. Expand the table row and seelimits
andrequests
.limits
andrequests
.Test Impact
Added e2e test
Request review criteria:
Self checklist (all need to be checked):
If you have UI changes:
After the PR is posted & before it merges:
main
.