Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions src/main/java/com/cloudbees/jenkins/Cleaner.java
Original file line number Diff line number Diff line change
@@ -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;
Expand All @@ -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
Expand All @@ -39,8 +39,8 @@ public class Cleaner extends PeriodicWork {
/**
* Called when a {@link GitHubPushTrigger} is about to be removed.
*/
/* package */ void onStop(Job<?, ?> job) {
cleanQueue.addAll(GitHubRepositoryNameContributor.parseAssociatedNames(job));
/* package */ void onStop(Item item) {
cleanQueue.addAll(GitHubRepositoryNameContributor.parseAssociatedNames(item));
}

@Override
Expand All @@ -61,8 +61,8 @@ protected void doRun() throws Exception {

URL url = GitHubPlugin.configuration().getHookUrl();

List<Job> jobs = Jenkins.getInstance().getAllItems(Job.class);
List<GitHubRepositoryName> aliveRepos = from(jobs)
List<Item> items = Jenkins.getInstance().getAllItems(Item.class);
List<GitHubRepositoryName> aliveRepos = from(items)
.filter(isAlive()) // live repos
.transformAndConcat(associatedNames()).toList();

Expand Down
13 changes: 8 additions & 5 deletions src/main/java/com/cloudbees/jenkins/GitHubPushTrigger.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -368,19 +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")
public FormValidation doCheckHookRegistered(@AncestorInPath Job<?, ?> job) {
Preconditions.checkNotNull(job, "Job can't be null if wants to check hook in monitor");
@Restricted(NoExternalUse.class) // invoked from Stapler
public FormValidation doCheckHookRegistered(@AncestorInPath Item item) {
Preconditions.checkNotNull(item, "Item can't be null if wants to check hook in monitor");

Collection<GitHubRepositoryName> repos = GitHubRepositoryNameContributor.parseAssociatedNames(job);
Collection<GitHubRepositoryName> repos = GitHubRepositoryNameContributor.parseAssociatedNames(item);

for (GitHubRepositoryName repo : repos) {
if (monitor.isProblemWith(repo)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -36,41 +37,57 @@ 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<GitHubRepositoryName> 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<GitHubRepositoryName> 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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@since

* @param item the item.
* @param result the collection to add repository names to
* @since FIXME
*/
private boolean overriddenMethodHasDeprecatedSignature(Job<?, ?> job) {
return Util.isOverridden(
@SuppressWarnings("deprecation")
public /*abstract*/ void parseAssociatedNames(Item item, Collection<GitHubRepositoryName> 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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe put a warning on the FINE level? Just in case somebody needs to debug the compat issue

}
} 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");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐛 So it will expose the error when somebody uses Predicates like withTrigger e.g. in DefaultPushGHEventSubscriber.#isApplicable(). Instead of returning false this method will start throwing Error after the patch if there is an incompatible contributor. Just if I read it right...

IMHO there should be one of the following fixes:

  • Log warning and do nothing
  • Replace the Error by non-runtime exception and implement correct handling in the upper logic

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually this is more a design error in the original API. These methods taking the collection should actually have been protected as they form part of the SPI not the API.

If you have an old implementation, then you will have overridden parseAssociatedNames(Job,Collection) or parseAssociatedNames(AbstractProject,Collection) and thus we delegate to those.

If you have a new implementation, then you are supposed to override parseAssociatedNames(Item,Collection) and if you don't then this exception should blow up in your face and give you the AME.

If we had these SPI methods as protected then we could make parseAssociatedNames(Item,Collection) abstract and use the API methods like parseAssociatedNames(Item) to route through to the implementation of existing extension points... but instead, as this is public API we have to do it this way and we have no way to force new implementations to override parseAssociatedNames(Item,Collection) other than this exception

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also please check the previous implementation of parseAssociatedNames(Job<?, ?> job, Collection<GitHubRepositoryName> result) which threw the exact same exception. All I am doing here is moving the exception throwing from the previously most generic method to the new most generic method.

The exception will only be thrown if you have an implementation that does not override any parseAssociatedNames methods. If this had been designed as an SPI then the implementation methods would be protected and we could have shifted the abstract and used a guard.

To clarify for @KostyaSha this is not a bug just migration of existing pattern

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stephenc only since

}
}

public static ExtensionList<GitHubRepositoryNameContributor> all() {
Expand All @@ -82,13 +99,21 @@ public static ExtensionList<GitHubRepositoryNameContributor> all() {
*/
@Deprecated
public static Collection<GitHubRepositoryName> parseAssociatedNames(AbstractProject<?, ?> job) {
return parseAssociatedNames((Job) job);
return parseAssociatedNames((Item) job);
}

/**
* @deprecated Use {@link #parseAssociatedNames(Item)}
*/
@Deprecated
public static Collection<GitHubRepositoryName> parseAssociatedNames(Job<?, ?> job) {
return parseAssociatedNames((Item) job);
}

public static Collection<GitHubRepositoryName> parseAssociatedNames(Item item) {
Set<GitHubRepositoryName> names = new HashSet<GitHubRepositoryName>();
for (GitHubRepositoryNameContributor c : all()) {
c.parseAssociatedNames(job, names);
c.parseAssociatedNames(item, names);
}
return names;
}
Expand All @@ -99,11 +124,11 @@ public static Collection<GitHubRepositoryName> parseAssociatedNames(Job<?, ?> jo
@Extension
public static class FromSCM extends GitHubRepositoryNameContributor {
@Override
public void parseAssociatedNames(Job<?, ?> job, Collection<GitHubRepositoryName> result) {
SCMTriggerItem item = SCMTriggerItems.asSCMTriggerItem(job);
EnvVars envVars = buildEnv(job);
if (item != null) {
for (SCM scm : item.getSCMs()) {
public void parseAssociatedNames(Item item, Collection<GitHubRepositoryName> 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);
}
}
Expand Down
8 changes: 4 additions & 4 deletions src/main/java/com/cloudbees/jenkins/GitHubTrigger.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -46,9 +46,9 @@ public interface GitHubTrigger {
@Extension
class GitHubRepositoryNameContributorImpl extends GitHubRepositoryNameContributor {
@Override
public void parseAssociatedNames(Job<?, ?> job, Collection<GitHubRepositoryName> result) {
if (job instanceof ParameterizedJobMixIn.ParameterizedJob) {
ParameterizedJobMixIn.ParameterizedJob p = (ParameterizedJobMixIn.ParameterizedJob) job;
public void parseAssociatedNames(Item item, Collection<GitHubRepositoryName> 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());
Expand Down
30 changes: 23 additions & 7 deletions src/main/java/com/cloudbees/jenkins/GitHubWebHook.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -70,21 +71,36 @@ 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
* @since FIXME
*/
public void registerHookFor(Item item) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@since

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<Job> reRegisterAllHooks() {
return from(getJenkinsInstance().getAllItems(Job.class))
public List<Item> reRegisterAllHooks() {
return from(getJenkinsInstance().getAllItems(Item.class))
.filter(isBuildable())
.filter(isAlive())
.transform(reRegisterHookForJob()).toList();
.transform(reRegisterHookForJob())
.toList();
}

/**
Expand All @@ -101,11 +117,11 @@ public void doIndex(@Nonnull @GHEventHeader GHEvent event, @Nonnull @GHEventPayl
.transform(processEvent(event, payload)).toList();
}

private Function<Job, Job> reRegisterHookForJob() {
return new Function<Job, Job>() {
private <T extends Item> Function<T, T> reRegisterHookForJob() {
return new Function<T, T>() {
@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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -181,10 +181,10 @@ public FormValidation doReRegister() {
return FormValidation.warning("Works only when Jenkins manages hooks (one ore more creds specified)");
}

List<Job> registered = GitHubWebHook.get().reRegisterAllHooks();
List<Item> 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")
Expand Down
Loading