-
Notifications
You must be signed in to change notification settings - Fork 6.5k
fix: hydration errors not set on applications #24755
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: hydration errors not set on applications #24755
Conversation
Signed-off-by: Alexandre Gaudreault <[email protected]>
Signed-off-by: Alexandre Gaudreault <[email protected]>
Signed-off-by: Alexandre Gaudreault <[email protected]>
Signed-off-by: Alexandre Gaudreault <[email protected]>
Signed-off-by: Alexandre Gaudreault <[email protected]>
Signed-off-by: Alexandre Gaudreault <[email protected]>
Signed-off-by: Alexandre Gaudreault <[email protected]>
❌ Preview Environment deleted from BunnyshellAvailable commands (reply to this comment):
|
Signed-off-by: Alexandre Gaudreault <[email protected]>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #24755 +/- ##
=========================================
Coverage ? 60.77%
=========================================
Files ? 404
Lines ? 66217
Branches ? 0
=========================================
Hits ? 40242
Misses ? 22725
Partials ? 3250 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Alexandre Gaudreault <[email protected]>
| func (h *Hydrator) setAppHydratorError(app *appv1.Application, err error) { | ||
| // if the operation is not in progress, we do not update the status | ||
| if app.Status.SourceHydrator.CurrentOperation.Phase != appv1.HydrateOperationPhaseHydrating { | ||
| return |
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.
Maybe log a warning too? If we hit this line, I think we've encountered a version of the race condition described above.
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.
i'll leave the stuff for the race condition to be handled in another PR. This is to make sure that while iterating on the app to set an error, we do not set a generic error if we've already set an app specific error
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.
Do you have immediate plans to work on the race condition? If not, I think a log would be important to help people understand why their app is misbehaving.
Co-authored-by: Michael Crenshaw <[email protected]> Signed-off-by: Alexandre Gaudreault <[email protected]>
Co-authored-by: Michael Crenshaw <[email protected]> Signed-off-by: Alexandre Gaudreault <[email protected]>
Signed-off-by: Alexandre Gaudreault <[email protected]>
…argo-cd into handle-hydration-error
Signed-off-by: Alexandre Gaudreault <[email protected]>
Co-authored-by: Michael Crenshaw <[email protected]> Signed-off-by: Alexandre Gaudreault <[email protected]>
Co-authored-by: Michael Crenshaw <[email protected]> Signed-off-by: Alexandre Gaudreault <[email protected]>
Signed-off-by: Alexandre Gaudreault <[email protected]>
…argo-cd into handle-hydration-error
Signed-off-by: Alexandre Gaudreault <[email protected]>
|
❌ Cherry-pick failed for 3.2. Please check the workflow logs for details. |
Signed-off-by: Alexandre Gaudreault <[email protected]> Co-authored-by: Michael Crenshaw <[email protected]> Signed-off-by: Alexandre Gaudreault <[email protected]>
Signed-off-by: Alexandre Gaudreault <[email protected]> Co-authored-by: Michael Crenshaw <[email protected]>
When the hydration "group" is processed, we must have as little errors as possible before retrieving the applications that are part of that group. Any errors that happen can only be logged in the controller, and the users will not have any feedback on their applications.
Once we have the applications, we can update the hydration status of the applications that failed with the correct error. We must also update all other applications in the same group with an error that will help the user identify the applciation in causing the hydration to fail.