-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
fix(controller/ui): fix pod with sidecar state #19843
Conversation
❌ Preview Environment deleted from BunnyshellAvailable commands (reply to this comment):
|
❌ Preview Environment deleted from BunnyshellAvailable commands (reply to this comment):
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #19843 +/- ##
=========================================
Coverage ? 56.04%
=========================================
Files ? 322
Lines ? 44768
Branches ? 0
=========================================
Hits ? 25088
Misses ? 17081
Partials ? 2599 ☔ View full report in Codecov by Sentry. |
@linghaoSu why is this still a draft? anything missing? |
I assume you also want to test in https://github.com/argoproj/argo-cd/blob/1e359c8c0d25dc3a223ab184c1d623bb4d68aa5a/controller/cache/info_test.go? Let me know if you have time to continue with this PR, otherwise I can also try to take over. We really need this fix to land. |
@alexef Sorry for the delay. I'll try to get it done this week. My fix pr in the dashboard is closed, so I'll refer to the merged fixes for this one. |
020c9de
to
79ace2c
Compare
Hi @alexef, I believe this PR is ready for review. Could you please take a look? References the dashboard pr: https://github.com/kubernetes/dashboard/pull/9483/files#diff-207a2dad9e596869b3612d332964328e081f20fb9cbeb6f9bbb41dfa13472152R39-R124 |
79ace2c
to
5cbbc7d
Compare
eefd2ce
to
b08fc2e
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.
couple of questions
I included restartable initContainers in the total container calculation, following the approach used in kubectl. @alexef If it's convenient, could you please review it again? |
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.
great. now we only need an approver :)
Signed-off-by: linghaoSu <[email protected]>
…play Signed-off-by: linghaoSu <[email protected]>
8ca555d
to
75d6b80
Compare
Hi @crenshaw-dev, I believe this PR is ready for review. Could you please take a look? This issue has 16 thumbs up, so it might be helpful to address it. If you spot any potential issues with this PR, please let me know, and I'll be happy to work on fixing them. Thanks! |
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 for the PR! Just a few nitpicks and requests for code coverage.
Co-authored-by: Michael Crenshaw <[email protected]> Signed-off-by: Linghao Su <[email protected]>
Signed-off-by: linghaoSu <[email protected]>
Signed-off-by: linghaoSu <[email protected]>
Hi @crenshaw-dev, Thank you for your suggestions! I believe I've addressed all the comments. Could you please take another look? |
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.
This LGTM, will follow up with the UI fix. Thanks!
/cherry-pick release-2.13 |
Cherry-pick failed with |
@linghaoSu would you mind opening cherry-picks for 2.10-2.13? |
* fix(controller): change pod status calculate with sidecar Signed-off-by: linghaoSu <[email protected]> * fix(controller): add restartable sidecar count in total container display Signed-off-by: linghaoSu <[email protected]> * fix(controller): update info test case conditions Co-authored-by: Michael Crenshaw <[email protected]> Signed-off-by: Linghao Su <[email protected]> * fix(controller): add more test case to cover more conditions Signed-off-by: linghaoSu <[email protected]> * fix(ui): check is condition exist before for of Signed-off-by: linghaoSu <[email protected]> --------- Signed-off-by: linghaoSu <[email protected]> Signed-off-by: Linghao Su <[email protected]> Co-authored-by: Michael Crenshaw <[email protected]>
* fix(controller): change pod status calculate with sidecar Signed-off-by: linghaoSu <[email protected]> * fix(controller): add restartable sidecar count in total container display Signed-off-by: linghaoSu <[email protected]> * fix(controller): update info test case conditions Co-authored-by: Michael Crenshaw <[email protected]> Signed-off-by: Linghao Su <[email protected]> * fix(controller): add more test case to cover more conditions Signed-off-by: linghaoSu <[email protected]> * fix(ui): check is condition exist before for of Signed-off-by: linghaoSu <[email protected]> --------- Signed-off-by: linghaoSu <[email protected]> Signed-off-by: Linghao Su <[email protected]> Co-authored-by: Michael Crenshaw <[email protected]>
* fix(controller): change pod status calculate with sidecar Signed-off-by: linghaoSu <[email protected]> * fix(controller): add restartable sidecar count in total container display Signed-off-by: linghaoSu <[email protected]> * fix(controller): update info test case conditions Co-authored-by: Michael Crenshaw <[email protected]> Signed-off-by: Linghao Su <[email protected]> * fix(controller): add more test case to cover more conditions Signed-off-by: linghaoSu <[email protected]> * fix(ui): check is condition exist before for of Signed-off-by: linghaoSu <[email protected]> --------- Signed-off-by: linghaoSu <[email protected]> Signed-off-by: Linghao Su <[email protected]> Co-authored-by: Michael Crenshaw <[email protected]>
* fix(controller): change pod status calculate with sidecar Signed-off-by: linghaoSu <[email protected]> * fix(controller): add restartable sidecar count in total container display Signed-off-by: linghaoSu <[email protected]> * fix(controller): update info test case conditions Co-authored-by: Michael Crenshaw <[email protected]> Signed-off-by: Linghao Su <[email protected]> * fix(controller): add more test case to cover more conditions Signed-off-by: linghaoSu <[email protected]> * fix(ui): check is condition exist before for of Signed-off-by: linghaoSu <[email protected]> --------- Signed-off-by: linghaoSu <[email protected]> Signed-off-by: Linghao Su <[email protected]> Co-authored-by: Michael Crenshaw <[email protected]>
Hi @crenshaw-dev, I've manually cherry-picked this fix to 2.10-2.13. Could you please take a look at those PRs? |
* fix(controller): change pod status calculate with sidecar Signed-off-by: linghaoSu <[email protected]> * fix(controller): add restartable sidecar count in total container display Signed-off-by: linghaoSu <[email protected]> * fix(controller): update info test case conditions Co-authored-by: Michael Crenshaw <[email protected]> Signed-off-by: Linghao Su <[email protected]> * fix(controller): add more test case to cover more conditions Signed-off-by: linghaoSu <[email protected]> * fix(ui): check is condition exist before for of Signed-off-by: linghaoSu <[email protected]> --------- Signed-off-by: linghaoSu <[email protected]> Signed-off-by: Linghao Su <[email protected]> Co-authored-by: Michael Crenshaw <[email protected]> Signed-off-by: austin5219 <[email protected]>
@crenshaw-dev , However in 2.10 and 2.11, RestartPolicy is not a valid field in InitContainer, so this would cause a lint error. Perhaps we shouldn't cherry-pick this fix to those two releases. |
* fix(controller): change pod status calculate with sidecar * fix(controller): add restartable sidecar count in total container display * fix(controller): update info test case conditions * fix(controller): add more test case to cover more conditions * fix(ui): check is condition exist before for of --------- Signed-off-by: linghaoSu <[email protected]> Signed-off-by: Linghao Su <[email protected]> Co-authored-by: Michael Crenshaw <[email protected]>
* fix(controller): change pod status calculate with sidecar * fix(controller): add restartable sidecar count in total container display * fix(controller): update info test case conditions * fix(controller): add more test case to cover more conditions * fix(ui): check is condition exist before for of --------- Signed-off-by: linghaoSu <[email protected]> Signed-off-by: Linghao Su <[email protected]> Co-authored-by: Michael Crenshaw <[email protected]>
* fix(controller): change pod status calculate with sidecar Signed-off-by: linghaoSu <[email protected]> * fix(controller): add restartable sidecar count in total container display Signed-off-by: linghaoSu <[email protected]> * fix(controller): update info test case conditions Co-authored-by: Michael Crenshaw <[email protected]> Signed-off-by: Linghao Su <[email protected]> * fix(controller): add more test case to cover more conditions Signed-off-by: linghaoSu <[email protected]> * fix(ui): check is condition exist before for of Signed-off-by: linghaoSu <[email protected]> --------- Signed-off-by: linghaoSu <[email protected]> Signed-off-by: Linghao Su <[email protected]> Co-authored-by: Michael Crenshaw <[email protected]> Signed-off-by: Adrian Aneci <[email protected]>
Fixes #18725
Before:
After
Checklist: