search

3rd-party-integrations / github-team-sync

Sync GitHub teams to groups in Active Directory, LDAP, Okta, OneLogin or AzureAD when using any authentication method for GitHub.
MIT License
199 stars 67 forks source link

Improve Logging on errors #86

Open Chocrates opened 3 years ago

Chocrates commented 3 years ago

In some places we log exceptions and continue forward, here for example https://github.com/github/github-team-sync/blob/5d50752e9f1b812a9ddfe9f97ee666d3c3f349ac/githubapp/ldap.py#L87

Some of the errors printed currently are not that useful "mail" for instance when the actual exception is a missing attribute on the object,

We should clean this up.

anonymousr007 commented 3 years ago

Hi @Chocrates, I'm Rishabh, Can you please elaborate this issue?

Chocrates commented 3 years ago

Thanks @anonymousr007, in many places we log the exception directly. The message that the exception throws are things like "mail" or other tersely worded errors. See https://github.com/github/github-team-sync/blob/a18ca46a12b251c27e0582f769e073e0a0d24fad/githubapp/ldap.py#L87

It looks like this was mostly fixed in the latest code to print stack traces https://github.com/github/github-team-sync/blob/main/app.py#L94

If you'd like you can use this one to focus on rewriting all the logging to use a unified pattern, possibly using logging see https://github.com/github/github-team-sync/blob/main/githubapp/okta.py#L7

I don't have a strong opinion on how logging gets done though, as long as we get stack traces for exceptions :)

cc: @primetheus