From c9cf37d3247a1dfa44171a733d965eb6128de25e Mon Sep 17 00:00:00 2001 From: Stephen Connolly Date: Mon, 7 Nov 2016 12:35:08 +0000 Subject: [PATCH 1/4] [FIXED JENKINS-39533] Make GitHub plugin BuildableItem aware --- .../java/com/cloudbees/jenkins/Cleaner.java | 13 +-- .../cloudbees/jenkins/GitHubPushTrigger.java | 4 +- .../GitHubRepositoryNameContributor.java | 62 ++++++++++----- .../com/cloudbees/jenkins/GitHubTrigger.java | 8 +- .../com/cloudbees/jenkins/GitHubWebHook.java | 29 +++++-- .../github/config/GitHubPluginConfig.java | 8 +- .../github/extension/GHEventsSubscriber.java | 79 ++++++++++++++++++- .../github/util/FluentIterableWrapper.java | 9 +++ .../plugins/github/util/JobInfoHelpers.java | 59 +++++++++----- .../github/webhook/WebhookManager.java | 32 ++++++-- .../DefaultPushGHEventSubscriber.java | 6 +- .../subscriber/PingGHEventSubscriber.java | 6 +- .../cloudbees/jenkins/GitHubWebHookTest.java | 3 +- .../GitHubHookRegisterProblemMonitorTest.java | 3 +- .../extension/GHEventsSubscriberTest.java | 3 +- .../plugins/github/test/GHMockRule.java | 3 +- .../github/util/JobInfoHelpersTest.java | 7 +- .../github/webhook/WebhookManagerTest.java | 5 +- 18 files changed, 252 insertions(+), 87 deletions(-) diff --git a/src/main/java/com/cloudbees/jenkins/Cleaner.java b/src/main/java/com/cloudbees/jenkins/Cleaner.java index 7544ca6e2..32930ba74 100644 --- a/src/main/java/com/cloudbees/jenkins/Cleaner.java +++ b/src/main/java/com/cloudbees/jenkins/Cleaner.java @@ -1,7 +1,7 @@ package com.cloudbees.jenkins; import hudson.Extension; -import hudson.model.Job; +import hudson.model.Item; import hudson.model.PeriodicWork; import jenkins.model.Jenkins; import org.jenkinsci.plugins.github.GitHubPlugin; @@ -28,7 +28,7 @@ public class Cleaner extends PeriodicWork { /** * Queue contains repo names prepared to cleanup. - * After configure method on job, trigger calls {@link #onStop(Job)} + * After configure method on job, trigger calls {@link #onStop(Item)} * which converts to repo names with help of contributors. * * This queue is thread-safe, so any thread can write or @@ -38,9 +38,10 @@ public class Cleaner extends PeriodicWork { /** * Called when a {@link GitHubPushTrigger} is about to be removed. + * @deprecated use {@link #onStop(Item)} */ - /* package */ void onStop(Job job) { - cleanQueue.addAll(GitHubRepositoryNameContributor.parseAssociatedNames(job)); + /* package */ void onStop(Item item) { + cleanQueue.addAll(GitHubRepositoryNameContributor.parseAssociatedNames(item)); } @Override @@ -61,8 +62,8 @@ protected void doRun() throws Exception { URL url = GitHubPlugin.configuration().getHookUrl(); - List jobs = Jenkins.getInstance().getAllItems(Job.class); - List aliveRepos = from(jobs) + List items = Jenkins.getInstance().getAllItems(Item.class); + List aliveRepos = from(items) .filter(isAlive()) // live repos .transformAndConcat(associatedNames()).toList(); diff --git a/src/main/java/com/cloudbees/jenkins/GitHubPushTrigger.java b/src/main/java/com/cloudbees/jenkins/GitHubPushTrigger.java index c386afe0f..7331a7c54 100644 --- a/src/main/java/com/cloudbees/jenkins/GitHubPushTrigger.java +++ b/src/main/java/com/cloudbees/jenkins/GitHubPushTrigger.java @@ -377,8 +377,8 @@ private static ThreadFactory threadFactory() { * @since TODO */ @SuppressWarnings("unused") - public FormValidation doCheckHookRegistered(@AncestorInPath Job job) { - Preconditions.checkNotNull(job, "Job can't be null if wants to check hook in monitor"); + public FormValidation doCheckHookRegistered(@AncestorInPath Item job) { + Preconditions.checkNotNull(job, "Item can't be null if wants to check hook in monitor"); Collection repos = GitHubRepositoryNameContributor.parseAssociatedNames(job); diff --git a/src/main/java/com/cloudbees/jenkins/GitHubRepositoryNameContributor.java b/src/main/java/com/cloudbees/jenkins/GitHubRepositoryNameContributor.java index 948072527..d0c0bd2de 100644 --- a/src/main/java/com/cloudbees/jenkins/GitHubRepositoryNameContributor.java +++ b/src/main/java/com/cloudbees/jenkins/GitHubRepositoryNameContributor.java @@ -7,6 +7,7 @@ import hudson.Util; import hudson.model.AbstractProject; import hudson.model.EnvironmentContributor; +import hudson.model.Item; import hudson.model.Job; import hudson.model.TaskListener; import hudson.plugins.git.GitSCM; @@ -36,41 +37,54 @@ public abstract class GitHubRepositoryNameContributor implements ExtensionPoint * Looks at the definition of {@link AbstractProject} and list up the related github repositories, * then puts them into the collection. * - * @deprecated Use {@link #parseAssociatedNames(Job, Collection)} + * @deprecated Use {@link #parseAssociatedNames(Item, Collection)} */ @Deprecated public void parseAssociatedNames(AbstractProject job, Collection result) { - parseAssociatedNames((Job) job, result); + parseAssociatedNames((Item) job, result); } /** * Looks at the definition of {@link Job} and list up the related github repositories, * then puts them into the collection. + * @deprecated Use {@link #parseAssociatedNames(Item, Collection)} */ + @Deprecated public /*abstract*/ void parseAssociatedNames(Job job, Collection result) { - if (overriddenMethodHasDeprecatedSignature(job)) { - parseAssociatedNames((AbstractProject) job, result); - } else { - throw new AbstractMethodError("you must override the new overload of parseAssociatedNames"); - } + parseAssociatedNames((Item) job, result); } /** - * To select backward compatible method with old extensions - * with overridden {@link #parseAssociatedNames(AbstractProject, Collection)} - * - * @param job - parameter to check for old class - * - * @return true if overridden deprecated method + * Looks at the definition of {@link Item} and list up the related github repositories, + * then puts them into the collection. */ - private boolean overriddenMethodHasDeprecatedSignature(Job job) { - return Util.isOverridden( + @SuppressWarnings("deprecation") + public /*abstract*/ void parseAssociatedNames(Item item, Collection result) { + if (Util.isOverridden( + GitHubRepositoryNameContributor.class, + getClass(), + "parseAssociatedNames", + Job.class, + Collection.class + )) { + // if this impl is legacy, it cannot contribute to non-jobs, so not an error + if (item instanceof Job) { + parseAssociatedNames((Job) item, result); + } + } else if (Util.isOverridden( GitHubRepositoryNameContributor.class, getClass(), "parseAssociatedNames", AbstractProject.class, Collection.class - ) && job instanceof AbstractProject; + )) { + // if this impl is legacy, it cannot contribute to non-projects, so not an error + if (item instanceof AbstractProject) { + parseAssociatedNames((AbstractProject) item, result); + } + } else { + throw new AbstractMethodError("you must override the new overload of parseAssociatedNames"); + } } public static ExtensionList all() { @@ -82,13 +96,21 @@ public static ExtensionList all() { */ @Deprecated public static Collection parseAssociatedNames(AbstractProject job) { - return parseAssociatedNames((Job) job); + return parseAssociatedNames((Item) job); } + /** + * @deprecated Use {@link #parseAssociatedNames(Item)} + */ + @Deprecated public static Collection parseAssociatedNames(Job job) { + return parseAssociatedNames((Item) job); + } + + public static Collection parseAssociatedNames(Item item) { Set names = new HashSet(); for (GitHubRepositoryNameContributor c : all()) { - c.parseAssociatedNames(job, names); + c.parseAssociatedNames(item, names); } return names; } @@ -99,9 +121,9 @@ public static Collection parseAssociatedNames(Job jo @Extension public static class FromSCM extends GitHubRepositoryNameContributor { @Override - public void parseAssociatedNames(Job job, Collection result) { + public void parseAssociatedNames(Item job, Collection result) { SCMTriggerItem item = SCMTriggerItems.asSCMTriggerItem(job); - EnvVars envVars = buildEnv(job); + EnvVars envVars = job instanceof Job ? buildEnv((Job) job) : new EnvVars(); if (item != null) { for (SCM scm : item.getSCMs()) { addRepositories(scm, envVars, result); diff --git a/src/main/java/com/cloudbees/jenkins/GitHubTrigger.java b/src/main/java/com/cloudbees/jenkins/GitHubTrigger.java index 1908b934d..bfb5e72e0 100644 --- a/src/main/java/com/cloudbees/jenkins/GitHubTrigger.java +++ b/src/main/java/com/cloudbees/jenkins/GitHubTrigger.java @@ -3,7 +3,7 @@ import hudson.Extension; import hudson.Util; import hudson.model.AbstractProject; -import hudson.model.Job; +import hudson.model.Item; import hudson.triggers.Trigger; import jenkins.model.ParameterizedJobMixIn; @@ -46,9 +46,9 @@ public interface GitHubTrigger { @Extension class GitHubRepositoryNameContributorImpl extends GitHubRepositoryNameContributor { @Override - public void parseAssociatedNames(Job job, Collection result) { - if (job instanceof ParameterizedJobMixIn.ParameterizedJob) { - ParameterizedJobMixIn.ParameterizedJob p = (ParameterizedJobMixIn.ParameterizedJob) job; + public void parseAssociatedNames(Item item, Collection result) { + if (item instanceof ParameterizedJobMixIn.ParameterizedJob) { + ParameterizedJobMixIn.ParameterizedJob p = (ParameterizedJobMixIn.ParameterizedJob) item; // TODO use standard method in 1.621+ for (GitHubTrigger ght : Util.filter(p.getTriggers().values(), GitHubTrigger.class)) { result.addAll(ght.getGitHubRepositories()); diff --git a/src/main/java/com/cloudbees/jenkins/GitHubWebHook.java b/src/main/java/com/cloudbees/jenkins/GitHubWebHook.java index dd494795c..16436a08d 100644 --- a/src/main/java/com/cloudbees/jenkins/GitHubWebHook.java +++ b/src/main/java/com/cloudbees/jenkins/GitHubWebHook.java @@ -3,6 +3,7 @@ import com.google.common.base.Function; import hudson.Extension; import hudson.ExtensionPoint; +import hudson.model.Item; import hudson.model.Job; import hudson.model.RootAction; import hudson.model.UnprotectedRootAction; @@ -70,21 +71,35 @@ public String getUrlName() { * {@code GitHubWebHook.get().registerHookFor(job);} * * @param job not null project to register hook for + * @deprecated use {@link #registerHookFor(Item)} */ + @Deprecated public void registerHookFor(Job job) { reRegisterHookForJob().apply(job); } + /** + * If any wants to auto-register hook, then should call this method + * Example code: + * {@code GitHubWebHook.get().registerHookFor(item);} + * + * @param item not null item to register hook for + */ + public void registerHookFor(Item item) { + reRegisterHookForJob().apply(item); + } + /** * Calls {@link #registerHookFor(Job)} for every project which have subscriber * * @return list of jobs which jenkins tried to register hook */ - public List reRegisterAllHooks() { - return from(getJenkinsInstance().getAllItems(Job.class)) + public List reRegisterAllHooks() { + return from(getJenkinsInstance().getAllItems(Item.class)) .filter(isBuildable()) .filter(isAlive()) - .transform(reRegisterHookForJob()).toList(); + .transform(reRegisterHookForJob()) + .toList(); } /** @@ -101,11 +116,11 @@ public void doIndex(@Nonnull @GHEventHeader GHEvent event, @Nonnull @GHEventPayl .transform(processEvent(event, payload)).toList(); } - private Function reRegisterHookForJob() { - return new Function() { + private Function reRegisterHookForJob() { + return new Function() { @Override - public Job apply(Job job) { - LOGGER.debug("Calling registerHooks() for {}", notNull(job, "Job can't be null").getFullName()); + public T apply(T job) { + LOGGER.debug("Calling registerHooks() for {}", notNull(job, "Item can't be null").getFullName()); // We should handle wrong url of self defined hook url here in any case with try-catch :( URL hookUrl; diff --git a/src/main/java/org/jenkinsci/plugins/github/config/GitHubPluginConfig.java b/src/main/java/org/jenkinsci/plugins/github/config/GitHubPluginConfig.java index 5f2392679..8e78d8f14 100644 --- a/src/main/java/org/jenkinsci/plugins/github/config/GitHubPluginConfig.java +++ b/src/main/java/org/jenkinsci/plugins/github/config/GitHubPluginConfig.java @@ -6,7 +6,7 @@ import hudson.Extension; import hudson.XmlFile; import hudson.model.Descriptor; -import hudson.model.Job; +import hudson.model.Item; import hudson.util.FormValidation; import jenkins.model.GlobalConfiguration; import jenkins.model.Jenkins; @@ -181,10 +181,10 @@ public FormValidation doReRegister() { return FormValidation.warning("Works only when Jenkins manages hooks (one ore more creds specified)"); } - List registered = GitHubWebHook.get().reRegisterAllHooks(); + List registered = GitHubWebHook.get().reRegisterAllHooks(); - LOGGER.info("Called registerHooks() for {} jobs", registered.size()); - return FormValidation.ok("Called re-register hooks for %s jobs", registered.size()); + LOGGER.info("Called registerHooks() for {} items", registered.size()); + return FormValidation.ok("Called re-register hooks for %s items", registered.size()); } @SuppressWarnings("unused") diff --git a/src/main/java/org/jenkinsci/plugins/github/extension/GHEventsSubscriber.java b/src/main/java/org/jenkinsci/plugins/github/extension/GHEventsSubscriber.java index bdef0e98c..def37fb5b 100644 --- a/src/main/java/org/jenkinsci/plugins/github/extension/GHEventsSubscriber.java +++ b/src/main/java/org/jenkinsci/plugins/github/extension/GHEventsSubscriber.java @@ -4,7 +4,10 @@ import com.google.common.base.Predicate; import hudson.ExtensionList; import hudson.ExtensionPoint; +import hudson.model.Item; import hudson.model.Job; +import java.lang.reflect.Method; +import java.lang.reflect.Modifier; import jenkins.model.Jenkins; import org.jenkinsci.plugins.github.util.misc.NullSafeFunction; import org.jenkinsci.plugins.github.util.misc.NullSafePredicate; @@ -32,6 +35,7 @@ */ public abstract class GHEventsSubscriber implements ExtensionPoint { private static final Logger LOGGER = LoggerFactory.getLogger(GHEventsSubscriber.class); + private transient Boolean hasIsApplicableItem; /** * Should return true only if this subscriber interested in {@link #events()} set for this project @@ -39,9 +43,63 @@ public abstract class GHEventsSubscriber implements ExtensionPoint { * * @param project to check * - * @return true to provide events to register and subscribe for this project + * @return {@code true} to provide events to register and subscribe for this project + * @deprecated override {@link #isApplicable(Item)} instead. */ - protected abstract boolean isApplicable(@Nullable Job project); + @Deprecated + protected boolean isApplicable(@Nullable Job project) { + if (checkIsApplicableItem()) { + return isApplicable((Item) project); + } + // a legacy implementation which should not have been calling super.isApplicable(Job) + throw new AbstractMethodError("you must override the new overload of isApplicable"); + } + + /** + * Should return true only if this subscriber interested in {@link #events()} set for this project + * Don't call it directly, use {@link #isApplicableFor} static function + * + * @param item to check + * + * @return {@code true} to provide events to register and subscribe for this item + * @since FIXME + */ + protected abstract boolean isApplicable(@Nullable Item item); + + /** + * Call {@link #isApplicable(Item)} with safety for calling to legacy implementations before the abstract method + * was switched from {@link #isApplicable(Job)}. + * @param item to check. + * @return {@code true} to provide events to register and subscribe for this item + */ + @SuppressWarnings("deprecation") + private boolean safeIsApplicable(@Nullable Item item) { + return checkIsApplicableItem() ? isApplicable(item) : item instanceof Job && isApplicable((Job) item); + } + + private boolean checkIsApplicableItem() { + if (hasIsApplicableItem == null) { + boolean implemented = false; + // cannot use Util.isOverridden because method is protected and isOverridden only checks public methods + Class clazz = getClass(); + while (clazz != null && clazz != GHEventsSubscriber.class) { + try { + Method isApplicable = clazz.getDeclaredMethod("isApplicable", Item.class); + if (isApplicable.getDeclaringClass() != GHEventsSubscriber.class) { + // ok this is the first method we have found that could be an override + // if somebody overrode an inherited method with and `abstract` then we don't have the method + implemented = !Modifier.isAbstract(isApplicable.getModifiers()); + break; + } + } catch (NoSuchMethodException e) { + clazz = clazz.getSuperclass(); + } + } + // idempotent so no need for synchronization + this.hasIsApplicableItem = implemented; + } + return hasIsApplicableItem; + } /** * Should be not null. Should return only events which this extension can parse in {@link #onEvent(GHEvent, String)} @@ -92,12 +150,27 @@ protected Set applyNullSafe(@Nonnull GHEventsSubscriber subscriber) { * * @return predicate to use in iterable filtering * @see #isApplicable + * @deprecated use {@link #isApplicableFor(Item)}. */ + @Deprecated public static Predicate isApplicableFor(final Job project) { + return isApplicableFor((Item) project); + } + + /** + * Helps to filter only GHEventsSubscribers that can return TRUE on given item + * + * @param item to check every GHEventsSubscriber for being applicable + * + * @return predicate to use in iterable filtering + * @see #isApplicable + * @since FIXME + */ + public static Predicate isApplicableFor(final Item item) { return new NullSafePredicate() { @Override protected boolean applyNullSafe(@Nonnull GHEventsSubscriber subscriber) { - return subscriber.isApplicable(project); + return subscriber.safeIsApplicable(item); } }; } diff --git a/src/main/java/org/jenkinsci/plugins/github/util/FluentIterableWrapper.java b/src/main/java/org/jenkinsci/plugins/github/util/FluentIterableWrapper.java index 8a83f00e7..76430a82c 100644 --- a/src/main/java/org/jenkinsci/plugins/github/util/FluentIterableWrapper.java +++ b/src/main/java/org/jenkinsci/plugins/github/util/FluentIterableWrapper.java @@ -79,6 +79,15 @@ public final FluentIterableWrapper filter(Predicate predicate) { return from(Iterables.filter(iterable, predicate)); } + /** + * Returns the elements from this fluent iterable that are instances of the supplied type. The + * resulting fluent iterable's iterator does not support {@code remove()}. + */ + @CheckReturnValue + public final FluentIterableWrapper filter(Class clazz) { + return from(Iterables.filter(iterable, clazz)); + } + /** * Returns a fluent iterable that applies {@code function} to each element of this * fluent iterable. diff --git a/src/main/java/org/jenkinsci/plugins/github/util/JobInfoHelpers.java b/src/main/java/org/jenkinsci/plugins/github/util/JobInfoHelpers.java index 1ca60cd97..7d145ba59 100644 --- a/src/main/java/org/jenkinsci/plugins/github/util/JobInfoHelpers.java +++ b/src/main/java/org/jenkinsci/plugins/github/util/JobInfoHelpers.java @@ -5,6 +5,8 @@ import com.google.common.base.Function; import com.google.common.base.Predicate; import hudson.model.AbstractProject; +import hudson.model.BuildableItem; +import hudson.model.Item; import hudson.model.Job; import hudson.triggers.Trigger; import jenkins.model.ParameterizedJobMixIn; @@ -33,10 +35,10 @@ private JobInfoHelpers() { * * @return predicate with true on apply if job contains trigger of given class */ - public static Predicate withTrigger(final Class clazz) { - return new Predicate() { - public boolean apply(Job job) { - return triggerFrom(job, clazz) != null; + public static Predicate withTrigger(final Class clazz) { + return new Predicate() { + public boolean apply(Item item) { + return triggerFrom(item, clazz) != null; } }; } @@ -44,12 +46,12 @@ public boolean apply(Job job) { /** * Can be useful to ignore disabled jobs on reregistering hooks * - * @return predicate with true on apply if job is buildable + * @return predicate with true on apply if item is buildable */ - public static Predicate isBuildable() { - return new Predicate() { - public boolean apply(Job job) { - return job != null && job.isBuildable(); + public static Predicate isBuildable() { + return new Predicate() { + public boolean apply(ITEM item) { + return item instanceof Job ? ((Job) item).isBuildable() : item instanceof BuildableItem; } }; } @@ -57,25 +59,25 @@ public boolean apply(Job job) { /** * @return function which helps to convert job to repo names associated with this job */ - public static Function> associatedNames() { - return new Function>() { - public Collection apply(Job job) { - return GitHubRepositoryNameContributor.parseAssociatedNames(job); + public static Function> associatedNames() { + return new Function>() { + public Collection apply(ITEM item) { + return GitHubRepositoryNameContributor.parseAssociatedNames(item); } }; } /** - * If any of event subscriber interested in hook for job, then return true + * If any of event subscriber interested in hook for item, then return true * By default, push hook subscriber is interested in job with gh-push-trigger * - * @return predicate with true if job alive and should have hook + * @return predicate with true if item alive and should have hook */ - public static Predicate isAlive() { - return new Predicate() { + public static Predicate isAlive() { + return new Predicate() { @Override - public boolean apply(Job job) { - return !from(GHEventsSubscriber.all()).filter(isApplicableFor(job)).toList().isEmpty(); + public boolean apply(ITEM item) { + return !from(GHEventsSubscriber.all()).filter(isApplicableFor(item)).toList().isEmpty(); } }; } @@ -87,11 +89,26 @@ public boolean apply(Job job) { * * @return Trigger instance with required class or null * TODO use standard method in 1.621+ + * @deprecated use {@link #triggerFrom(Item, Class)} */ + @Deprecated @CheckForNull public static T triggerFrom(Job job, Class tClass) { - if (job instanceof ParameterizedJobMixIn.ParameterizedJob) { - ParameterizedJobMixIn.ParameterizedJob pJob = (ParameterizedJobMixIn.ParameterizedJob) job; + return triggerFrom((Item) job, tClass); + } + + /** + * @param item job to search trigger in + * @param tClass trigger with class which we want to receive from job + * @param type of trigger + * + * @return Trigger instance with required class or null + * TODO use standard method in 1.621+ + */ + @CheckForNull + public static T triggerFrom(Item item, Class tClass) { + if (item instanceof ParameterizedJobMixIn.ParameterizedJob) { + ParameterizedJobMixIn.ParameterizedJob pJob = (ParameterizedJobMixIn.ParameterizedJob) item; for (Trigger candidate : pJob.getTriggers().values()) { if (tClass.isInstance(candidate)) { 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 21f4293d7..68ae5a793 100644 --- a/src/main/java/org/jenkinsci/plugins/github/webhook/WebhookManager.java +++ b/src/main/java/org/jenkinsci/plugins/github/webhook/WebhookManager.java @@ -3,6 +3,7 @@ import com.cloudbees.jenkins.GitHubRepositoryName; import com.google.common.base.Function; import com.google.common.base.Predicate; +import hudson.model.Item; import hudson.model.Job; import hudson.util.Secret; import org.apache.commons.lang.Validate; @@ -79,24 +80,45 @@ public static WebhookManager forHookUrl(URL endpoint) { * * @return runnable to create hooks on run * @see #createHookSubscribedTo(List) + * @deprecated use {@link #registerFor(Item)} */ + @Deprecated public Runnable registerFor(final Job project) { - final Collection names = parseAssociatedNames(project); + return registerFor((Item) project); + } + + /** + * Creates runnable with ability to create hooks for given project + * For each GH repo name contributed by {@link com.cloudbees.jenkins.GitHubRepositoryNameContributor}, + * this runnable creates hook (with clean old one). + * + * Hook events job interested in, contributes to full set instances of {@link GHEventsSubscriber}. + * New events will be merged with old ones from existent hook. + * + * By default only push event is registered + * + * @param item to find for which repos we should create hooks + * + * @return runnable to create hooks on run + * @see #createHookSubscribedTo(List) + */ + public Runnable registerFor(final Item item) { + final Collection names = parseAssociatedNames(item); final List events = from(GHEventsSubscriber.all()) - .filter(isApplicableFor(project)) + .filter(isApplicableFor(item)) .transformAndConcat(extractEvents()).toList(); return new Runnable() { public void run() { if (events.isEmpty()) { LOGGER.debug("No any subscriber interested in {}, but hooks creation launched, skipping...", - project.getFullName()); + item.getFullName()); return; } LOGGER.info("GitHub webhooks activated for job {} with {} (events: {})", - project.getFullName(), names, events); + item.getFullName(), names, events); from(names) .transform(createHookSubscribedTo(events)) @@ -141,7 +163,7 @@ public void unregisterFor(GitHubRepositoryName name, List } /** - * Main logic of {@link #registerFor(Job)}. + * Main logic of {@link #registerFor(Item)}. * Updates hooks with replacing old ones with merged new ones * * @param events calculated events list to be registered in hook diff --git a/src/main/java/org/jenkinsci/plugins/github/webhook/subscriber/DefaultPushGHEventSubscriber.java b/src/main/java/org/jenkinsci/plugins/github/webhook/subscriber/DefaultPushGHEventSubscriber.java index bee94ab34..10499f815 100644 --- a/src/main/java/org/jenkinsci/plugins/github/webhook/subscriber/DefaultPushGHEventSubscriber.java +++ b/src/main/java/org/jenkinsci/plugins/github/webhook/subscriber/DefaultPushGHEventSubscriber.java @@ -6,7 +6,7 @@ import com.cloudbees.jenkins.GitHubTrigger; import com.cloudbees.jenkins.GitHubWebHook; import hudson.Extension; -import hudson.model.Job; +import hudson.model.Item; import hudson.security.ACL; import jenkins.model.Jenkins; import net.sf.json.JSONObject; @@ -41,7 +41,7 @@ public class DefaultPushGHEventSubscriber extends GHEventsSubscriber { * @return true if project has {@link GitHubPushTrigger} */ @Override - protected boolean isApplicable(Job project) { + protected boolean isApplicable(Item project) { return withTrigger(GitHubPushTrigger.class).apply(project); } @@ -75,7 +75,7 @@ protected void onEvent(GHEvent event, String payload) { ACL.impersonate(ACL.SYSTEM, new Runnable() { @Override public void run() { - for (Job job : Jenkins.getInstance().getAllItems(Job.class)) { + for (Item job : Jenkins.getInstance().getAllItems(Item.class)) { GitHubTrigger trigger = triggerFrom(job, GitHubPushTrigger.class); if (trigger != null) { LOGGER.debug("Considering to poke {}", job.getFullDisplayName()); diff --git a/src/main/java/org/jenkinsci/plugins/github/webhook/subscriber/PingGHEventSubscriber.java b/src/main/java/org/jenkinsci/plugins/github/webhook/subscriber/PingGHEventSubscriber.java index a5a0007bd..1c9487e66 100644 --- a/src/main/java/org/jenkinsci/plugins/github/webhook/subscriber/PingGHEventSubscriber.java +++ b/src/main/java/org/jenkinsci/plugins/github/webhook/subscriber/PingGHEventSubscriber.java @@ -2,7 +2,7 @@ import com.cloudbees.jenkins.GitHubRepositoryName; import hudson.Extension; -import hudson.model.Job; +import hudson.model.Item; import net.sf.json.JSONObject; import org.jenkinsci.plugins.github.admin.GitHubHookRegisterProblemMonitor; import org.jenkinsci.plugins.github.extension.GHEventsSubscriber; @@ -32,14 +32,14 @@ public class PingGHEventSubscriber extends GHEventsSubscriber { private transient GitHubHookRegisterProblemMonitor monitor; /** - * This subscriber is not applicable to any job + * This subscriber is not applicable to any item * * @param project ignored * * @return always false */ @Override - protected boolean isApplicable(Job project) { + protected boolean isApplicable(Item project) { return false; } diff --git a/src/test/java/com/cloudbees/jenkins/GitHubWebHookTest.java b/src/test/java/com/cloudbees/jenkins/GitHubWebHookTest.java index bba4ff7b4..0f1c367e9 100644 --- a/src/test/java/com/cloudbees/jenkins/GitHubWebHookTest.java +++ b/src/test/java/com/cloudbees/jenkins/GitHubWebHookTest.java @@ -2,6 +2,7 @@ import com.google.inject.Inject; +import hudson.model.Item; import hudson.model.Job; import org.jenkinsci.plugins.github.extension.GHEventsSubscriber; @@ -110,7 +111,7 @@ public TestSubscriber(GHEvent interested) { } @Override - protected boolean isApplicable(Job project) { + protected boolean isApplicable(Item project) { return true; } 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 238ef9389..a0d761de2 100644 --- a/src/test/java/org/jenkinsci/plugins/github/admin/GitHubHookRegisterProblemMonitorTest.java +++ b/src/test/java/org/jenkinsci/plugins/github/admin/GitHubHookRegisterProblemMonitorTest.java @@ -3,6 +3,7 @@ import com.cloudbees.jenkins.GitHubPushTrigger; import com.cloudbees.jenkins.GitHubRepositoryName; import hudson.model.FreeStyleProject; +import hudson.model.Item; import hudson.plugins.git.GitSCM; import org.jenkinsci.plugins.github.extension.GHEventsSubscriber; import org.jenkinsci.plugins.github.webhook.WebhookManager; @@ -148,7 +149,7 @@ public void shouldReportAboutHookProblemOnRegister() throws IOException { job.setScm(REPO_GIT_SCM); WebhookManager.forHookUrl(WebhookManagerTest.HOOK_ENDPOINT) - .registerFor(job).run(); + .registerFor((Item) job).run(); assertThat("should reg problem", monitor.isProblemWith(REPO), is(true)); } diff --git a/src/test/java/org/jenkinsci/plugins/github/extension/GHEventsSubscriberTest.java b/src/test/java/org/jenkinsci/plugins/github/extension/GHEventsSubscriberTest.java index 2ab02c55f..0f0187f2c 100644 --- a/src/test/java/org/jenkinsci/plugins/github/extension/GHEventsSubscriberTest.java +++ b/src/test/java/org/jenkinsci/plugins/github/extension/GHEventsSubscriberTest.java @@ -1,5 +1,6 @@ package org.jenkinsci.plugins.github.extension; +import hudson.model.Item; import hudson.model.Job; import org.junit.Test; @@ -30,7 +31,7 @@ public void shouldMatchAgainstEmptySetInsteadOfNull() throws Exception { public static class NullSubscriber extends GHEventsSubscriber { @Override - protected boolean isApplicable(Job project) { + protected boolean isApplicable(Item project) { return true; } diff --git a/src/test/java/org/jenkinsci/plugins/github/test/GHMockRule.java b/src/test/java/org/jenkinsci/plugins/github/test/GHMockRule.java index d1a0f8426..d0c2709e5 100644 --- a/src/test/java/org/jenkinsci/plugins/github/test/GHMockRule.java +++ b/src/test/java/org/jenkinsci/plugins/github/test/GHMockRule.java @@ -3,6 +3,7 @@ import com.cloudbees.jenkins.GitHubRepositoryName; import com.cloudbees.jenkins.GitHubRepositoryNameContributor; import com.github.tomakehurst.wiremock.junit.WireMockRule; +import hudson.model.Item; import hudson.model.Job; import org.jenkinsci.plugins.github.config.GitHubServerConfig; import org.junit.rules.TestRule; @@ -157,7 +158,7 @@ private GHMockRule addSetup(Runnable setup) { */ public static class FixedGHRepoNameTestContributor extends GitHubRepositoryNameContributor { @Override - public void parseAssociatedNames(Job job, Collection result) { + public void parseAssociatedNames(Item job, Collection result) { result.add(GHMockRule.REPO); } } diff --git a/src/test/java/org/jenkinsci/plugins/github/util/JobInfoHelpersTest.java b/src/test/java/org/jenkinsci/plugins/github/util/JobInfoHelpersTest.java index 6571a5911..04de9b1bb 100644 --- a/src/test/java/org/jenkinsci/plugins/github/util/JobInfoHelpersTest.java +++ b/src/test/java/org/jenkinsci/plugins/github/util/JobInfoHelpersTest.java @@ -2,6 +2,7 @@ import com.cloudbees.jenkins.GitHubPushTrigger; import hudson.model.FreeStyleProject; +import hudson.model.Item; import org.jenkinsci.plugins.workflow.job.WorkflowJob; import org.junit.ClassRule; import org.junit.Test; @@ -70,7 +71,7 @@ public void shouldGetTriggerFromAbstractProject() throws Exception { FreeStyleProject prj = jenkins.createFreeStyleProject(); prj.addTrigger(trigger); - assertThat("with trigger in free style job", triggerFrom(prj, GitHubPushTrigger.class), is(trigger)); + assertThat("with trigger in free style job", triggerFrom((Item) prj, GitHubPushTrigger.class), is(trigger)); } @Test @@ -79,13 +80,13 @@ public void shouldGetTriggerFromWorkflow() throws Exception { WorkflowJob job = jenkins.getInstance().createProject(WorkflowJob.class, "Test Workflow"); job.addTrigger(trigger); - assertThat("with trigger in workflow", triggerFrom(job, GitHubPushTrigger.class), is(trigger)); + assertThat("with trigger in workflow", triggerFrom((Item) job, GitHubPushTrigger.class), is(trigger)); } @Test public void shouldNotGetTriggerWhenNoOne() throws Exception { FreeStyleProject prj = jenkins.createFreeStyleProject(); - assertThat("without trigger in project", triggerFrom(prj, GitHubPushTrigger.class), nullValue()); + assertThat("without trigger in project", triggerFrom((Item) prj, GitHubPushTrigger.class), nullValue()); } } 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 e6952fc12..eb9bb37e1 100644 --- a/src/test/java/org/jenkinsci/plugins/github/webhook/WebhookManagerTest.java +++ b/src/test/java/org/jenkinsci/plugins/github/webhook/WebhookManagerTest.java @@ -7,6 +7,7 @@ import com.google.common.collect.ImmutableSet; import com.google.common.collect.Lists; import hudson.model.FreeStyleProject; +import hudson.model.Item; import hudson.plugins.git.GitSCM; import org.jenkinsci.plugins.github.GitHubPlugin; import org.jenkinsci.plugins.github.config.GitHubServerConfig; @@ -183,7 +184,7 @@ public void shouldNotAddPushEventByDefaultForProjectWithoutTrigger() throws IOEx FreeStyleProject project = jenkins.createFreeStyleProject(); project.setScm(GIT_SCM); - manager.registerFor(project).run(); + manager.registerFor((Item)project).run(); verify(manager, never()).createHookSubscribedTo(anyListOf(GHEvent.class)); } @@ -193,7 +194,7 @@ public void shouldAddPushEventByDefault() throws IOException { project.addTrigger(new GitHubPushTrigger()); project.setScm(GIT_SCM); - manager.registerFor(project).run(); + manager.registerFor((Item)project).run(); verify(manager).createHookSubscribedTo(newArrayList(PUSH)); } From fb039230766f7a2cb4aea1ab1f8d009ccd4a9198 Mon Sep 17 00:00:00 2001 From: Stephen Connolly Date: Wed, 7 Dec 2016 16:27:29 +0000 Subject: [PATCH 2/4] Address code review comments from Oleg --- src/main/java/com/cloudbees/jenkins/Cleaner.java | 1 + src/main/java/com/cloudbees/jenkins/GitHubPushTrigger.java | 3 +++ .../com/cloudbees/jenkins/GitHubRepositoryNameContributor.java | 3 +++ src/main/java/com/cloudbees/jenkins/GitHubWebHook.java | 1 + .../jenkinsci/plugins/github/extension/GHEventsSubscriber.java | 2 ++ 5 files changed, 10 insertions(+) diff --git a/src/main/java/com/cloudbees/jenkins/Cleaner.java b/src/main/java/com/cloudbees/jenkins/Cleaner.java index 32930ba74..3f370ec61 100644 --- a/src/main/java/com/cloudbees/jenkins/Cleaner.java +++ b/src/main/java/com/cloudbees/jenkins/Cleaner.java @@ -40,6 +40,7 @@ public class Cleaner extends PeriodicWork { * Called when a {@link GitHubPushTrigger} is about to be removed. * @deprecated use {@link #onStop(Item)} */ + @Deprecated /* package */ void onStop(Item item) { cleanQueue.addAll(GitHubRepositoryNameContributor.parseAssociatedNames(item)); } diff --git a/src/main/java/com/cloudbees/jenkins/GitHubPushTrigger.java b/src/main/java/com/cloudbees/jenkins/GitHubPushTrigger.java index 7331a7c54..82a41b822 100644 --- a/src/main/java/com/cloudbees/jenkins/GitHubPushTrigger.java +++ b/src/main/java/com/cloudbees/jenkins/GitHubPushTrigger.java @@ -27,6 +27,8 @@ import org.jenkinsci.plugins.github.config.GitHubPluginConfig; import org.jenkinsci.plugins.github.internal.GHPluginConfigException; import org.jenkinsci.plugins.github.migration.Migrator; +import org.kohsuke.accmod.Restricted; +import org.kohsuke.accmod.restrictions.NoExternalUse; import org.kohsuke.stapler.AncestorInPath; import org.kohsuke.stapler.DataBoundConstructor; import org.slf4j.Logger; @@ -377,6 +379,7 @@ private static ThreadFactory threadFactory() { * @since TODO */ @SuppressWarnings("unused") + @Restricted(NoExternalUse.class) // invoked from Stapler public FormValidation doCheckHookRegistered(@AncestorInPath Item job) { Preconditions.checkNotNull(job, "Item can't be null if wants to check hook in monitor"); diff --git a/src/main/java/com/cloudbees/jenkins/GitHubRepositoryNameContributor.java b/src/main/java/com/cloudbees/jenkins/GitHubRepositoryNameContributor.java index d0c0bd2de..f5a0550c3 100644 --- a/src/main/java/com/cloudbees/jenkins/GitHubRepositoryNameContributor.java +++ b/src/main/java/com/cloudbees/jenkins/GitHubRepositoryNameContributor.java @@ -57,6 +57,9 @@ public void parseAssociatedNames(AbstractProject job, Collection result) { diff --git a/src/main/java/com/cloudbees/jenkins/GitHubWebHook.java b/src/main/java/com/cloudbees/jenkins/GitHubWebHook.java index 16436a08d..736ef8501 100644 --- a/src/main/java/com/cloudbees/jenkins/GitHubWebHook.java +++ b/src/main/java/com/cloudbees/jenkins/GitHubWebHook.java @@ -84,6 +84,7 @@ public void registerHookFor(Job job) { * {@code GitHubWebHook.get().registerHookFor(item);} * * @param item not null item to register hook for + * @since FIXME */ public void registerHookFor(Item item) { reRegisterHookForJob().apply(item); diff --git a/src/main/java/org/jenkinsci/plugins/github/extension/GHEventsSubscriber.java b/src/main/java/org/jenkinsci/plugins/github/extension/GHEventsSubscriber.java index def37fb5b..20f563a68 100644 --- a/src/main/java/org/jenkinsci/plugins/github/extension/GHEventsSubscriber.java +++ b/src/main/java/org/jenkinsci/plugins/github/extension/GHEventsSubscriber.java @@ -8,6 +8,7 @@ import hudson.model.Job; import java.lang.reflect.Method; import java.lang.reflect.Modifier; +import javax.annotation.CheckForNull; import jenkins.model.Jenkins; import org.jenkinsci.plugins.github.util.misc.NullSafeFunction; import org.jenkinsci.plugins.github.util.misc.NullSafePredicate; @@ -35,6 +36,7 @@ */ public abstract class GHEventsSubscriber implements ExtensionPoint { private static final Logger LOGGER = LoggerFactory.getLogger(GHEventsSubscriber.class); + @CheckForNull private transient Boolean hasIsApplicableItem; /** From 4088eb379cdc3808f276fc77ac79df51f454969f Mon Sep 17 00:00:00 2001 From: Stephen Connolly Date: Thu, 8 Dec 2016 16:13:24 +0000 Subject: [PATCH 3/4] not actually deprecated --- src/main/java/com/cloudbees/jenkins/Cleaner.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/main/java/com/cloudbees/jenkins/Cleaner.java b/src/main/java/com/cloudbees/jenkins/Cleaner.java index 3f370ec61..182ece08e 100644 --- a/src/main/java/com/cloudbees/jenkins/Cleaner.java +++ b/src/main/java/com/cloudbees/jenkins/Cleaner.java @@ -38,9 +38,7 @@ public class Cleaner extends PeriodicWork { /** * Called when a {@link GitHubPushTrigger} is about to be removed. - * @deprecated use {@link #onStop(Item)} */ - @Deprecated /* package */ void onStop(Item item) { cleanQueue.addAll(GitHubRepositoryNameContributor.parseAssociatedNames(item)); } From 7608d994fe63e939b7159549ccf3bb78c1403e48 Mon Sep 17 00:00:00 2001 From: Stephen Connolly Date: Fri, 9 Dec 2016 09:16:28 +0000 Subject: [PATCH 4/4] Address review comments --- .../java/com/cloudbees/jenkins/GitHubPushTrigger.java | 10 +++++----- .../jenkins/GitHubRepositoryNameContributor.java | 10 +++++----- .../plugins/github/util/FluentIterableWrapper.java | 1 + .../jenkinsci/plugins/github/util/JobInfoHelpers.java | 1 + .../plugins/github/webhook/WebhookManager.java | 1 + 5 files changed, 13 insertions(+), 10 deletions(-) diff --git a/src/main/java/com/cloudbees/jenkins/GitHubPushTrigger.java b/src/main/java/com/cloudbees/jenkins/GitHubPushTrigger.java index 82a41b822..18c448497 100644 --- a/src/main/java/com/cloudbees/jenkins/GitHubPushTrigger.java +++ b/src/main/java/com/cloudbees/jenkins/GitHubPushTrigger.java @@ -370,20 +370,20 @@ private static ThreadFactory threadFactory() { } /** - * Checks that repo defined in this job is not in administrative monitor as failed to be registered. + * Checks that repo defined in this item is not in administrative monitor as failed to be registered. * If that so, shows warning with some instructions * - * @param job - to check against. Should be not null and have at least one repo defined + * @param item - to check against. Should be not null and have at least one repo defined * * @return warning or empty string * @since TODO */ @SuppressWarnings("unused") @Restricted(NoExternalUse.class) // invoked from Stapler - public FormValidation doCheckHookRegistered(@AncestorInPath Item job) { - Preconditions.checkNotNull(job, "Item can't be null if wants to check hook in monitor"); + public FormValidation doCheckHookRegistered(@AncestorInPath Item item) { + Preconditions.checkNotNull(item, "Item can't be null if wants to check hook in monitor"); - Collection repos = GitHubRepositoryNameContributor.parseAssociatedNames(job); + Collection repos = GitHubRepositoryNameContributor.parseAssociatedNames(item); for (GitHubRepositoryName repo : repos) { if (monitor.isProblemWith(repo)) { diff --git a/src/main/java/com/cloudbees/jenkins/GitHubRepositoryNameContributor.java b/src/main/java/com/cloudbees/jenkins/GitHubRepositoryNameContributor.java index f5a0550c3..3fd042cd9 100644 --- a/src/main/java/com/cloudbees/jenkins/GitHubRepositoryNameContributor.java +++ b/src/main/java/com/cloudbees/jenkins/GitHubRepositoryNameContributor.java @@ -124,11 +124,11 @@ public static Collection parseAssociatedNames(Item item) { @Extension public static class FromSCM extends GitHubRepositoryNameContributor { @Override - public void parseAssociatedNames(Item job, Collection result) { - SCMTriggerItem item = SCMTriggerItems.asSCMTriggerItem(job); - EnvVars envVars = job instanceof Job ? buildEnv((Job) job) : new EnvVars(); - if (item != null) { - for (SCM scm : item.getSCMs()) { + public void parseAssociatedNames(Item item, Collection result) { + SCMTriggerItem triggerItem = SCMTriggerItems.asSCMTriggerItem(item); + EnvVars envVars = item instanceof Job ? buildEnv((Job) item) : new EnvVars(); + if (triggerItem != null) { + for (SCM scm : triggerItem.getSCMs()) { addRepositories(scm, envVars, result); } } diff --git a/src/main/java/org/jenkinsci/plugins/github/util/FluentIterableWrapper.java b/src/main/java/org/jenkinsci/plugins/github/util/FluentIterableWrapper.java index 76430a82c..fef4b4e86 100644 --- a/src/main/java/org/jenkinsci/plugins/github/util/FluentIterableWrapper.java +++ b/src/main/java/org/jenkinsci/plugins/github/util/FluentIterableWrapper.java @@ -82,6 +82,7 @@ public final FluentIterableWrapper filter(Predicate predicate) { /** * Returns the elements from this fluent iterable that are instances of the supplied type. The * resulting fluent iterable's iterator does not support {@code remove()}. + * @since FIXME */ @CheckReturnValue public final FluentIterableWrapper filter(Class clazz) { diff --git a/src/main/java/org/jenkinsci/plugins/github/util/JobInfoHelpers.java b/src/main/java/org/jenkinsci/plugins/github/util/JobInfoHelpers.java index 7d145ba59..6116343ec 100644 --- a/src/main/java/org/jenkinsci/plugins/github/util/JobInfoHelpers.java +++ b/src/main/java/org/jenkinsci/plugins/github/util/JobInfoHelpers.java @@ -103,6 +103,7 @@ public static T triggerFrom(Job job, Class tClass) * @param type of trigger * * @return Trigger instance with required class or null + * @since FIXME * TODO use standard method in 1.621+ */ @CheckForNull 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 68ae5a793..dda486407 100644 --- a/src/main/java/org/jenkinsci/plugins/github/webhook/WebhookManager.java +++ b/src/main/java/org/jenkinsci/plugins/github/webhook/WebhookManager.java @@ -101,6 +101,7 @@ public Runnable registerFor(final Job project) { * * @return runnable to create hooks on run * @see #createHookSubscribedTo(List) + * @since FIXME */ public Runnable registerFor(final Item item) { final Collection names = parseAssociatedNames(item);