fix(echo): Trigger properly off Github Pull Requests (#609)

* fix(echo): Trigger properly off Github Pull Requests

* Add test, add null checks

* Refactor

* Change mechanism of detecting push, change test

* Default to push

* Remove null conditional

* Fix to string

echo-webhooks/src/main/groovy/com/netflix/spinnaker/echo/scm/GithubWebhookEventHandler.java

@@ -21,8 +21,11 @@
 import com.fasterxml.jackson.databind.DeserializationFeature;
 import com.fasterxml.jackson.databind.ObjectMapper;
 import com.netflix.spinnaker.echo.model.Event;
+import com.netflix.spinnaker.echo.scm.github.GithubPullRequestEvent;
+import com.netflix.spinnaker.echo.scm.github.GithubPushEvent;
+import com.netflix.spinnaker.echo.scm.github.GithubWebhookEvent;
+import java.util.HashMap;
 import java.util.Map;
-import lombok.Data;
 import lombok.extern.slf4j.Slf4j;
 import org.springframework.stereotype.Component;
 
@@ -42,7 +45,11 @@ public boolean handles(String source) {
   }
 
   public boolean shouldSendEvent(Event event) {
-    return !event.content.containsKey("hook_id");
+    if (event.content.containsKey("hook_id")) {
+      return false;
+    }
+
+    return true;
   }
 
   public void handle(Event event, Map postedEvent) {
@@ -59,34 +66,35 @@ public void handle(Event event, Map postedEvent) {
       return;
     }
 
-    GithubWebhookEvent githubWebhookEvent =
-        objectMapper.convertValue(postedEvent, GithubWebhookEvent.class);
+    GithubWebhookEvent webhookEvent = null;
+    String githubEvent = "";
 
-    event.content.put("hash", githubWebhookEvent.after);
-    if (githubWebhookEvent.ref == null) {
-      event.content.put("branch", "");
+    // TODO: Detect based on header rather than body key - depends on Gate properly passing headers
+    // `x-github-event`: `push` or `pull_request`
+    if (event.content.containsKey("pull_request")) {
+      webhookEvent = objectMapper.convertValue(event.content, GithubPullRequestEvent.class);
+      githubEvent = "pull_request";
     } else {
-      event.content.put("branch", githubWebhookEvent.ref.replace("refs/heads/", ""));
+      // Default to 'Push'
+      webhookEvent = objectMapper.convertValue(event.content, GithubPushEvent.class);
+      githubEvent = "push";
     }
-    event.content.put("repoProject", githubWebhookEvent.repository.owner.name);
-    event.content.put("slug", githubWebhookEvent.repository.name);
-  }
 
-  @Data
-  private static class GithubWebhookEvent {
-    String after;
-    String ref;
-    GithubWebhookRepository repository;
-  }
-
-  @Data
-  private static class GithubWebhookRepository {
-    GithubOwner owner;
-    String name;
-  }
+    String fullRepoName = webhookEvent.getFullRepoName(event, postedEvent);
+    Map<String, String> results = new HashMap<>();
+    results.put("repoProject", webhookEvent.getRepoProject(event, postedEvent));
+    results.put("slug", webhookEvent.getSlug(event, postedEvent));
+    results.put("hash", webhookEvent.getHash(event, postedEvent));
+    results.put("branch", webhookEvent.getBranch(event, postedEvent));
+    event.content.putAll(results);
 
-  @Data
-  private static class GithubOwner {
-    String name;
+    log.info(
+        "Github Webhook event received: {} {} {} {} {} {}",
+        kv("githubEvent", githubEvent),
+        kv("repository", fullRepoName),
+        kv("project", results.get("repoProject")),
+        kv("slug", results.get("slug")),
+        kv("branch", results.get("branch")),
+        kv("hash", results.get("hash")));
   }
 }

echo-webhooks/src/main/groovy/com/netflix/spinnaker/echo/scm/github/GithubPullRequestEvent.java

@@ -0,0 +1,107 @@
+/*
+ * Copyright 2019 Netflix, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.netflix.spinnaker.echo.scm.github;
+
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.netflix.spinnaker.echo.model.Event;
+import java.util.Map;
+import java.util.Optional;
+import lombok.Data;
+
+@Data
+public class GithubPullRequestEvent implements GithubWebhookEvent {
+  Repository repository;
+
+  @JsonProperty("pull_request")
+  PullRequest pullRequest;
+
+  // `.repository.full_name`
+  @Override
+  public String getFullRepoName(Event event, Map postedEvent) {
+    return Optional.of(this)
+        .map(GithubPullRequestEvent::getRepository)
+        .map(Repository::getFullName)
+        .orElse("");
+  }
+
+  // `.repository.owner.login`
+  @Override
+  public String getRepoProject(Event event, Map postedEvent) {
+    return Optional.of(this)
+        .map(GithubPullRequestEvent::getRepository)
+        .map(Repository::getOwner)
+        .map(RepositoryOwner::getLogin)
+        .orElse("");
+  }
+
+  // `.repository.name`
+  @Override
+  public String getSlug(Event event, Map postedEvent) {
+    return Optional.of(this)
+        .map(GithubPullRequestEvent::getRepository)
+        .map(Repository::getName)
+        .orElse("");
+  }
+
+  // `.pull_request.head.sha`
+  // TODO
+  @Override
+  public String getHash(Event event, Map postedEvent) {
+    return Optional.of(this)
+        .map(GithubPullRequestEvent::getPullRequest)
+        .map(PullRequest::getHead)
+        .map(PullRequestHead::getSha)
+        .orElse("");
+  }
+
+  // `.pull_request.head.ref`
+  // TODO
+  @Override
+  public String getBranch(Event event, Map postedEvent) {
+    // Replace on "" still returns "", which is fine
+    return Optional.of(this)
+        .map(GithubPullRequestEvent::getPullRequest)
+        .map(PullRequest::getHead)
+        .map(PullRequestHead::getRef)
+        .orElse("");
+  }
+
+  @Data
+  private static class Repository {
+    RepositoryOwner owner;
+    String name;
+
+    @JsonProperty("full_name")
+    String fullName;
+  }
+
+  @Data
+  private static class RepositoryOwner {
+    String login;
+  }
+
+  @Data
+  private static class PullRequest {
+    PullRequestHead head;
+  }
+
+  @Data
+  private static class PullRequestHead {
+    String ref;
+    String sha;
+  }
+}

echo-webhooks/src/main/groovy/com/netflix/spinnaker/echo/scm/github/GithubPushEvent.java

@@ -0,0 +1,85 @@
+/*
+ * Copyright 2019 Netflix, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.netflix.spinnaker.echo.scm.github;
+
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.netflix.spinnaker.echo.model.Event;
+import java.util.Map;
+import java.util.Optional;
+import lombok.Data;
+
+@Data
+public class GithubPushEvent implements GithubWebhookEvent {
+  Repository repository;
+  String after;
+  String ref;
+
+  // `.repository.full_name`
+  @Override
+  public String getFullRepoName(Event event, Map postedEvent) {
+    return Optional.of(this)
+        .map(GithubPushEvent::getRepository)
+        .map(Repository::getFullName)
+        .orElse("");
+  }
+
+  // `.repository.owner.name`
+  @Override
+  public String getRepoProject(Event event, Map postedEvent) {
+    return Optional.of(this)
+        .map(GithubPushEvent::getRepository)
+        .map(Repository::getOwner)
+        .map(RepositoryOwner::getName)
+        .orElse("");
+  }
+
+  // `.repository.name`
+  @Override
+  public String getSlug(Event event, Map postedEvent) {
+    return Optional.of(this)
+        .map(GithubPushEvent::getRepository)
+        .map(Repository::getName)
+        .orElse("");
+  }
+
+  // `.after`
+  @Override
+  public String getHash(Event event, Map postedEvent) {
+    return Optional.of(this).map(GithubPushEvent::getAfter).orElse("");
+  }
+
+  // `.ref`, remove `refs/heads/`
+  @Override
+  public String getBranch(Event event, Map postedEvent) {
+    // Replace on "" still returns "", which is fine
+    return Optional.of(this).map(GithubPushEvent::getRef).orElse("").replace("refs/heads/", "");
+  }
+
+  @Data
+  private static class Repository {
+    RepositoryOwner owner;
+    String name;
+
+    @JsonProperty("full_name")
+    String fullName;
+  }
+
+  @Data
+  private static class RepositoryOwner {
+    String name;
+  }
+}

echo-webhooks/src/main/groovy/com/netflix/spinnaker/echo/scm/github/GithubWebhookEvent.java

@@ -0,0 +1,32 @@
+/*
+ * Copyright 2019 Netflix, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.netflix.spinnaker.echo.scm.github;
+
+import com.netflix.spinnaker.echo.model.Event;
+import java.util.Map;
+
+public interface GithubWebhookEvent {
+  String getFullRepoName(Event event, Map postedEvent);
+
+  String getRepoProject(Event event, Map postedEvent);
+
+  String getSlug(Event event, Map postedEvent);
+
+  String getHash(Event event, Map postedEvent);
+
+  String getBranch(Event event, Map postedEvent);
+}

echo-webhooks/src/test/groovy/com/netflix/spinnaker/echo/controllers/WebhooksControllerSpec.groovy

@@ -364,7 +364,7 @@ class WebhooksControllerSpec extends Specification {
     response.eventId == event.eventId
   }
 
-  void 'handles Github Webhook Event'() {
+  void 'handles Github Webhook Push Event'() {
     def event
 
     given:
@@ -386,6 +386,51 @@ class WebhooksControllerSpec extends Specification {
             "owner": {
               "name": "Codertocat"
             }
+          },
+          "commits": [
+            {
+              "id": "0000000000000000000000000000000000000000"
+            }
+          ]
+        }
+        """, new HttpHeaders())
+
+    then:
+    1 * controller.propagator.processEvent(_) >> {
+      event = it[0]
+    }
+
+    event.content.hash == "0000000000000000000000000000000000000000"
+    event.content.repoProject == "Codertocat"
+    event.content.slug == "Hello-World"
+    event.content.branch == "simple-tag"
+  }
+
+  void 'handles Github PR Webhook Event'() {
+    def event
+
+    given:
+    WebhooksController controller = new WebhooksController(mapper: new ObjectMapper(), scmWebhookHandler: scmWebhookHandler)
+    controller.propagator = Mock(EventPropagator)
+    controller.artifactExtractor = Mock(ArtifactExtractor)
+    controller.artifactExtractor.extractArtifacts(_, _, _) >> []
+
+    when:
+    def response = controller.forwardEvent(
+      "git",
+      "github",
+      """{
+          "pull_request": {
+            "head": {
+              "ref": "simple-tag",
+              "sha": "0000000000000000000000000000000000000000"
+            }
+          },
+          "repository": {
+            "name": "Hello-World",
+            "owner": {
+              "login": "Codertocat"
+            }
           }
         }
         """, new HttpHeaders())