From 6c63c1030e353fa982c102a523244e476ef3ea74 Mon Sep 17 00:00:00 2001 From: Carlos Sanchez Date: Mon, 10 Feb 2020 12:11:44 +0100 Subject: [PATCH 1/5] [JENKINS-49332] Better error messages when github repo webhook can't be created Don't throw exceptions as we are handling the error, just print to the log --- .../github/webhook/WebhookManager.java | 37 ++++++++++++++----- 1 file changed, 28 insertions(+), 9 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 index b13194df1..40a72bc6f 100644 --- a/src/main/java/org/jenkinsci/plugins/github/webhook/WebhookManager.java +++ b/src/main/java/org/jenkinsci/plugins/github/webhook/WebhookManager.java @@ -2,6 +2,7 @@ import com.cloudbees.jenkins.GitHubRepositoryName; import com.google.common.base.Function; +import com.google.common.base.Optional; import com.google.common.base.Predicate; import hudson.model.Item; import hudson.model.Job; @@ -10,6 +11,7 @@ import org.jenkinsci.plugins.github.GitHubPlugin; import org.jenkinsci.plugins.github.admin.GitHubHookRegisterProblemMonitor; 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; @@ -28,7 +30,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; @@ -141,10 +142,19 @@ public void run() { */ public void unregisterFor(GitHubRepositoryName name, List 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 - ); + FluentIterableWrapper reposAllowedtoManageWebhooks = from( + name.resolve(allowedToManageHooks())); + if (!reposAllowedtoManageWebhooks.first().isPresent()) { + LOGGER.info("There are no github repos configured to allow webhook management for: {}", name); + return; + } + Optional repoWithAdminAccess = reposAllowedtoManageWebhooks + .firstMatch(withAdminAccess()); + if (!repoWithAdminAccess.isPresent()) { + LOGGER.info("None of the github repos configured have admin access for: {}", name); + return; + } + GHRepository repo = repoWithAdminAccess.get(); LOGGER.debug("Check {} for redundant hooks...", repo); @@ -176,10 +186,19 @@ protected Function 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 - ); + FluentIterableWrapper reposAllowedtoManageWebhooks = from( + name.resolve(allowedToManageHooks())); + if (!reposAllowedtoManageWebhooks.first().isPresent()) { + LOGGER.info("There are no github repos configured to allow webhook management for: {}", name); + return null; + } + Optional repoWithAdminAccess = reposAllowedtoManageWebhooks + .firstMatch(withAdminAccess()); + if (!repoWithAdminAccess.isPresent()) { + LOGGER.info("None of the github repos configured have admin access for: {}", name); + return null; + } + GHRepository repo = repoWithAdminAccess.get(); Validate.notEmpty(events, "Events list for hook can't be empty"); From e219dfe5485c5c908070635069fc45194fac2b25 Mon Sep 17 00:00:00 2001 From: Carlos Sanchez Date: Tue, 12 May 2020 12:21:43 +0200 Subject: [PATCH 2/5] [JENKINS-49332] Refactor to avoid duplication Fix compilation and tests --- .../GitHubHookRegisterProblemMonitor.java | 4 +- .../github/webhook/WebhookManager.java | 48 ++++++++++--------- .../GitHubHookRegisterProblemMonitorTest.java | 2 +- .../github/webhook/WebhookManagerTest.java | 2 +- 4 files changed, 29 insertions(+), 27 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/github/admin/GitHubHookRegisterProblemMonitor.java b/src/main/java/org/jenkinsci/plugins/github/admin/GitHubHookRegisterProblemMonitor.java index 430951820..36bc50a92 100644 --- a/src/main/java/org/jenkinsci/plugins/github/admin/GitHubHookRegisterProblemMonitor.java +++ b/src/main/java/org/jenkinsci/plugins/github/admin/GitHubHookRegisterProblemMonitor.java @@ -86,12 +86,12 @@ public void registerProblem(GitHubRepositoryName repo, Throwable throwable) { } /** - * Used by {@link #registerProblem(GitHubRepositoryName, Throwable)} + * Registers problems. * * @param repo full named GitHub repo, if null nothing will be done * @param message message to show in the interface. Will be used default if blank */ - private void registerProblem(GitHubRepositoryName repo, String message) { + public void registerProblem(GitHubRepositoryName repo, String message) { if (repo == null) { return; } diff --git a/src/main/java/org/jenkinsci/plugins/github/webhook/WebhookManager.java b/src/main/java/org/jenkinsci/plugins/github/webhook/WebhookManager.java index 4ba1d0df7..b05ed1869 100644 --- a/src/main/java/org/jenkinsci/plugins/github/webhook/WebhookManager.java +++ b/src/main/java/org/jenkinsci/plugins/github/webhook/WebhookManager.java @@ -2,7 +2,6 @@ import com.cloudbees.jenkins.GitHubRepositoryName; import com.google.common.base.Function; -import com.google.common.base.Optional; import com.google.common.base.Predicate; import hudson.model.Item; import hudson.model.Job; @@ -144,19 +143,10 @@ public void run() { */ public void unregisterFor(GitHubRepositoryName name, List aliveRepos) { try { - FluentIterableWrapper reposAllowedtoManageWebhooks = from( - name.resolve(allowedToManageHooks())); - if (!reposAllowedtoManageWebhooks.first().isPresent()) { - LOGGER.info("There are no github repos configured to allow webhook management for: {}", name); + GHRepository repo = repoWithWebhookAccess(name); + if (repo == null) { return; } - Optional repoWithAdminAccess = reposAllowedtoManageWebhooks - .firstMatch(withAdminAccess()); - if (!repoWithAdminAccess.isPresent()) { - LOGGER.info("None of the github repos configured have admin access for: {}", name); - return; - } - GHRepository repo = repoWithAdminAccess.get(); LOGGER.debug("Check {} for redundant hooks...", repo); @@ -175,6 +165,27 @@ public void unregisterFor(GitHubRepositoryName name, List } } + private GHRepository repoWithWebhookAccess(GitHubRepositoryName name) { + FluentIterableWrapper reposAllowedtoManageWebhooks = from(name.resolve(allowedToManageHooks())); + if (!reposAllowedtoManageWebhooks.first().isPresent()) { + String msg = String.format("There are no github repos configured to allow webhook management for: %s", + name); + LOGGER.info(msg); + GitHubHookRegisterProblemMonitor.get().registerProblem(name, msg); + return null; + } + com.google.common.base.Optional repoWithAdminAccess = reposAllowedtoManageWebhooks + .firstMatch(withAdminAccess()); + if (!repoWithAdminAccess.isPresent()) { + String msg = String.format("None of the github repos configured have admin access for: %s", name); + LOGGER.info(msg); + GitHubHookRegisterProblemMonitor.get().registerProblem(name, msg); + return null; + } + GHRepository repo = repoWithAdminAccess.get(); + return repo; + } + /** * Main logic of {@link #registerFor(Item)}. * Updates hooks with replacing old ones with merged new ones @@ -188,19 +199,10 @@ protected Function createHookSubscribedTo(final Li @Override protected GHHook applyNullSafe(@Nonnull GitHubRepositoryName name) { try { - FluentIterableWrapper reposAllowedtoManageWebhooks = from( - name.resolve(allowedToManageHooks())); - if (!reposAllowedtoManageWebhooks.first().isPresent()) { - LOGGER.info("There are no github repos configured to allow webhook management for: {}", name); - return null; - } - Optional repoWithAdminAccess = reposAllowedtoManageWebhooks - .firstMatch(withAdminAccess()); - if (!repoWithAdminAccess.isPresent()) { - LOGGER.info("None of the github repos configured have admin access for: {}", name); + GHRepository repo = repoWithWebhookAccess(name); + if (repo == null) { return null; } - GHRepository repo = repoWithAdminAccess.get(); Validate.notEmpty(events, "Events list for hook can't be empty"); diff --git a/src/test/java/org/jenkinsci/plugins/github/admin/GitHubHookRegisterProblemMonitorTest.java b/src/test/java/org/jenkinsci/plugins/github/admin/GitHubHookRegisterProblemMonitorTest.java index 4bbabbf86..dc787e896 100644 --- a/src/test/java/org/jenkinsci/plugins/github/admin/GitHubHookRegisterProblemMonitorTest.java +++ b/src/test/java/org/jenkinsci/plugins/github/admin/GitHubHookRegisterProblemMonitorTest.java @@ -82,7 +82,7 @@ public void shouldNotAddNullRepo() throws Exception { @Test public void shouldNotAddNullExc() throws Exception { - monitor.registerProblem(REPO, null); + monitor.registerProblem(REPO, (Throwable) null); assertThat("should be no problems", monitor.getProblems().keySet(), empty()); } diff --git a/src/test/java/org/jenkinsci/plugins/github/webhook/WebhookManagerTest.java b/src/test/java/org/jenkinsci/plugins/github/webhook/WebhookManagerTest.java index d829d1fff..f6217fe1a 100644 --- a/src/test/java/org/jenkinsci/plugins/github/webhook/WebhookManagerTest.java +++ b/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(); } From 248b4f90e1ff1f6e55178ce76aa130ce368852d9 Mon Sep 17 00:00:00 2001 From: Carlos Sanchez Date: Wed, 13 May 2020 12:03:12 +0200 Subject: [PATCH 3/5] [JENKINS-49332] Do not register a problem if we don't have admin permissions --- .../github/config/GitHubServerConfig.java | 4 +- .../github/webhook/WebhookManager.java | 9 +-- .../GitHubHookRegisterProblemMonitorTest.java | 56 ++++++++++++++++++- 3 files changed, 58 insertions(+), 11 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/github/config/GitHubServerConfig.java b/src/main/java/org/jenkinsci/plugins/github/config/GitHubServerConfig.java index ba6f778b0..7864ab5a1 100644 --- a/src/main/java/org/jenkinsci/plugins/github/config/GitHubServerConfig.java +++ b/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 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; } diff --git a/src/main/java/org/jenkinsci/plugins/github/webhook/WebhookManager.java b/src/main/java/org/jenkinsci/plugins/github/webhook/WebhookManager.java index b05ed1869..5db84fa3c 100644 --- a/src/main/java/org/jenkinsci/plugins/github/webhook/WebhookManager.java +++ b/src/main/java/org/jenkinsci/plugins/github/webhook/WebhookManager.java @@ -168,18 +168,13 @@ public void unregisterFor(GitHubRepositoryName name, List private GHRepository repoWithWebhookAccess(GitHubRepositoryName name) { FluentIterableWrapper reposAllowedtoManageWebhooks = from(name.resolve(allowedToManageHooks())); if (!reposAllowedtoManageWebhooks.first().isPresent()) { - String msg = String.format("There are no github repos configured to allow webhook management for: %s", - name); - LOGGER.info(msg); - GitHubHookRegisterProblemMonitor.get().registerProblem(name, msg); + LOGGER.debug("There are no github repos configured to allow webhook management for: {}", name); return null; } com.google.common.base.Optional repoWithAdminAccess = reposAllowedtoManageWebhooks .firstMatch(withAdminAccess()); if (!repoWithAdminAccess.isPresent()) { - String msg = String.format("None of the github repos configured have admin access for: %s", name); - LOGGER.info(msg); - GitHubHookRegisterProblemMonitor.get().registerProblem(name, msg); + LOGGER.debug("None of the github repos configured have admin access for: {}", name); return null; } GHRepository repo = repoWithAdminAccess.get(); diff --git a/src/test/java/org/jenkinsci/plugins/github/admin/GitHubHookRegisterProblemMonitorTest.java b/src/test/java/org/jenkinsci/plugins/github/admin/GitHubHookRegisterProblemMonitorTest.java index dc787e896..3430e7738 100644 --- a/src/test/java/org/jenkinsci/plugins/github/admin/GitHubHookRegisterProblemMonitorTest.java +++ b/src/test/java/org/jenkinsci/plugins/github/admin/GitHubHookRegisterProblemMonitorTest.java @@ -6,6 +6,8 @@ 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; @@ -13,13 +15,19 @@ 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,6 +177,8 @@ 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(); @@ -156,13 +186,35 @@ public void shouldReportAboutHookProblemOnRegister() throws IOException { } @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.emptyList()); assertThat("should reg problem", monitor.isProblemWith(REPO), is(true)); } + @Test + public void shouldNotReportAboutHookAuthProblemOnUnregister() { + WebhookManager.forHookUrl(WebhookManagerTest.HOOK_ENDPOINT) + .unregisterFor(REPO, Collections.emptyList()); + + assertThat("should not reg problem", monitor.isProblemWith(REPO), is(false)); + } + @Test public void shouldResolveOnPingHook() { monitor.registerProblem(REPO_FROM_PING_PAYLOAD, new IOException()); From baee633c9379cc441782884c13df31a4fd61cb51 Mon Sep 17 00:00:00 2001 From: Carlos Sanchez Date: Wed, 13 May 2020 12:35:15 +0200 Subject: [PATCH 4/5] Make getCachedClient synchronized to fix spotbugs error --- .../org/jenkinsci/plugins/github/config/GitHubServerConfig.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/org/jenkinsci/plugins/github/config/GitHubServerConfig.java b/src/main/java/org/jenkinsci/plugins/github/config/GitHubServerConfig.java index 7864ab5a1..5a29c5d82 100644 --- a/src/main/java/org/jenkinsci/plugins/github/config/GitHubServerConfig.java +++ b/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 */ - protected GitHub getCachedClient() { + protected synchronized GitHub getCachedClient() { return cachedClient; } From 06cb9220d3a295519d060614f0daf95517a81bea Mon Sep 17 00:00:00 2001 From: Carlos Sanchez Date: Wed, 13 May 2020 19:29:47 +0200 Subject: [PATCH 5/5] [JENKINS-49332] Increased visibility is no longer needed --- .../github/admin/GitHubHookRegisterProblemMonitor.java | 4 ++-- .../github/admin/GitHubHookRegisterProblemMonitorTest.java | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/github/admin/GitHubHookRegisterProblemMonitor.java b/src/main/java/org/jenkinsci/plugins/github/admin/GitHubHookRegisterProblemMonitor.java index 36bc50a92..430951820 100644 --- a/src/main/java/org/jenkinsci/plugins/github/admin/GitHubHookRegisterProblemMonitor.java +++ b/src/main/java/org/jenkinsci/plugins/github/admin/GitHubHookRegisterProblemMonitor.java @@ -86,12 +86,12 @@ public void registerProblem(GitHubRepositoryName repo, Throwable throwable) { } /** - * Registers problems. + * Used by {@link #registerProblem(GitHubRepositoryName, Throwable)} * * @param repo full named GitHub repo, if null nothing will be done * @param message message to show in the interface. Will be used default if blank */ - public void registerProblem(GitHubRepositoryName repo, String message) { + private void registerProblem(GitHubRepositoryName repo, String message) { if (repo == null) { return; } diff --git a/src/test/java/org/jenkinsci/plugins/github/admin/GitHubHookRegisterProblemMonitorTest.java b/src/test/java/org/jenkinsci/plugins/github/admin/GitHubHookRegisterProblemMonitorTest.java index 3430e7738..8a4f3e875 100644 --- a/src/test/java/org/jenkinsci/plugins/github/admin/GitHubHookRegisterProblemMonitorTest.java +++ b/src/test/java/org/jenkinsci/plugins/github/admin/GitHubHookRegisterProblemMonitorTest.java @@ -110,7 +110,7 @@ public void shouldNotAddNullRepo() throws Exception { @Test public void shouldNotAddNullExc() throws Exception { - monitor.registerProblem(REPO, (Throwable) null); + monitor.registerProblem(REPO, null); assertThat("should be no problems", monitor.getProblems().keySet(), empty()); }