-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Adds support for showing warning icon in StatefulSet #5055
Adds support for showing warning icon in StatefulSet #5055
Conversation
Signed-off-by: Harkishen Singh <harkishensingh@hotmail.com>
Welcome @Harkishen-Singh! |
/assign @floreks |
I'd argue that when desired nr of pods is different than actual number it does not necessarily mean there is something wrong with it. It might be upscaling/downscaling. This case is definitely not a warning, more like pending. |
Codecov Report
@@ Coverage Diff @@
## master #5055 +/- ##
=======================================
Coverage 45.42% 45.42%
=======================================
Files 214 214
Lines 10078 10078
Branches 100 100
=======================================
Hits 4578 4578
Misses 5232 5232
Partials 268 268 Continue to review full report at Codecov.
|
@floreks sure. I actually implemented that way since I did not any other notable things in response from the API ( What do you suggest should be the recommended way here? @floreks |
Our error state is actually a warning state. If there are warning/error events associated with any resource, an error icon will appear and proper message will be available. I don't think a warning status is needed here. |
@floreks does that mean, the green check in |
This is weird because 0/1 should mean that there is a pending pod waiting to be created. You can extend the pending/success checks to check for pending pods and if desired/actual pods match. In such case it should show pending status. |
Got your point. However, in this case, the pods will forever remain in a pending case. Hence I thought that showing a warning or alert icon will be required, as the issue says. We should also note that the StatefulSets pie chart in the Workload status shows |
Signed-off-by: Harkishen Singh <harkishensingh@hotmail.com>
@floreks updated the logic as per discussion. |
src/app/frontend/common/components/resourcelist/statefulset/component.ts
Outdated
Show resolved
Hide resolved
src/app/frontend/common/components/resourcelist/statefulset/component.ts
Outdated
Show resolved
Hide resolved
Signed-off-by: Harkishen Singh <harkishensingh@hotmail.com>
@floreks done. |
/lgtm |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: floreks, Harkishen-Singh 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 |
Signed-off-by: Harkishen Singh harkishensingh@hotmail.com
Fixes: #4205
Explanatory Image: