Skip to content
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

Log missing admin access on INFO level #299

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kutzi
Copy link
Member

@kutzi kutzi commented Sep 5, 2022

I don't know what the motivation was to log this on DEBUG only (less verbose logs?), but IMO you're missing quite important info this way in the default log level INFO.
I.e. when a new hook should be created, you only see in the logs:
INFO o.j.p.g.webhook.WebhookManager$1#run: GitHub webhooks activated for job X with [GitHubRepositoryName[...]] (events: [PUSH])
which looks like it's successful already ('webhooks activated')
but you miss the DEBUG message which says that you have a correct repo configured, but missing the admin rights
`

@kutzi
Copy link
Member Author

kutzi commented Sep 15, 2022

In fact - just ran into the same issue again - the logs on INFO level for successful and unsuccessful hook generation look exactly the same!
Only the GitHub webhooks activated for job log

@KostyaSha
Copy link
Member

The problem of info level is that on setups with thousands of repos it will spam. I usually use info only for not really repeating errors. For debug purpose admin can add extra logger (afair this is described in docs).

@kutzi
Copy link
Member Author

kutzi commented Sep 15, 2022

Is one line of log more in the case of an error really spam?
It would only log if the other line is logged as well, right?

@kutzi
Copy link
Member Author

kutzi commented Sep 15, 2022

If you have configured a user, but the user doesn't have the required permissions, I would consider it as an error.
In contrast to when you don't have configured an user at all. In which case it would currently fail with the same log as well, AFAIK

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants