github-plugin
github-plugin copied to clipboard
jenkinsci
Problem: [JENKINS-60901] GitHub manages hooks even when it has not…
…been configured to do it (and complains a lot in the log)
https://issues.jenkins-ci.org/browse/JENKINS-60901
Solution: Check if configured to manage hooks before trying to register or unregister them (which in case of no credential set up, results in a meaningful exception message plus a lengthy but somewhat useless stacktrace). If nothing else, this reduces I/O and storage overhead due to hook not configured and disabled.
Signed-off-by: Jim Klimov [email protected]
This change is 
In our use-case, there are a lot of stack traces (probably the majority of what the jenkins instance logs) while there is no credential set up AND the "Manage hooks" is unchecked. So it is supposed to not try managing, somewhat intentionally (our Jenkins is inside corporate perimeter, Github can't access it anyway).
Note that according to help message for the "Manage Hooks" checkbox, there may be other places in code where it would check for credentials:
Will this configuration be used to manage credentials for repositories where it has admin rights? If unchecked, this credentials still can be used to manipulate commit statuses, but will be ignored to manage hooks.
But the noisy (and in our case pointless) error message is only associated with register/unregister activity.
Help would be welcome :(
Despite a number of guesswork iterations, this fix fails on org.jenkinsci.plugins.github.admin.GitHubHookRegisterProblemMonitorTest.shouldReportAboutHookProblemOnUnregister() and ...OnRegister() tests.
By the logic, there should be no registered failure if the management of hooks is not enabled, and no skipping (so attempt, maybe fail) if it is enabled. But I can't just get that processed right.
Maybe time to give up and register a "failure" that we can't register because we are told not to, which is bogus.
As somewhat usual, I spent a lot more effort fighting self-tests than adding the actual feature. Sigh. ;)
After applying the custom build of plugin and restarting our master, I do not see the log noise anymore.
Would be nice if someone could check that the resulting plugin also can actually set up the hooks when checkbox tells it to :)
Sorry I fell out of the loop, hope to get back to that soon. Or feel free to follow up with PRing wanted cleanups on top of that :)
Is it still relevant @jimklimov ? we have a minor merge conflict but looks good overall
I'm not with computers for a month, so hope to check by October :)
Jim
On Sat, Aug 14, 2021, 17:05 Oleg Nenashev @.***> wrote:
Is it still relevant @jimklimov https://github.com/jimklimov ? we have a minor merge conflict but looks good overall
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/jenkinsci/github-plugin/pull/226#issuecomment-898905429, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAMPTFC6AF37VWKZRT6EAJ3T42A3DANCNFSM4K5SICPA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email .
Looking at the merge conflicts, it seems that #225 introduced a repoWithWebhookAccess() in src/main/java/org/jenkinsci/plugins/github/webhook/WebhookManager.java with similar effect.
I don't think I've been running vanilla master versions of the plugin lately, however, so would welcome a thorough confirmation whether with recent Jenkins core server and release of this plugin, the problem is gone (and my PR obsoleted) or not.