-
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
Cluster storage that was disconnected from the workbench doesn't update it's size info #3481
Cluster storage that was disconnected from the workbench doesn't update it's size info #3481
Conversation
@kaedward please check the wordings for this popover. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3481 +/- ##
==========================================
- Coverage 85.37% 85.34% -0.03%
==========================================
Files 1352 1354 +2
Lines 30869 31119 +250
Branches 8616 8691 +75
==========================================
+ Hits 26353 26558 +205
- Misses 4516 4561 +45
... and 115 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
20651d5
to
f57af52
Compare
Wording from katie "To complete the storage size update, connect and run the workbench." |
f57af52
to
d77bc2b
Compare
@dpanshug @Gkrumbach07 As I mentioned in the RHOAIENG-527, it would be better to show the same alert when users change the storage size in the editing form. What do you think? |
We already show a warning there for the following: unbound pvcs and cannot reduce size of pvc This one would conflict with both of these as it could be shown when both existing warnings are shown. We could just stack the warning alerts on top of each other |
@Gkrumbach07 Do you mean the warning under the number input field? If so, this warning seems to appear only in storage that is connected to the workbench. Right? (correct me if I am wrong) |
@xianli123 you are right. this should be fine then |
@Gkrumbach07 the change could be:
Does it make sense? cc @dpanshug @kywalker-rh |
@xianli123 Currently if the storage is never connected to any workbench, the user can't edit the size. Do you mean we should enable it? cc @Gkrumbach07 |
@dpanshug It's weird. I can't reproduce the warning you shared, even though the storage didn't connect to any workbench. The screenshot below shows what I got. |
@xianli123 to reproduce it. You have to create a new cluster storage which never has connected to a workbench. You will see the field disabled. But when the first connection is made, and you remove it, the field will always be enabled. It is a sort of weird behavior. |
So this warning will show when a workbench has its size changed and is either not connected to a workbench or its connected workbenches are not running. |
d77bc2b
to
4e84c18
Compare
@xianli123 @Gkrumbach07 PR updated, pls check the description for latest screenshots |
Thanks @dpanshug! LGTM! |
@dpanshug can you rebase |
4e84c18
to
625ca8e
Compare
…te it's size info
625ca8e
to
a23a924
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.
thanks
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED 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 |
RHOAIENG-527
Description
When the storage size is updated but no pod connected, the size doesn't get updated on UI. So added a condition which will display the updated size with warning.
If the storage is connected to workbenches
If the storage is not connected to any workbench
How Has This Been Tested?
Test Impact
Added cypress test to check the warning when cluster storage size is updated but no workbench is connected.
Request review criteria:
Self checklist (all need to be checked):
If you have UI changes:
After the PR is posted & before it merges:
main
@xianli123 @kaedward