-
Notifications
You must be signed in to change notification settings - Fork 780
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
refactor: loggers in webhook handlers #2786
Conversation
Signed-off-by: Alex Pana <8968914+acpana@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.
Variable renaming makes this PR too verbose to review as-is.
Signed-off-by: Alex Pana <8968914+acpana@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.
LGTM, assuming the comment about needing to clobber am.log() in the audit manager gets addressed.
Signed-off-by: Alex Pana <8968914+acpana@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.
Few nits, otherwise LGTM
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #2786 +/- ##
==========================================
- Coverage 52.90% 52.82% -0.08%
==========================================
Files 132 132
Lines 11629 11631 +2
==========================================
- Hits 6152 6144 -8
- Misses 4999 5008 +9
- Partials 478 479 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>
Per a comment made on the stats PR:
#2686 (comment)
this patch primarily adds
log
objects on the webhook handlers.