Skip to content

Commit 0c663b9

Browse files
KostyaShaKostyaSha
authored
Merge pull request jenkinsci#225 from carlossg/JENKINS-49332
[JENKINS-49332] Better error messages
2 parents a19b6b5 + 06cb922 commit 0c663b9

4 files changed

Lines changed: 82 additions & 14 deletions

File tree

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -227,7 +227,7 @@ public void setClientCacheSize(int clientCacheSize) {
227227
/**
228228
* @return cached GH client or null
229229
*/
230-
private GitHub getCachedClient() {
230+
protected synchronized GitHub getCachedClient() {
231231
return cachedClient;
232232
}
233233

@@ -236,7 +236,7 @@ private GitHub getCachedClient() {
236236
*
237237
* @param cachedClient updated client. Maybe null to invalidate cache
238238
*/
239-
private synchronized void setCachedClient(GitHub cachedClient) {
239+
protected synchronized void setCachedClient(GitHub cachedClient) {
240240
this.cachedClient = cachedClient;
241241
}
242242

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

Lines changed: 25 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
import org.jenkinsci.plugins.github.admin.GitHubHookRegisterProblemMonitor;
1212
import org.jenkinsci.plugins.github.config.HookSecretConfig;
1313
import org.jenkinsci.plugins.github.extension.GHEventsSubscriber;
14+
import org.jenkinsci.plugins.github.util.FluentIterableWrapper;
1415
import org.jenkinsci.plugins.github.util.misc.NullSafeFunction;
1516
import org.jenkinsci.plugins.github.util.misc.NullSafePredicate;
1617
import org.kohsuke.github.GHEvent;
@@ -31,7 +32,6 @@
3132
import java.util.Set;
3233

3334
import static com.cloudbees.jenkins.GitHubRepositoryNameContributor.parseAssociatedNames;
34-
import static com.google.common.base.Preconditions.checkNotNull;
3535
import static com.google.common.base.Predicates.notNull;
3636
import static com.google.common.base.Predicates.or;
3737
import static java.lang.String.format;
@@ -143,10 +143,10 @@ public void run() {
143143
*/
144144
public void unregisterFor(GitHubRepositoryName name, List<GitHubRepositoryName> aliveRepos) {
145145
try {
146-
GHRepository repo = checkNotNull(
147-
from(name.resolve(allowedToManageHooks())).firstMatch(withAdminAccess()).orNull(),
148-
"There are no credentials with admin access to manage hooks on %s", name
149-
);
146+
GHRepository repo = repoWithWebhookAccess(name);
147+
if (repo == null) {
148+
return;
149+
}
150150

151151
LOGGER.debug("Check {} for redundant hooks...", repo);
152152

@@ -165,6 +165,22 @@ public void unregisterFor(GitHubRepositoryName name, List<GitHubRepositoryName>
165165
}
166166
}
167167

168+
private GHRepository repoWithWebhookAccess(GitHubRepositoryName name) {
169+
FluentIterableWrapper<GHRepository> reposAllowedtoManageWebhooks = from(name.resolve(allowedToManageHooks()));
170+
if (!reposAllowedtoManageWebhooks.first().isPresent()) {
171+
LOGGER.debug("There are no github repos configured to allow webhook management for: {}", name);
172+
return null;
173+
}
174+
com.google.common.base.Optional<GHRepository> repoWithAdminAccess = reposAllowedtoManageWebhooks
175+
.firstMatch(withAdminAccess());
176+
if (!repoWithAdminAccess.isPresent()) {
177+
LOGGER.debug("None of the github repos configured have admin access for: {}", name);
178+
return null;
179+
}
180+
GHRepository repo = repoWithAdminAccess.get();
181+
return repo;
182+
}
183+
168184
/**
169185
* Main logic of {@link #registerFor(Item)}.
170186
* Updates hooks with replacing old ones with merged new ones
@@ -178,10 +194,10 @@ protected Function<GitHubRepositoryName, GHHook> createHookSubscribedTo(final Li
178194
@Override
179195
protected GHHook applyNullSafe(@Nonnull GitHubRepositoryName name) {
180196
try {
181-
GHRepository repo = checkNotNull(
182-
from(name.resolve(allowedToManageHooks())).firstMatch(withAdminAccess()).orNull(),
183-
"There are no credentials with admin access to manage hooks on %s", name
184-
);
197+
GHRepository repo = repoWithWebhookAccess(name);
198+
if (repo == null) {
199+
return null;
200+
}
185201

186202
Validate.notEmpty(events, "Events list for hook can't be empty");
187203

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

Lines changed: 54 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,20 +6,28 @@
66
import hudson.model.Item;
77
import hudson.plugins.git.GitSCM;
88
import org.jenkinsci.plugins.github.extension.GHSubscriberEvent;
9+
import org.jenkinsci.plugins.github.GitHubPlugin;
10+
import org.jenkinsci.plugins.github.config.GitHubServerConfig;
911
import org.jenkinsci.plugins.github.extension.GHEventsSubscriber;
1012
import org.jenkinsci.plugins.github.webhook.WebhookManager;
1113
import org.jenkinsci.plugins.github.webhook.WebhookManagerTest;
1214
import org.jenkinsci.plugins.github.webhook.subscriber.PingGHEventSubscriber;
1315
import org.junit.Before;
1416
import org.junit.Rule;
1517
import org.junit.Test;
18+
import org.junit.runner.RunWith;
1619
import org.jvnet.hudson.test.Issue;
1720
import org.jvnet.hudson.test.JenkinsRule;
1821
import org.jvnet.hudson.test.recipes.LocalData;
1922
import org.kohsuke.github.GHEvent;
23+
import org.kohsuke.github.GHRepository;
24+
import org.kohsuke.github.GitHub;
25+
import org.mockito.Mock;
26+
import org.mockito.runners.MockitoJUnitRunner;
2027

2128
import javax.inject.Inject;
2229
import java.io.IOException;
30+
import java.util.Arrays;
2331
import java.util.Collections;
2432

2533
import static com.cloudbees.jenkins.GitHubRepositoryName.create;
@@ -32,14 +40,17 @@
3240
import static org.hamcrest.Matchers.notNullValue;
3341
import static org.hamcrest.Matchers.nullValue;
3442
import static org.junit.Assert.assertThat;
43+
import static org.mockito.Mockito.when;
3544

3645
/**
3746
* @author lanwen (Merkushev Kirill)
3847
*/
3948
@Issue("JENKINS-24690")
49+
@RunWith(MockitoJUnitRunner.class)
4050
public class GitHubHookRegisterProblemMonitorTest {
4151
private static final GitHubRepositoryName REPO = new GitHubRepositoryName("host", "user", "repo");
42-
private static final GitSCM REPO_GIT_SCM = new GitSCM("git://host/user/repo.git");
52+
private static final String REPO_GIT_URI = "host/user/repo.git";
53+
private static final GitSCM REPO_GIT_SCM = new GitSCM("git://"+REPO_GIT_URI);
4354

4455
private static final GitHubRepositoryName REPO_FROM_PING_PAYLOAD = create("https://github.com/lanwen/test");
4556

@@ -55,9 +66,26 @@ public class GitHubHookRegisterProblemMonitorTest {
5566
@Rule
5667
public JenkinsRule jRule = new JenkinsRule();
5768

69+
@Mock
70+
private GitHub github;
71+
@Mock
72+
private GHRepository ghRepository;
73+
74+
class GitHubServerConfigForTest extends GitHubServerConfig {
75+
public GitHubServerConfigForTest(String credentialsId) {
76+
super(credentialsId);
77+
this.setCachedClient(github);
78+
}
79+
}
80+
5881
@Before
5982
public void setUp() throws Exception {
6083
jRule.getInstance().getInjector().injectMembers(this);
84+
GitHubServerConfig config = new GitHubServerConfigForTest("");
85+
config.setApiUrl("http://" + REPO_GIT_URI);
86+
GitHubPlugin.configuration().setConfigs(Arrays.asList(config));
87+
when(github.getRepository("user/repo")).thenReturn(ghRepository);
88+
when(ghRepository.hasAdminAccess()).thenReturn(true);
6189
}
6290

6391
@Test
@@ -149,20 +177,44 @@ public void shouldReportAboutHookProblemOnRegister() throws IOException {
149177
job.addTrigger(new GitHubPushTrigger());
150178
job.setScm(REPO_GIT_SCM);
151179

180+
when(github.getRepository("user/repo"))
181+
.thenThrow(new RuntimeException("shouldReportAboutHookProblemOnRegister"));
152182
WebhookManager.forHookUrl(WebhookManagerTest.HOOK_ENDPOINT)
153183
.registerFor((Item) job).run();
154184

155185
assertThat("should reg problem", monitor.isProblemWith(REPO), is(true));
156186
}
157187

158188
@Test
159-
public void shouldReportAboutHookProblemOnUnregister() {
189+
public void shouldNotReportAboutHookProblemOnRegister() throws IOException {
190+
FreeStyleProject job = jRule.createFreeStyleProject();
191+
job.addTrigger(new GitHubPushTrigger());
192+
job.setScm(REPO_GIT_SCM);
193+
194+
WebhookManager.forHookUrl(WebhookManagerTest.HOOK_ENDPOINT)
195+
.registerFor((Item) job).run();
196+
197+
assertThat("should reg problem", monitor.isProblemWith(REPO), is(false));
198+
}
199+
200+
@Test
201+
public void shouldReportAboutHookProblemOnUnregister() throws IOException {
202+
when(github.getRepository("user/repo"))
203+
.thenThrow(new RuntimeException("shouldReportAboutHookProblemOnUnregister"));
160204
WebhookManager.forHookUrl(WebhookManagerTest.HOOK_ENDPOINT)
161205
.unregisterFor(REPO, Collections.<GitHubRepositoryName>emptyList());
162206

163207
assertThat("should reg problem", monitor.isProblemWith(REPO), is(true));
164208
}
165209

210+
@Test
211+
public void shouldNotReportAboutHookAuthProblemOnUnregister() {
212+
WebhookManager.forHookUrl(WebhookManagerTest.HOOK_ENDPOINT)
213+
.unregisterFor(REPO, Collections.<GitHubRepositoryName>emptyList());
214+
215+
assertThat("should not reg problem", monitor.isProblemWith(REPO), is(false));
216+
}
217+
166218
@Test
167219
public void shouldResolveOnPingHook() {
168220
monitor.registerProblem(REPO_FROM_PING_PAYLOAD, new IOException());

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -86,7 +86,7 @@ public class WebhookManagerTest {
8686
@Test
8787
public void shouldDoNothingOnNoAdminRights() throws Exception {
8888
manager.unregisterFor(nonactive, newArrayList(active));
89-
verify(manager, times(1)).withAdminAccess();
89+
verify(manager, never()).withAdminAccess();
9090
verify(manager, never()).fetchHooks();
9191
}
9292

0 commit comments

Comments
 (0)