[JENKINS-49332] Better error messages

src/main/java/org/jenkinsci/plugins/github/config/GitHubServerConfig.java

@@ -227,7 +227,7 @@ public void setClientCacheSize(int clientCacheSize) {
     /**
      * @return cached GH client or null
      */
-    private GitHub getCachedClient() {
+    protected synchronized GitHub getCachedClient() {
         return cachedClient;
     }
 
@@ -236,7 +236,7 @@ private GitHub getCachedClient() {
      *
      * @param cachedClient updated client. Maybe null to invalidate cache
      */
-    private synchronized void setCachedClient(GitHub cachedClient) {
+    protected synchronized void setCachedClient(GitHub cachedClient) {
         this.cachedClient = cachedClient;
     }
 

src/main/java/org/jenkinsci/plugins/github/webhook/WebhookManager.java

@@ -11,6 +11,7 @@
 import org.jenkinsci.plugins.github.admin.GitHubHookRegisterProblemMonitor;
 import org.jenkinsci.plugins.github.config.HookSecretConfig;
 import org.jenkinsci.plugins.github.extension.GHEventsSubscriber;
+import org.jenkinsci.plugins.github.util.FluentIterableWrapper;
 import org.jenkinsci.plugins.github.util.misc.NullSafeFunction;
 import org.jenkinsci.plugins.github.util.misc.NullSafePredicate;
 import org.kohsuke.github.GHEvent;
@@ -31,7 +32,6 @@
 import java.util.Set;
 
 import static com.cloudbees.jenkins.GitHubRepositoryNameContributor.parseAssociatedNames;
-import static com.google.common.base.Preconditions.checkNotNull;
 import static com.google.common.base.Predicates.notNull;
 import static com.google.common.base.Predicates.or;
 import static java.lang.String.format;
@@ -143,10 +143,10 @@ public void run() {
      */
     public void unregisterFor(GitHubRepositoryName name, List<GitHubRepositoryName> aliveRepos) {
         try {
-            GHRepository repo = checkNotNull(
-                    from(name.resolve(allowedToManageHooks())).firstMatch(withAdminAccess()).orNull(),
-                    "There are no credentials with admin access to manage hooks on %s", name
-            );
+            GHRepository repo = repoWithWebhookAccess(name);
+            if (repo == null) {
+                return;
+            }
 
             LOGGER.debug("Check {} for redundant hooks...", repo);
 
@@ -165,6 +165,22 @@ public void unregisterFor(GitHubRepositoryName name, List<GitHubRepositoryName>
         }
     }
 
+    private GHRepository repoWithWebhookAccess(GitHubRepositoryName name) {
+        FluentIterableWrapper<GHRepository> reposAllowedtoManageWebhooks = from(name.resolve(allowedToManageHooks()));
+        if (!reposAllowedtoManageWebhooks.first().isPresent()) {
+            LOGGER.debug("There are no github repos configured to allow webhook management for: {}", name);
+            return null;
+        }
+        com.google.common.base.Optional<GHRepository> repoWithAdminAccess = reposAllowedtoManageWebhooks
+                .firstMatch(withAdminAccess());
+        if (!repoWithAdminAccess.isPresent()) {
+            LOGGER.debug("None of the github repos configured have admin access for: {}", name);
+            return null;
+        }
+        GHRepository repo = repoWithAdminAccess.get();
+        return repo;
+    }
+
     /**
      * Main logic of {@link #registerFor(Item)}.
      * Updates hooks with replacing old ones with merged new ones
@@ -178,10 +194,10 @@ protected Function<GitHubRepositoryName, GHHook> createHookSubscribedTo(final Li
             @Override
             protected GHHook applyNullSafe(@Nonnull GitHubRepositoryName name) {
                 try {
-                    GHRepository repo = checkNotNull(
-                            from(name.resolve(allowedToManageHooks())).firstMatch(withAdminAccess()).orNull(),
-                            "There are no credentials with admin access to manage hooks on %s", name
-                    );
+                    GHRepository repo = repoWithWebhookAccess(name);
+                    if (repo == null) {
+                        return null;
+                    }
 
                     Validate.notEmpty(events, "Events list for hook can't be empty");
 

src/test/java/org/jenkinsci/plugins/github/admin/GitHubHookRegisterProblemMonitorTest.java

@@ -6,20 +6,28 @@
 import hudson.model.Item;
 import hudson.plugins.git.GitSCM;
 import org.jenkinsci.plugins.github.extension.GHSubscriberEvent;
+import org.jenkinsci.plugins.github.GitHubPlugin;
+import org.jenkinsci.plugins.github.config.GitHubServerConfig;
 import org.jenkinsci.plugins.github.extension.GHEventsSubscriber;
 import org.jenkinsci.plugins.github.webhook.WebhookManager;
 import org.jenkinsci.plugins.github.webhook.WebhookManagerTest;
 import org.jenkinsci.plugins.github.webhook.subscriber.PingGHEventSubscriber;
 import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
+import org.junit.runner.RunWith;
 import org.jvnet.hudson.test.Issue;
 import org.jvnet.hudson.test.JenkinsRule;
 import org.jvnet.hudson.test.recipes.LocalData;
 import org.kohsuke.github.GHEvent;
+import org.kohsuke.github.GHRepository;
+import org.kohsuke.github.GitHub;
+import org.mockito.Mock;
+import org.mockito.runners.MockitoJUnitRunner;
 
 import javax.inject.Inject;
 import java.io.IOException;
+import java.util.Arrays;
 import java.util.Collections;
 
 import static com.cloudbees.jenkins.GitHubRepositoryName.create;
@@ -32,14 +40,17 @@
 import static org.hamcrest.Matchers.notNullValue;
 import static org.hamcrest.Matchers.nullValue;
 import static org.junit.Assert.assertThat;
+import static org.mockito.Mockito.when;
 
 /**
  * @author lanwen (Merkushev Kirill)
  */
 @Issue("JENKINS-24690")
+@RunWith(MockitoJUnitRunner.class)
 public class GitHubHookRegisterProblemMonitorTest {
     private static final GitHubRepositoryName REPO = new GitHubRepositoryName("host", "user", "repo");
-    private static final GitSCM REPO_GIT_SCM = new GitSCM("git://host/user/repo.git");
+    private static final String REPO_GIT_URI = "host/user/repo.git";
+    private static final GitSCM REPO_GIT_SCM = new GitSCM("git://"+REPO_GIT_URI);
 
     private static final GitHubRepositoryName REPO_FROM_PING_PAYLOAD = create("https://github.com/lanwen/test");
 
@@ -55,9 +66,26 @@ public class GitHubHookRegisterProblemMonitorTest {
     @Rule
     public JenkinsRule jRule = new JenkinsRule();
 
+    @Mock
+    private GitHub github;
+    @Mock
+    private GHRepository ghRepository;
+
+    class GitHubServerConfigForTest extends GitHubServerConfig {
+        public GitHubServerConfigForTest(String credentialsId) {
+            super(credentialsId);
+            this.setCachedClient(github);
+        }
+    }
+
     @Before
     public void setUp() throws Exception {
         jRule.getInstance().getInjector().injectMembers(this);
+        GitHubServerConfig config = new GitHubServerConfigForTest("");
+        config.setApiUrl("http://" + REPO_GIT_URI);
+        GitHubPlugin.configuration().setConfigs(Arrays.asList(config));
+        when(github.getRepository("user/repo")).thenReturn(ghRepository);
+        when(ghRepository.hasAdminAccess()).thenReturn(true);
     }
 
     @Test
@@ -149,20 +177,44 @@ public void shouldReportAboutHookProblemOnRegister() throws IOException {
         job.addTrigger(new GitHubPushTrigger());
         job.setScm(REPO_GIT_SCM);
 
+        when(github.getRepository("user/repo"))
+                .thenThrow(new RuntimeException("shouldReportAboutHookProblemOnRegister"));
         WebhookManager.forHookUrl(WebhookManagerTest.HOOK_ENDPOINT)
                 .registerFor((Item) job).run();
 
         assertThat("should reg problem", monitor.isProblemWith(REPO), is(true));
     }
 
     @Test
-    public void shouldReportAboutHookProblemOnUnregister() {
+    public void shouldNotReportAboutHookProblemOnRegister() throws IOException {
+        FreeStyleProject job = jRule.createFreeStyleProject();
+        job.addTrigger(new GitHubPushTrigger());
+        job.setScm(REPO_GIT_SCM);
+
+        WebhookManager.forHookUrl(WebhookManagerTest.HOOK_ENDPOINT)
+                .registerFor((Item) job).run();
+
+        assertThat("should reg problem", monitor.isProblemWith(REPO), is(false));
+    }
+
+    @Test
+    public void shouldReportAboutHookProblemOnUnregister() throws IOException {
+        when(github.getRepository("user/repo"))
+                .thenThrow(new RuntimeException("shouldReportAboutHookProblemOnUnregister"));
         WebhookManager.forHookUrl(WebhookManagerTest.HOOK_ENDPOINT)
                 .unregisterFor(REPO, Collections.<GitHubRepositoryName>emptyList());
 
         assertThat("should reg problem", monitor.isProblemWith(REPO), is(true));
     }
 
+    @Test
+    public void shouldNotReportAboutHookAuthProblemOnUnregister() {
+        WebhookManager.forHookUrl(WebhookManagerTest.HOOK_ENDPOINT)
+                .unregisterFor(REPO, Collections.<GitHubRepositoryName>emptyList());
+
+        assertThat("should not reg problem", monitor.isProblemWith(REPO), is(false));
+    }
+
     @Test
     public void shouldResolveOnPingHook() {
         monitor.registerProblem(REPO_FROM_PING_PAYLOAD, new IOException());

src/test/java/org/jenkinsci/plugins/github/webhook/WebhookManagerTest.java

@@ -86,7 +86,7 @@ public class WebhookManagerTest {
     @Test
     public void shouldDoNothingOnNoAdminRights() throws Exception {
         manager.unregisterFor(nonactive, newArrayList(active));
-        verify(manager, times(1)).withAdminAccess();
+        verify(manager, never()).withAdminAccess();
         verify(manager, never()).fetchHooks();
     }