bugfix(kstatus): do not wait forever on failed resources#31730
bugfix(kstatus): do not wait forever on failed resources#31730scottrigby merged 1 commit intohelm:mainfrom
Conversation
26679af to
1f86ca0
Compare
|
I have run the entire test suite of Flux's helm-controller with this fix and it passed 👍 https://github.com/fluxcd/helm-controller/actions/runs/21015460312/job/60419627508 |
1f86ca0 to
c2a4f53
Compare
AustinAbro321
left a comment
There was a problem hiding this comment.
This is a nice improvement. Thanks
gjenkins8
left a comment
There was a problem hiding this comment.
Some comments, nothing major
c2a4f53 to
3192701
Compare
Signed-off-by: Matheus Pimenta <matheuscscp@gmail.com>
3192701 to
bbec77c
Compare
|
@gjenkins8 All fixed, pls take another look 🙏 |
There was a problem hiding this comment.
Pull request overview
This PR fixes an issue where Helm would wait indefinitely for resources in a Failed state. Since Failed is a terminal state in kstatus that won't transition to Current without manual intervention, the wait logic has been updated to exit early when resources fail.
Changes:
- Modified
statusObserverto exclude failed resources from blocking the wait when waiting for resources to become current - Refactored error handling in wait functions to always report non-ready resources rather than only on context timeout
- Updated error message format to include namespace and be more consistent across different wait functions
- Added comprehensive test coverage for failed resource scenarios
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| pkg/kube/statuswait.go | Added logic to treat Failed status as terminal and exclude from waiting; refactored error reporting to be more comprehensive |
| pkg/kube/statuswait_test.go | Added jobFailedManifest constant; created TestStatusWaitWithFailedResources with multiple test cases; updated all tests to use string-based error matching instead of error comparison; fixed trailing whitespace |
| pkg/kube/client_test.go | Added createManifest helper function; refactored test setup to use fake dynamic clients; modernized map type from interface{} to any |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
scottrigby
left a comment
There was a problem hiding this comment.
Really good catch. I'm a little surprised no one noticed this before now, since it's been here since v4.0.0.
|
@scottrigby can we have it in upcoming |
What this PR does / why we need it:
Fixes: #31729
Special notes for your reviewer:
In kstatus
Failedis a terminal state. There's no point in making Helm wait on it forever, as it will never transition toCurrentwithout intervention.If applicable:
docs neededlabel should be applied if so)