[JENKINS-36123] Address review comments from James Nord

jenkinsci · Jun 22, 2016 · 5a2a265 · 5a2a265
1 parent 4ac67a4
commit 5a2a265
Show file tree

Hide file tree

Showing 6 changed files with 54 additions and 38 deletions.
diff --git a/core/src/main/java/hudson/cli/BuildCommand.java b/core/src/main/java/hudson/cli/BuildCommand.java
@@ -151,10 +151,8 @@ protected int run() throws Exception {
             if (item == null)
                 throw new AbortException(job.getFullDisplayName()+" has no SCM trigger, but checkSCM was specified");
             // pre-emtively check for a polling veto
-            for (SCMPollingDecisionHandler handler : SCMPollingDecisionHandler.all()) {
-                if (!handler.shouldPoll(job)) {
-                    return 0;
-                }
+            if (SCMPollingDecisionHandler.firstVeto(job) != null) {
+                return 0;
             }
             if (!item.poll(new StreamTaskListener(stdout, getClientCharset())).hasChanges())
                 return 0;

diff --git a/core/src/main/java/hudson/model/AbstractProject.java b/core/src/main/java/hudson/model/AbstractProject.java
@@ -1323,11 +1323,10 @@ public PollingResult poll( TaskListener listener ) {
             listener.getLogger().println(Messages.AbstractProject_Disabled());
             return NO_CHANGES;
         }
-        for (SCMPollingDecisionHandler handler : SCMPollingDecisionHandler.all()) {
-            if (!handler.shouldPoll(this)) {
-                listener.getLogger().println(Messages.AbstractProject_PollingVetoed(handler));
-                return NO_CHANGES;
-            }
+        SCMPollingDecisionHandler veto = SCMPollingDecisionHandler.firstVeto(this);
+        if (veto != null) {
+            listener.getLogger().println(Messages.AbstractProject_PollingVetoed(veto));
+            return NO_CHANGES;
         }
 
         R lb = getLastBuild();

diff --git a/core/src/main/java/hudson/triggers/SCMTrigger.java b/core/src/main/java/hudson/triggers/SCMTrigger.java
@@ -27,8 +27,6 @@
 import antlr.ANTLRException;
 import com.google.common.base.Preconditions;
 import hudson.Extension;
-import hudson.ExtensionList;
-import hudson.ExtensionPoint;
 import hudson.Util;
 import hudson.console.AnnotatedLargeText;
 import hudson.model.AbstractBuild;
@@ -39,7 +37,6 @@
 import hudson.model.CauseAction;
 import hudson.model.Item;
 import hudson.model.Run;
-import hudson.model.listeners.SCMPollListener;
 import hudson.scm.SCM;
 import hudson.scm.SCMDescriptor;
 import hudson.util.FlushProofOutputStream;
@@ -49,15 +46,6 @@
 import hudson.util.SequentialExecutionQueue;
 import hudson.util.StreamTaskListener;
 import hudson.util.TimeUnit2;
-import jenkins.scm.SCMPollingDecisionHandler;
-import org.apache.commons.io.FileUtils;
-import org.apache.commons.jelly.XMLOutput;
-import org.jenkinsci.Symbol;
-import org.kohsuke.accmod.Restricted;
-import org.kohsuke.accmod.restrictions.DoNotUse;
-import org.kohsuke.stapler.StaplerRequest;
-import org.kohsuke.stapler.DataBoundConstructor;
-
 import java.io.File;
 import java.io.IOException;
 import java.io.PrintStream;
@@ -76,15 +64,23 @@
 import java.util.logging.Level;
 import java.util.logging.Logger;
 import jenkins.model.Jenkins;
+import jenkins.model.RunAction2;
+import jenkins.scm.SCMPollingDecisionHandler;
 import jenkins.triggers.SCMTriggerItem;
+import jenkins.util.SystemProperties;
 import net.sf.json.JSONObject;
+import org.apache.commons.io.FileUtils;
+import org.apache.commons.jelly.XMLOutput;
+import org.jenkinsci.Symbol;
+import org.kohsuke.accmod.Restricted;
+import org.kohsuke.accmod.restrictions.DoNotUse;
 import org.kohsuke.accmod.restrictions.NoExternalUse;
+import org.kohsuke.stapler.DataBoundConstructor;
 import org.kohsuke.stapler.QueryParameter;
+import org.kohsuke.stapler.StaplerRequest;
 import org.kohsuke.stapler.StaplerResponse;
 
-import static java.util.logging.Level.*;
-import jenkins.model.RunAction2;
-import jenkins.util.SystemProperties;
+import static java.util.logging.Level.WARNING;
 
 
 /**
@@ -555,13 +551,20 @@ public void run() {
             }
             // we can pre-emtively check the SCMPollingDecisionHandler instances here
             // note that job().poll(listener) should also check this
-            for (SCMPollingDecisionHandler handler : SCMPollingDecisionHandler.all()) {
-                if (!handler.shouldPoll(job)) {
-                    LOGGER.log(Level.FINE, "Skipping polling for {0} due to veto from {1}",
-                            new Object[]{job.getFullDisplayName(), handler}
-                    );
-                    return;
+            SCMPollingDecisionHandler veto = SCMPollingDecisionHandler.firstVeto(job);
+            if (veto != null) {
+                try (StreamTaskListener listener = new StreamTaskListener(getLogFile())) {
+                    listener.getLogger().println(
+                            "Skipping polling on " + DateFormat.getDateTimeInstance().format(new Date())
+                                    + " due to veto from " + veto);
+                } catch (IOException e) {
+                    LOGGER.log(Level.SEVERE, "Failed to record SCM polling for " + job, e);
                 }
+
+                LOGGER.log(Level.FINE, "Skipping polling for {0} due to veto from {1}",
+                        new Object[]{job.getFullDisplayName(), veto}
+                );
+                return;
             }
 
             String threadName = Thread.currentThread().getName();

diff --git a/core/src/main/java/jenkins/scm/SCMPollingDecisionHandler.java b/core/src/main/java/jenkins/scm/SCMPollingDecisionHandler.java
@@ -26,6 +26,8 @@
 import hudson.ExtensionList;
 import hudson.ExtensionPoint;
 import hudson.model.Item;
+import javax.annotation.CheckForNull;
+import javax.annotation.Nonnull;
 
 /**
  * Extension point for deciding if particular job should be polled or not.
@@ -42,13 +44,29 @@ public abstract class SCMPollingDecisionHandler implements ExtensionPoint {
      *
      * @param item The item.
      */
-    public abstract boolean shouldPoll(Item item);
+    public abstract boolean shouldPoll(@Nonnull Item item);
 
     /**
      * All registered {@link SCMPollingDecisionHandler}s
      */
+    @Nonnull
     public static ExtensionList<SCMPollingDecisionHandler> all() {
         return ExtensionList.lookup(SCMPollingDecisionHandler.class);
     }
 
+    /**
+     * Returns the first {@link SCMPollingDecisionHandler} that returns {@code false} from {@link #shouldPoll(Item)}
+     * @param item the item
+     * @return the first veto or {@code null} if there are no vetos
+     */
+    @CheckForNull
+    public static SCMPollingDecisionHandler firstVeto(@Nonnull Item item) {
+        for (SCMPollingDecisionHandler handler : all()) {
+            if (!handler.shouldPoll(item)) {
+                return handler;
+            }
+        }
+        return null;
+    }
+
 }
diff --git a/core/src/main/java/jenkins/triggers/SCMTriggerItem.java b/core/src/main/java/jenkins/triggers/SCMTriggerItem.java
@@ -120,11 +120,10 @@ private static final class Bridge implements SCMTriggerItem {
                 return delegate.asProject().scheduleBuild2(quietPeriod, null, actions);
             }
             @Override public PollingResult poll(TaskListener listener) {
-                for (SCMPollingDecisionHandler handler : SCMPollingDecisionHandler.all()) {
-                    if (!handler.shouldPoll(asItem())) {
-                        listener.getLogger().println(Messages.SCMTriggerItem_PollingVetoed(handler));
-                        return PollingResult.NO_CHANGES;
-                    }
+                SCMPollingDecisionHandler veto = SCMPollingDecisionHandler.firstVeto(asItem());
+                if (!veto.shouldPoll(asItem())) {
+                    listener.getLogger().println(Messages.SCMTriggerItem_PollingVetoed(veto));
+                    return PollingResult.NO_CHANGES;
                 }
                 return delegate.poll(listener);
             }

diff --git a/test/src/test/java/hudson/triggers/SCMTriggerTest.java b/test/src/test/java/hudson/triggers/SCMTriggerTest.java
@@ -88,8 +88,7 @@ public void simultaneousPollAndBuild() throws Exception {
     }
 
     /**
-     * Make sure that SCMTrigger doesn't trigger another build when a build has just started,
-     * but not yet completed its SCM update.
+     * Make sure that SCMTrigger doesn't poll when there is a polling veto in place.
      */
     @Test
     @Issue("JENKINS-36123")