eddies / github-webhook-cloud-function Goto Github PK

Tests for httpsRequest

httpsRequest needs better test coverage.

Relevant Links

• https://jestjs.io/docs/en/mock-functions
• https://nodejs.org/docs/latest-v8.x/api/https.html#https_https_request_options_callback
• https://nodejs.org/docs/latest-v8.x/api/http.html#http_http_request_options_callback

EventEmitter Refactoring

Consider refactoring our so-called "handler" to actually be an event handler.

Relevant Links

• https://nodejs.org/docs/latest-v8.x/api/events.html

Memoize API calls to Trello

The API calls to Trello for getBoardId and getCustomFields should be memoized to avoid making more HTTP requests than necessary.

Trello Card Ids at end of branch names

We presently support branch names that lead with the trello card id immediately followed by a forward slash, e.g. nqPiDKmw/9-grand-canyon-national-park.

This is inconvenient in a day-to-day command-line workflow. It requires developers to remember the card id when switching branches, e.g. git checkout nqPiDKmw/9-grand-canyon-national-park. It would be preferable if the card id was at the end of the branch name, e.g. grand-canyon-national-park#nqPiDKmw (or with some other separator). This would allow developers to switch/checkout branches without having to remember the card id.

All of the logic for parsing the branch name for the card id is isolated to getShortLink

Add AWS Lambda Support

Might need to rename this project to something like github-webhook-serverless or serverless-github-webhook.

Relevant Links

• https://docs.aws.amazon.com/lambda/latest/dg/programming-model.html
• https://aws.amazon.com/blogs/compute/node-js-8-10-runtime-now-available-in-aws-lambda/

Flexible placement of Trello Card IDs

#20 changed the behavior of the cloud function to require the Trello card id at the end of the branch name, e.g. grand-canyon-national-park#nqPiDKmw.

The previous behavior expected the Trello card id at the beginning, e.g. nqPiDKmw/9-grand-canyon-national-park (which mapped neatly onto the latter part of the Trello card url).

Either style of branch naming should be allowed.

Insufficient request detail in log messages

Cloud Functions log console.log and console.error to Stackdriver. Presently, the only log messages in Stackdriver are:

• Function execution started
• Function execution took NN ms, finished with status code: NNN

which includes no detail about the client, request, etc.

github-webhook-cloud-function should output more log messages.

Relevant Links

• https://cloud.google.com/functions/docs/monitoring/logging

Description

When a branch is force pushed, the Trello comment should indicate that the push was forced, e.g.

josh force pushed ... instead of the current josh pushed ...

Update dev dependencies (2018-08)

Update eslint-config-airbnb (v13.1.0 introduces support for eslint 5) and other outdated devDependencies

Relevant Links

• https://github.com/airbnb/javascript/blob/master/packages/eslint-config-airbnb-base/CHANGELOG.md#1310--2018-08-13
• https://github.com/facebook/jest/blob/master/CHANGELOG.md#2350
• https://github.com/benmosher/eslint-plugin-import/blob/master/CHANGELOG.md#2140---2018-08-13

PR comment wipes out review status

Steps to Reproduce the Problem

1. Assign a reviewer to a PR
2. Verify that the associated Trello card's "review" field contains the assigned reviewer
3. Reviewer requests changes
4. Verify that the associated Trello card's "review" field is updated with the "changes requested" status (i.e. a red circle)
5. Make a comment to the PR
6. The associated Trello card's "review" field is now empty

Update dev dependencies (2019-02)

Update eslint, eslint-plugin-import and jest:

Package               Current  Latest
eslint                  5.4.0  5.14.0
eslint-plugin-import   2.14.0  2.16.0
jest                   23.5.0  24.1.0

PR Status with Custom Fields

Use Trello's Custom Fields to indicate PR status on a card.

Currently, github-webhook-cloud-function adds an attachment URL to a Trello card when a PR is opened. It would be great if the front of the card could indicate the PR status: open, closed or merged.

Labels could be used as an indicator, but by default, you only see the label color, so you would have to know what color meant what, even more problematic if you were already using the label color for something else.

A Custom Fields of type list, could have three options: open, closed and merged, and each can have a color assigned.

Relevant Links

• https://developers.trello.com/v1.0/reference#custom-fields
• https://developers.trello.com/docs/getting-started-custom-fields
• https://help.trello.com/article/1067-using-the-custom-fields-power-up

Maintain persistent connections

Per https://cloud.google.com/functions/docs/bestpractices/networking, we should

...maintain persistent connections in a function. Failure to do so can result in quickly exhausting connection quotas

and provides an example of using http.Agent({keepAlive: true}) to "maintain persistent connections instead of creating a new connection upon every function invocation".

Relevant Links

• https://cloud.google.com/functions/docs/bestpractices/networking
• googleapis/nodejs-bigquery#41 (comment)

(I'm not sure if the second link applies, in which case we may run into issues if we implement this)

HTTP 207 Multi-Status

pull_request events with action opened can result in multiple API calls to Trello, e.g.:

• POST an attachment URL
• GET boardId
• GET Custom Fields for a board
• POST Custom Field to a board
• PUT custom field to a card

If the Custom Fields Power-Up is not enabled for a board, the POST Custom Field will fail with a 403 Forbidden.

If the board has a Custom Field named 'PR' but is missing one of 'Open', 'Closed' or 'Merged' list options, the PUT will fail with a 500.

So, the attachment URL could succeed but one or more of the subsequent operations could fail. One way to handle this in the response is to return an HTTP 207 Multi-Status.

Alternatively, we could use a 200 with a better response body. Presently, however, we're returning 200s for events that we are not supporting (but doesn't represent an error), e.g. a pull_request event with action 'labeled'. Because we use a different status code (201) for events we do support, it's easy to distinguish the two when parsing logs on GCP and Webhook results from GitHub.

Relevant Links

• RFC 4918: Multi-Status Response
• Example of Successful PROPPATCH
• Argument for JSON response

Update @google-cloud/functions-emulator to 1.0.0-beta.5

Addresses CVE-2018-1002204

Relevant Links

• https://github.com/GoogleCloudPlatform/cloud-functions-emulator/blob/master/CHANGELOG.md#100-beta5---31-july-2018

Return 403 instead of 401 for X-Hub-Signature mis-match

Instead of returning a 401, we should return a 403.

Relevant Links

• https://tools.ietf.org/html/rfc7231#section-6.5.3
• https://stackoverflow.com/questions/3297048/403-forbidden-vs-401-unauthorized-http-responses