feat(status-event): enabled processing to be triggered by a status-event

hoping that use of the status-event can replace the use of the pull_request-event, but this enables the choice for now. the intent is that one or the other should be used, not both. this is to prevent collisions between polling from the pull_request-event and the more intentional status-event processing. switching to the status-event eliminates polling, which allows acceptance to be triggered after what would have been beyond the polling timeout. hopefully this will also improve the behavior related to #77 because a successful status would need to be reported after the PR is opened. ideally this gives enough time for all checks to be registered for #21
greenkeeper-keeper · Mar 30, 2017 · 8c25ebb · 8c25ebb
1 parent be551d8
commit 8c25ebb
Show file tree

Hide file tree

Showing 7 changed files with 61 additions and 35 deletions.
diff --git a/src/handler.js b/src/handler.js
@@ -31,7 +31,7 @@ export default function (request, reply, settings) {
   if (isValidGreenkeeperUpdate({event, action, sender})) {
     reply('ok').code(ACCEPTED);
 
-    return process(request, {...settings, pollWhenPending: true});
+    return process(request, request.payload.pull_request, {...settings, pollWhenPending: true});
   }
 
   if (successfulStatusCouldBeForGreenkeeperPR(event, state, branches)) {
@@ -41,8 +41,10 @@ export default function (request, reply, settings) {
       .then(pullRequests => {
         if (!pullRequests.length) reply('no PRs for this commit').code(BAD_REQUEST);
         else if (pullRequests.length > 1) reply(boom.internal('too many PRs exist for this commit'));
-        else if (openedByGreenkeeperBot(pullRequests[0].user.html_url)) reply('ok').code(ACCEPTED);
-        else reply('PR is not from greenkeeper').code(BAD_REQUEST);
+        else if (openedByGreenkeeperBot(pullRequests[0].user.html_url)) {
+          reply('ok').code(ACCEPTED);
+          process(request, pullRequests[0], settings);
+        } else reply('PR is not from greenkeeper').code(BAD_REQUEST);
       })
       .catch(e => reply(boom.internal('failed to fetch PRs', e)));
   }

diff --git a/src/process.js b/src/process.js
@@ -1,21 +1,24 @@
 import createActions from './github/actions';
 
-export default function (request, {github, squash, deleteBranches = false, pollWhenPending}) {
-  const {pull_request, number} = request.payload;
+export default function (
+  request,
+  {head, url, number, comments_url},
+  {github, squash, deleteBranches = false, pollWhenPending}
+) {
   const {ensureAcceptability, acceptPR, deleteBranch, postErrorComment} = createActions(github);
 
   return ensureAcceptability({
-    repo: pull_request.head.repo,
-    ref: pull_request.head.ref,
-    url: pull_request.url,
+    repo: head.repo,
+    ref: head.ref,
+    url,
     pollWhenPending
   }, message => request.log(message))
-    .then(() => acceptPR(pull_request.url, pull_request.head.sha, number, squash, message => request.log(message)))
-    .then(() => deleteBranch(pull_request.head, deleteBranches))
+    .then(() => acceptPR(url, head.sha, number, squash, message => request.log(message)))
+    .then(() => deleteBranch(head, deleteBranches))
     .catch(err => {
       request.log(['error', 'PR'], err);
 
-      return postErrorComment(pull_request.comments_url, err)
+      return postErrorComment(comments_url, err)
         .catch(e => request.log(`failed to log comment against the PR: ${e}`));
     });
 }
diff --git a/test/integration/features/status-webhook.feature b/test/integration/features/status-webhook.feature
@@ -1,11 +1,10 @@
 Feature: Commit status event webhook
 
-  @wip
   Scenario: Success status-event for head commit of greenkeeper PR
     Given the webhook is for a status event and a "success" state
     And the commit is only on one, non-master branch
-    And an open PR exists for the commit
     And the PR was submitted by the greenkeeper integration
+    And an open PR exists for the commit
     And statuses exist for the PR
     And the PR can be merged
     When the webhook is received

diff --git a/test/integration/features/step_definitions/github-api.js b/test/integration/features/step_definitions/github-api.js
@@ -19,7 +19,7 @@ function stubTheCommentsEndpoint(githubScope, authorizationHeader) {
   });
 }
 
-defineSupportCode(({Before, After, Given, Then, setWorldConstructor}) => {
+defineSupportCode(({Before, After, Given, setWorldConstructor}) => {
   setWorldConstructor(World);
 
   let githubScope, authorizationHeader;
@@ -44,8 +44,16 @@ defineSupportCode(({Before, After, Given, Then, setWorldConstructor}) => {
       .matchHeader('Authorization', authorizationHeader)
       .get(`/repos/${this.repo}/pulls?head=${this.repoOwner}:${this.commitBranches[0]}`)
       .reply(OK, [{
+        url: 'https://api.github.com/123',
         user: {html_url: this.prSender || any.url()},
-        number: this.prNumber
+        number: this.prNumber,
+        head: {
+          sha: this.sha,
+          ref: this.ref,
+          repo: {
+            full_name: this.repo
+          }
+        }
       }]);
 
     callback();
@@ -72,7 +80,8 @@ defineSupportCode(({Before, After, Given, Then, setWorldConstructor}) => {
   });
 
   Given(/^the PR can be merged$/, function (callback) {
-    githubScope
+    this.prProcessed = new Promise(resolve => {
+      githubScope
       .matchHeader('Authorization', authorizationHeader)
       .put('/123/merge', {
         sha: this.sha,
@@ -82,7 +91,9 @@ defineSupportCode(({Before, After, Given, Then, setWorldConstructor}) => {
       })
       .reply(OK, uri => {
         this.mergeUri = uri;
+        resolve();
       });
+    });
 
     callback();
   });
@@ -135,10 +146,4 @@ defineSupportCode(({Before, After, Given, Then, setWorldConstructor}) => {
 
     callback();
   });
-
-  Then(/^the PR is merged$/, function (callback) {
-    assert.equal(this.mergeUri, '/123/merge');
-
-    callback();
-  });
 });
diff --git a/test/integration/features/step_definitions/pr.js b/test/integration/features/step_definitions/pr.js
@@ -1,9 +1,10 @@
 import {defineSupportCode} from 'cucumber';
 import any from '@travi/any';
+import {assert} from 'chai';
 import {World} from '../support/world';
 import {GREENKEEPER_INTEGRATION_GITHUB_URL, GREENKEEPER_BOT_GITHUB_URL} from '../../../../src/greenkeeper';
 
-defineSupportCode(({Given, setWorldConstructor}) => {
+defineSupportCode(({Given, Then, setWorldConstructor}) => {
   setWorldConstructor(World);
 
   Given(/^the PR was submitted by the greenkeeper integration$/, function (callback) {
@@ -23,4 +24,10 @@ defineSupportCode(({Given, setWorldConstructor}) => {
 
     callback();
   });
+
+  Then(/^the PR is merged$/, function (callback) {
+    assert.equal(this.mergeUri, '/123/merge');
+
+    callback();
+  });
 });
diff --git a/test/unit/handler-test.js b/test/unit/handler-test.js
@@ -27,6 +27,7 @@ suite('handler', () => {
     sandbox.stub(greenkeeper, 'default')
       .returns(false)
       .withArgs(greenkeeperSender).returns(true);
+    sandbox.stub(process, 'default').resolves();
   });
 
   teardown(() => {
@@ -35,15 +36,15 @@ suite('handler', () => {
   });
 
   suite('`pull_request` event', () => {
-    setup(() => sandbox.stub(process, 'default').resolves());
-
     test('that response is accepted when pr was opened by greenkeeper and is then processed', () => {
+      const pullRequest = any.simpleObject();
       const request = {
         payload: {
           action: 'opened',
           sender: {
             html_url: greenkeeperSender
-          }
+          },
+          pull_request: pullRequest
         },
         headers: {'x-github-event': 'pull_request'},
         log: () => undefined
@@ -52,7 +53,7 @@ suite('handler', () => {
 
       return handler(request, reply, settings).then(() => {
         assert.calledWith(code, ACCEPTED);
-        assert.calledWith(process.default, request, {...settings, pollWhenPending: true});
+        assert.calledWith(process.default, request, pullRequest, {...settings, pollWhenPending: true});
       });
     });
 
@@ -104,9 +105,10 @@ suite('handler', () => {
       sandbox.stub(actionsFactory, 'default').withArgs(githubCredentials).returns({getPullRequestsForCommit});
     });
 
-    test('that the response is accepted when the event is a successful status and a matching greenkeeper PR', () => {
+    test('that the webhook is accepted and processed for a successful status and a matching greenkeeper PR', () => {
       const repository = any.simpleObject();
       const branch = any.string();
+      const pullRequest = {user: {html_url: greenkeeperSender}};
       const request = {
         payload: {
           state: 'success',
@@ -120,9 +122,12 @@ suite('handler', () => {
       getPullRequestsForCommit.withArgs({
         repo: repository,
         ref: branch
-      }).resolves([{user: {html_url: greenkeeperSender}}]);
+      }).resolves([pullRequest]);
 
-      return handler(request, reply, settings).then(() => assert.calledWith(code, ACCEPTED));
+      return handler(request, reply, settings).then(() => {
+        assert.calledWith(code, ACCEPTED);
+        assert.calledWith(process.default, request, pullRequest, settings);
+      });
     });
 
     test('that the response is bad-request when the state is not `success`', () => {

diff --git a/test/unit/process-test.js b/test/unit/process-test.js
@@ -42,7 +42,8 @@ suite('process', () => {
     deleteBranch.resolves();
 
     return processPR(
-      {payload: {number, pull_request: {url, head}}, log},
+      {log},
+      {url, head, number},
       {github: githubCredentials, squash, deleteBranches, pollWhenPending}
     ).then(() => {
       const message = any.string();
@@ -65,7 +66,8 @@ suite('process', () => {
     deleteBranch.resolves();
 
     return processPR(
-      {payload: {number, pull_request: {url, head}}},
+      {},
+      {url, head},
       {github: githubCredentials, squash}
     ).then(() => {
       assert.calledOnce(ensureAcceptability);
@@ -80,7 +82,8 @@ suite('process', () => {
     postErrorComment.resolves(error);
 
     return processPR(
-      {payload: {number, pull_request: {comments_url: url, head}}, log: () => undefined},
+      {log: () => undefined},
+      {comments_url: url, head},
       {github: githubCredentials, squash, deleteBranches}
     ).then(() => {
       assert.notCalled(acceptPR);
@@ -96,7 +99,8 @@ suite('process', () => {
     postErrorComment.resolves(error);
 
     return processPR(
-      {payload: {number, pull_request: {comments_url: url, head}}, log: () => undefined},
+      {log: () => undefined},
+      {comments_url: url, head},
       {github: githubCredentials, squash, deleteBranches}
     ).then(() => {
       assert.notCalled(deleteBranch);
@@ -110,7 +114,8 @@ suite('process', () => {
     postErrorComment.rejects(error);
 
     return processPR(
-      {payload: {number, pull_request: {url, head}}, log: () => undefined},
+      {log: () => undefined},
+      {url, head},
       {github: githubCredentials, squash, deleteBranches}
     ).then(() => {
       assert.calledOnce(postErrorComment);