Problem: [JENKINS-60901] GitHub manages hooks even when it has not…

[jimklimov]

…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 jimklimov@gmail.com

---

This change is 

[jimklimov]

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.

[jimklimov]

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.

[jimklimov]

As somewhat usual, I spent a lot more effort fighting self-tests than adding the actual feature. Sigh. ;)

[jimklimov]

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 :)

[KostyaSha]

it's not a problem if it configured not to manage, so message is not need here

[jimklimov]

I agree. But without this, the selftests fail in many places because they expect a problem to be registered. And I am not aware enough about the intricacies of the plugin to change this too (neither how to do so OTOH, nor if eventually doing so would break/neuter some other tests).

[KostyaSha]

so null is a valid case here, but checkNotNull is throwing NPE. Just remove checkNotNull and return from method without anything.

[KostyaSha]

Optionally for debug purposes print into

   GHRepository repo = from(name.resolve(allowedToManageHooks())).firstMatch(withAdminAccess()).orNull();
   if (isNull(repo)) {
       LOGGER.debug("Skipped removing GitHub webhook for {} because not configured to Manage Hooks or "
                            + "there are no credentials with admin access to manage hooks", name);
       return;
   }

[jimklimov]

I am not sure I got this comment properly, or my implementation change for that matter :)

I believe the original code had null as valid case of the not-finding-a-match via orNull(), and handling that via checkNotNull().

My intention was to pick out the same match-or-not logic into the repo variable, which same way can end up null, but do the same checkNotNull handling after checking if we are at all configured to manage the hooks.

IIRC resolving of the repo with each and any call to register/unregister routines was also dictated by selftests somehow. My first shot was to check enablement of webhook management, log and return if it is not enabled - and then the tests failed until I changed the call chain to what is here.

[KostyaSha]

So registration of error should happen only for available allowedToManage repos but without admin access

[KostyaSha]

repo will be always null if there is no repos in iterator

[jimklimov]

Hm, makes sense :)

[jimklimov]

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 :)

[oleg-nenashev]

Is it still relevant @jimklimov ? we have a minor merge conflict but looks good overall

[jimklimov]

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 <#226 (comment)>, 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> .

[avidspartan1]

I'm also seeing the behavior this PR is addressing. Any status here?

[jimklimov]

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.