fix: include target ref in GitHubPushCause

Author: dnwe

src/main/java/com/cloudbees/jenkins/GitHubPushCause.java

@@ -19,38 +19,45 @@ public class GitHubPushCause extends SCMTriggerCause {
      * The name of the user who pushed to GitHub.
      */
     private String pushedBy;
+    /**
+     * The target ref of the triggering GitHub push.
+     */
+    private String ref;
 
-    public GitHubPushCause(String pusher) {
-        this("", pusher);
+    public GitHubPushCause(String pusher, String ref) {
+        this("", pusher, ref);
     }
 
-    public GitHubPushCause(String pollingLog, String pusher) {
+    public GitHubPushCause(String pollingLog, String pusher, String ref) {
         super(pollingLog);
-        pushedBy = pusher;
+        this.pushedBy = pusher;
+        this.ref = ref;
     }
 
-    public GitHubPushCause(File pollingLog, String pusher) throws IOException {
+    public GitHubPushCause(File pollingLog, String pusher, String ref) throws IOException {
         super(pollingLog);
-        pushedBy = pusher;
+        this.pushedBy = pusher;
+        this.ref = ref;
     }
 
     @Override
     public String getShortDescription() {
-        return format("Started by GitHub push by %s", trimToEmpty(pushedBy));
+        return format("Started by GitHub push to %s by %s", trimToEmpty(ref), trimToEmpty(pushedBy));
     }
 
     @Override
     public boolean equals(Object o) {
         return o instanceof GitHubPushCause
                 && Objects.equals(this.pushedBy, ((GitHubPushCause) o).pushedBy)
+                && Objects.equals(this.ref, ((GitHubPushCause) o).ref)
                 && super.equals(o);
     }
 
     @Override
     public int hashCode() {
         int hash = super.hashCode();
         hash = 89 * hash + Objects.hash(this.pushedBy);
+        hash = 89 * hash + Objects.hash(this.ref);
         return hash;
     }
 }
-

src/main/java/com/cloudbees/jenkins/GitHubPushTrigger.java

@@ -83,10 +83,11 @@ public void onPost() {
     /**
      * Called when a POST is made.
      */
-    public void onPost(String triggeredByUser) {
+    public void onPost(String triggeredByUser, String triggeredByRef) {
         onPost(GitHubTriggerEvent.create()
                 .withOrigin(SCMEvent.originOf(Stapler.getCurrentRequest()))
                 .withTriggeredByUser(triggeredByUser)
+                .withTriggeredByRef(triggeredByRef)
                 .build()
         );
     }
@@ -102,6 +103,7 @@ public void onPost(final GitHubTriggerEvent event) {
         Job<?, ?> currentJob = notNull(job, "Job can't be null");
 
         final String pushBy = event.getTriggeredByUser();
+        final String ref = event.getTriggeredByRef();
         DescriptorImpl d = getDescriptor();
         d.checkThreadPoolSizeAndUpdateIfNecessary();
         d.queue.execute(new Runnable() {
@@ -150,10 +152,10 @@ public void run() {
                 if (runPolling()) {
                     GitHubPushCause cause;
                     try {
-                        cause = new GitHubPushCause(getLogFileForJob(currentJob), pushBy);
+                        cause = new GitHubPushCause(getLogFileForJob(currentJob), pushBy, ref);
                     } catch (IOException e) {
                         LOGGER.warn("Failed to parse the polling log", e);
-                        cause = new GitHubPushCause(pushBy);
+                        cause = new GitHubPushCause(pushBy, ref);
                     }
 
                     if (asParameterizedJobMixIn(currentJob).scheduleBuild(cause)) {

src/main/java/com/cloudbees/jenkins/GitHubRepositoryNameContributor.java

@@ -112,7 +112,7 @@ public void parseAssociatedNames(AbstractProject<?, ?> job, Collection<GitHubRep
                 getClass(),
                 "parseAssociatedBranches",
                 AbstractProject.class,
-            Collection.class
+                Collection.class
         )) {
             // if this impl is legacy, it cannot contribute to non-projects, so not an error
             if (item instanceof AbstractProject) {
@@ -151,6 +151,22 @@ public static Collection<GitHubRepositoryName> parseAssociatedNames(Item item) {
         return names;
     }
 
+    /**
+     * @deprecated Use {@link #parseAssociatedBranches(Job)}
+     */
+    @Deprecated
+    public static Collection<GitHubBranch> parseAssociatedBranches(AbstractProject<?, ?> job) {
+        return parseAssociatedBranches((Item) job);
+    }
+
+    /**
+     * @deprecated Use {@link #parseAssociatedBranches(Item)}
+     */
+    @Deprecated
+    public static Collection<GitHubBranch> parseAssociatedBranches(Job<?, ?> job) {
+        return parseAssociatedBranches((Item) job);
+    }
+
     public static Collection<GitHubBranch> parseAssociatedBranches(Item item) {
         Set<GitHubBranch> names = new HashSet<GitHubBranch>();
         for (GitHubRepositoryNameContributor c : all()) {

src/main/java/com/cloudbees/jenkins/GitHubTrigger.java

@@ -23,7 +23,7 @@ public interface GitHubTrigger {
     void onPost();
 
     // TODO: document me
-    void onPost(String triggeredByUser);
+    void onPost(String triggeredByUser, String triggeredByRef);
 
     /**
      * Obtains the list of the repositories that this trigger is looking at.

src/main/java/com/cloudbees/jenkins/GitHubTriggerEvent.java

@@ -21,11 +21,16 @@ public class GitHubTriggerEvent {
      * The user that the event was provided by.
      */
     private final String triggeredByUser;
+    /**
+     * The target ref that the push event was for.
+     */
+    private final String triggeredByRef;
 
-    private GitHubTriggerEvent(long timestamp, String origin, String triggeredByUser) {
+    private GitHubTriggerEvent(long timestamp, String origin, String triggeredByUser, String triggeredByRef) {
         this.timestamp = timestamp;
         this.origin = origin;
         this.triggeredByUser = triggeredByUser;
+        this.triggeredByRef = triggeredByRef;
     }
 
     public static Builder create() {
@@ -44,6 +49,10 @@ public String getTriggeredByUser() {
         return triggeredByUser;
     }
 
+    public String getTriggeredByRef() {
+        return triggeredByRef;
+    }
+
     @Override
     public boolean equals(Object o) {
         if (this == o) {
@@ -61,14 +70,18 @@ public boolean equals(Object o) {
         if (origin != null ? !origin.equals(that.origin) : that.origin != null) {
             return false;
         }
-        return triggeredByUser != null ? triggeredByUser.equals(that.triggeredByUser) : that.triggeredByUser == null;
+        if (triggeredByUser != null ? !triggeredByUser.equals(that.triggeredByUser) : that.triggeredByUser != null) {
+            return false;
+        }
+        return triggeredByRef != null ? triggeredByRef.equals(that.triggeredByRef) : that.triggeredByRef == null;
     }
 
     @Override
     public int hashCode() {
         int result = (int) (timestamp ^ (timestamp >>> 32));
         result = 31 * result + (origin != null ? origin.hashCode() : 0);
         result = 31 * result + (triggeredByUser != null ? triggeredByUser.hashCode() : 0);
+        result = 31 * result + (triggeredByRef != null ? triggeredByRef.hashCode() : 0);
         return result;
     }
 
@@ -78,6 +91,7 @@ public String toString() {
                 + "timestamp=" + timestamp
                 + ", origin='" + origin + '\''
                 + ", triggeredByUser='" + triggeredByUser + '\''
+                + ", triggeredByRef='" + triggeredByRef + '\''
                 + '}';
     }
 
@@ -88,6 +102,7 @@ public static class Builder {
         private long timestamp;
         private String origin;
         private String triggeredByUser;
+        private String triggeredByRef;
 
         private Builder() {
             timestamp = System.currentTimeMillis();
@@ -108,8 +123,13 @@ public Builder withTriggeredByUser(String triggeredByUser) {
             return this;
         }
 
+        public Builder withTriggeredByRef(String triggeredByRef) {
+            this.triggeredByRef = triggeredByRef;
+            return this;
+        }
+
         public GitHubTriggerEvent build() {
-            return new GitHubTriggerEvent(timestamp, origin, triggeredByUser);
+            return new GitHubTriggerEvent(timestamp, origin, triggeredByUser, triggeredByRef);
         }
 
         @Override
@@ -118,6 +138,7 @@ public String toString() {
                     + "timestamp=" + timestamp
                     + ", origin='" + origin + '\''
                     + ", triggeredByUser='" + triggeredByUser + '\''
+                    + ", triggeredByRef='" + triggeredByRef + '\''
                     + '}';
         }
     }

src/main/java/org/jenkinsci/plugins/github/webhook/subscriber/DefaultPushGHEventSubscriber.java

@@ -22,6 +22,7 @@
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
+import java.util.Collection;
 import java.util.Set;
 
 import static com.google.common.collect.Sets.immutableEnumSet;
@@ -66,6 +67,9 @@ protected Set<GHEvent> events() {
      * @param event   only PUSH event
      */
     @Override
+    @edu.umd.cs.findbugs.annotations.SuppressFBWarnings(
+        value = "SIC_INNER_SHOULD_BE_STATIC_ANON", justification = "anonymous inner class is acceptable"
+    )
     protected void onEvent(final GHSubscriberEvent event) {
         GHEventPayload.Push push;
         try {
@@ -77,7 +81,7 @@ protected void onEvent(final GHSubscriberEvent event) {
         URL repoUrl = push.getRepository().getUrl();
         final String pusherName = push.getPusher().getName();
         final String ref = push.getRef();
-        LOGGER.info("Received PushEvent for {} from {}", repoUrl, event.getOrigin());
+        LOGGER.info("Received PushEvent for {} {} from {}", repoUrl, ref, event.getOrigin());
         GitHubRepositoryName fromEventRepository = GitHubRepositoryName.create(repoUrl.toExternalForm());
 
         if (fromEventRepository == null) {
@@ -94,12 +98,8 @@ protected void onEvent(final GHSubscriberEvent event) {
         }
 
         final GitHubRepositoryName changedRepository = fromEventRepository;
-
         if (changedRepository != null) {
-            // run in high privilege to see all the projects anonymous users don't see.
-            // this is safe because when we actually schedule a build, it's a build that can
-            // happen at some random time anyway.
-            Runnable body = new Runnable() {
+            final Runnable body = new Runnable() {
                 @Override
                 public void run() {
                     for (Item job : Jenkins.get().getAllItems(Item.class)) {
@@ -109,32 +109,48 @@ public void run() {
                         }
                         String fullDisplayName = job.getFullDisplayName();
                         LOGGER.debug("Considering to poke {}", fullDisplayName);
-                        if (!GitHubRepositoryNameContributor.parseAssociatedNames(job).contains(changedRepository)) {
-                            LOGGER.debug("Skipped {} because it doesn't have a matching repository.", fullDisplayName);
+                        final Collection<GitHubRepositoryName> names =
+                            GitHubRepositoryNameContributor.parseAssociatedNames(job);
+                        if (!names.contains(changedRepository)) {
+                            LOGGER.debug(
+                                "Skipped {} because {} doesn't have a matching repository for {}.",
+                                fullDisplayName, names, changedRepository);
                             continue;
                         }
-                        boolean foundBranch = false;
-                        for (GitHubBranch branch : GitHubRepositoryNameContributor.parseAssociatedBranches(job)) {
-                            if (branch.matches(changedRepository, ref)) {
-                                foundBranch = true;
-                                break;
+                        try {
+                            final Collection<GitHubBranch> branchSpecs =
+                                GitHubRepositoryNameContributor.parseAssociatedBranches(job);
+                            if (!branchSpecs.isEmpty()) {
+                                boolean foundBranch = false;
+                                for (GitHubBranch branch : branchSpecs) {
+                                    if (branch.matches(changedRepository, ref)) {
+                                        foundBranch = true;
+                                        break;
+                                    }
+                                }
+                                if (!foundBranch) {
+                                    LOGGER.debug("Skipped {} because it doesn't have a matching branch specifier.",
+                                        job.getFullDisplayName());
+                                    continue;
+                                }
                             }
-                        }
-                        if (!foundBranch) {
-                            LOGGER.debug("Skipped {} because it doesn't have a matching branch specifier.",
-                                job.getFullDisplayName());
-                            continue;
+                        } catch (Exception e) {
+                            LOGGER.error("parseAssociatedBranches threw exception", e);
                         }
                         LOGGER.info("Poked {}", fullDisplayName);
                         trigger.onPost(GitHubTriggerEvent.create()
                                 .withTimestamp(event.getTimestamp())
                                 .withOrigin(event.getOrigin())
                                 .withTriggeredByUser(pusherName)
+                                .withTriggeredByRef(ref)
                                 .build()
                         );
                     }
                 }
             };
+            // run in high privilege to see all the projects anonymous users don't see.
+            // this is safe because when we actually schedule a build, it's a build that can
+            // happen at some random time anyway.
             ACL.impersonate(ACL.SYSTEM, body);
 
             for (GitHubWebHook.Listener listener : ExtensionList.lookup(GitHubWebHook.Listener.class)) {

src/test/java/com/cloudbees/jenkins/GitHubPushTriggerTest.java

@@ -26,6 +26,7 @@
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.is;
 import static org.jenkinsci.plugins.github.webhook.subscriber.DefaultPushGHEventListenerTest.TRIGGERED_BY_USER_FROM_RESOURCE;
+import static org.jenkinsci.plugins.github.webhook.subscriber.DefaultPushGHEventListenerTest.TRIGGERED_BY_REF_FROM_RESOURCE;
 
 /**
  * @author lanwen (Merkushev Kirill)
@@ -69,7 +70,7 @@ public void shouldStartWorkflowByTrigger() throws Exception {
         buildData.buildsByBranchName = new HashMap<String, Build>();
         buildData.getLastBuiltRevision().setSha1(ObjectId.zeroId());
 
-        trigger.onPost(TRIGGERED_BY_USER_FROM_RESOURCE);
+        trigger.onPost(TRIGGERED_BY_USER_FROM_RESOURCE, TRIGGERED_BY_REF_FROM_RESOURCE);
 
         TimeUnit.SECONDS.sleep(job.getQuietPeriod());
         jRule.waitUntilNoActivity();

src/test/java/org/jenkinsci/plugins/github/webhook/subscriber/DefaultPushGHEventListenerTest.java

@@ -29,6 +29,7 @@ public class DefaultPushGHEventListenerTest {
 
     public static final GitSCM GIT_SCM_FROM_RESOURCE = new GitSCM("ssh://[email protected]/lanwen/test.git");
     public static final String TRIGGERED_BY_USER_FROM_RESOURCE = "lanwen";
+    public static final String TRIGGERED_BY_REF_FROM_RESOURCE = "refs/heads/master";    
 
     @Rule
     public JenkinsRule jenkins = new JenkinsRule();
@@ -62,6 +63,7 @@ public void shouldParsePushPayload() throws Exception {
                 .withTimestamp(subscriberEvent.getTimestamp())
                 .withOrigin("shouldParsePushPayload")
                 .withTriggeredByUser(TRIGGERED_BY_USER_FROM_RESOURCE)
+                .withTriggeredByRef(TRIGGERED_BY_REF_FROM_RESOURCE)
                 .build()
         ));
     }
@@ -85,6 +87,7 @@ public void shouldReceivePushHookOnWorkflow() throws Exception {
                 .withTimestamp(subscriberEvent.getTimestamp())
                 .withOrigin("shouldReceivePushHookOnWorkflow")
                 .withTriggeredByUser(TRIGGERED_BY_USER_FROM_RESOURCE)
+                .withTriggeredByRef(TRIGGERED_BY_REF_FROM_RESOURCE)
                 .build()
         ));
     }