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

…en configured to do it (and complains a lot in the log) 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]>
jenkinsci · Feb 28, 2020 · 99378b0 · 99378b0
1 parent 51dc96b
commit 99378b0
Show file tree

Hide file tree

Showing 2 changed files with 17 additions and 0 deletions.
diff --git a/src/main/java/org/jenkinsci/plugins/github/webhook/WebhookManager.java b/src/main/java/org/jenkinsci/plugins/github/webhook/WebhookManager.java
@@ -141,6 +141,11 @@ public void run() {
      */
     public void unregisterFor(GitHubRepositoryName name, List<GitHubRepositoryName> aliveRepos) {
         try {
+            if (name.resolve(allowedToManageHooks()).firstMatch().isPresent()) {
+                LOGGER.debug("Skipped adding GitHub webhook for {} because not configured to Manage Hooks", name);
+                return;
+            }
+
             GHRepository repo = checkNotNull(
                     from(name.resolve(allowedToManageHooks())).firstMatch(withAdminAccess()).orNull(),
                     "There are no credentials with admin access to manage hooks on %s", name
@@ -176,6 +181,12 @@ protected Function<GitHubRepositoryName, GHHook> createHookSubscribedTo(final Li
             @Override
             protected GHHook applyNullSafe(@Nonnull GitHubRepositoryName name) {
                 try {
+                    if (name.resolve(allowedToManageHooks()).firstMatch().isPresent()) {
+                        LOGGER.debug("Skipped adding GitHub webhook for {} because not configured to Manage Hooks",
+                                name);
+                        return null;
+                    }
+
                     GHRepository repo = checkNotNull(
                             from(name.resolve(allowedToManageHooks())).firstMatch(withAdminAccess()).orNull(),
                             "There are no credentials with admin access to manage hooks on %s", name

diff --git a/src/test/java/org/jenkinsci/plugins/github/admin/GitHubHookRegisterProblemMonitorTest.java b/src/test/java/org/jenkinsci/plugins/github/admin/GitHubHookRegisterProblemMonitorTest.java
@@ -149,6 +149,9 @@ public void shouldReportAboutHookProblemOnRegister() throws IOException {
         job.addTrigger(new GitHubPushTrigger());
         job.setScm(REPO_GIT_SCM);
 
+        GitHubServerConfig conf = new GitHubServerConfig(WebhookManagerTest.HOOK_ENDPOINT);
+        conf.setManageHooks(true);
+
         WebhookManager.forHookUrl(WebhookManagerTest.HOOK_ENDPOINT)
                 .registerFor((Item) job).run();
 
@@ -157,6 +160,9 @@ public void shouldReportAboutHookProblemOnRegister() throws IOException {
 
     @Test
     public void shouldReportAboutHookProblemOnUnregister() {
+        GitHubServerConfig conf = new GitHubServerConfig(WebhookManagerTest.HOOK_ENDPOINT);
+        conf.setManageHooks(true);
+
         WebhookManager.forHookUrl(WebhookManagerTest.HOOK_ENDPOINT)
                 .unregisterFor(REPO, Collections.<GitHubRepositoryName>emptyList());