-
Notifications
You must be signed in to change notification settings - Fork 819
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: missing notification on error #3076
fix: missing notification on error #3076
Conversation
Signed-off-by: zachaller <zachaller@users.noreply.github.com>
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #3076 +/- ##
==========================================
- Coverage 81.73% 81.71% -0.02%
==========================================
Files 134 134
Lines 20406 20416 +10
==========================================
+ Hits 16678 16683 +5
- Misses 2869 2873 +4
- Partials 859 860 +1
☔ View full report in Codecov by Sentry. |
Signed-off-by: zachaller <zachaller@users.noreply.github.com>
Signed-off-by: zachaller <zachaller@users.noreply.github.com>
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.
Pls check my comments
utils/record/record.go
Outdated
} | ||
log.Infof("Trigger %s result: %v", trigger, res) | ||
log.Infof("trigger %s result: %v", trigger, res) |
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.
nit: Argo CD logs we usually start with uppercase.
} | ||
|
||
emptyCondition := hash("") | ||
|
||
// We should not return in these loops because we want other configured notifications to still send if they can. | ||
errors := []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.
Not sure if I am understanding this correctly but I can't find where this list of errors
is returned..
Signed-off-by: zachaller <zachaller@users.noreply.github.com>
Signed-off-by: zachaller <zachaller@users.noreply.github.com>
Signed-off-by: zachaller <zachaller@users.noreply.github.com>
Signed-off-by: zachaller <zachaller@users.noreply.github.com>
Kudos, SonarCloud Quality Gate passed!
|
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.
LGTM
* fix: missing notification on error Signed-off-by: zachaller <zachaller@users.noreply.github.com> * fix tests Signed-off-by: zachaller <zachaller@users.noreply.github.com> * aggregate errors Signed-off-by: zachaller <zachaller@users.noreply.github.com> * fix bad stat counts Signed-off-by: zachaller <zachaller@users.noreply.github.com> * return errors Signed-off-by: zachaller <zachaller@users.noreply.github.com> * fix tests on return of errors Signed-off-by: zachaller <zachaller@users.noreply.github.com> * change case on logs Signed-off-by: zachaller <zachaller@users.noreply.github.com> * missed a case Signed-off-by: zachaller <zachaller@users.noreply.github.com> --------- Signed-off-by: zachaller <zachaller@users.noreply.github.com>
* fix: missing notification on error Signed-off-by: zachaller <zachaller@users.noreply.github.com> * fix tests Signed-off-by: zachaller <zachaller@users.noreply.github.com> * aggregate errors Signed-off-by: zachaller <zachaller@users.noreply.github.com> * fix bad stat counts Signed-off-by: zachaller <zachaller@users.noreply.github.com> * return errors Signed-off-by: zachaller <zachaller@users.noreply.github.com> * fix tests on return of errors Signed-off-by: zachaller <zachaller@users.noreply.github.com> * change case on logs Signed-off-by: zachaller <zachaller@users.noreply.github.com> * missed a case Signed-off-by: zachaller <zachaller@users.noreply.github.com> --------- Signed-off-by: zachaller <zachaller@users.noreply.github.com> Signed-off-by: Philip Clark <philip.clark@iterable.com>
* fix: missing notification on error Signed-off-by: zachaller <zachaller@users.noreply.github.com> * fix tests Signed-off-by: zachaller <zachaller@users.noreply.github.com> * aggregate errors Signed-off-by: zachaller <zachaller@users.noreply.github.com> * fix bad stat counts Signed-off-by: zachaller <zachaller@users.noreply.github.com> * return errors Signed-off-by: zachaller <zachaller@users.noreply.github.com> * fix tests on return of errors Signed-off-by: zachaller <zachaller@users.noreply.github.com> * change case on logs Signed-off-by: zachaller <zachaller@users.noreply.github.com> * missed a case Signed-off-by: zachaller <zachaller@users.noreply.github.com> --------- Signed-off-by: zachaller <zachaller@users.noreply.github.com> Signed-off-by: Philip Clark <philip.clark@iterable.com>
* fix: missing notification on error Signed-off-by: zachaller <zachaller@users.noreply.github.com> * fix tests Signed-off-by: zachaller <zachaller@users.noreply.github.com> * aggregate errors Signed-off-by: zachaller <zachaller@users.noreply.github.com> * fix bad stat counts Signed-off-by: zachaller <zachaller@users.noreply.github.com> * return errors Signed-off-by: zachaller <zachaller@users.noreply.github.com> * fix tests on return of errors Signed-off-by: zachaller <zachaller@users.noreply.github.com> * change case on logs Signed-off-by: zachaller <zachaller@users.noreply.github.com> * missed a case Signed-off-by: zachaller <zachaller@users.noreply.github.com> --------- Signed-off-by: zachaller <zachaller@users.noreply.github.com> Signed-off-by: Philip Clark <philip.clark@iterable.com>
Notifications would stop processing if any error happened on a single notification 'API' configuration, we should continue processing other notifications so that we do not miss sending a notification that is properly configured.
This also fixes an incorrect prometheus stat, we where only incrementing errors and success counts on api creations not at the sending and erroring of the actually execution of the notification.