From 28aa21c3d570a2b6eb1757a06b1149e223479112 Mon Sep 17 00:00:00 2001 From: Brandon Keepers Date: Sun, 15 Jul 2018 10:24:40 -0500 Subject: [PATCH 1/6] Move webhook type declarations --- src/@types/@octokit/webhooks/index.d.ts | 67 +++++++++++++++++++++++++ src/serializers.ts | 6 +-- test/serializers.test.js | 4 +- 3 files changed, 72 insertions(+), 5 deletions(-) create mode 100644 src/@types/@octokit/webhooks/index.d.ts diff --git a/src/@types/@octokit/webhooks/index.d.ts b/src/@types/@octokit/webhooks/index.d.ts new file mode 100644 index 0000000..beec1dd --- /dev/null +++ b/src/@types/@octokit/webhooks/index.d.ts @@ -0,0 +1,67 @@ +declare module '@octokit/webhooks' { + import { Application } from 'express' + + interface Options { + path: string, + secret: string + } + + declare class Webhooks { + public middleware: Application + + constructor (Options) + + public on (event: string, callback: (event: WebhookEvent | Error) => void) + } + + declare namespace Webhooks { + export interface WebhookEvent { + id: string + name: string + payload: WebhookPayloadWithRepository + protocol?: 'http' | 'https' + host?: string + url?: string + } + + export interface PayloadRepository { + [key: string]: any + full_name: string + name: string + owner: { + [key: string]: any + login: string + name: string + } + html_url: string + } + + export interface WebhookPayloadWithRepository { + [key: string]: any + repository?: PayloadRepository + issue?: { + [key: string]: any + number: number + html_url: string + body: string + } + pull_request?: { + [key: string]: any + number: number + html_url: string + body: string + } + sender?: { + [key: string]: any + type: string + } + action?: string + installation?: { + id: number + [key: string]: any + } + } + } + + export = Webhooks +} diff --git a/src/serializers.ts b/src/serializers.ts index e0d9443..0474e6f 100644 --- a/src/serializers.ts +++ b/src/serializers.ts @@ -1,14 +1,14 @@ +import { PayloadRepository, WebhookEvent } from '@octokit/webhooks' import bunyan from 'bunyan' import express from 'express' -import { PayloadRepository } from './context' export const serializers: bunyan.StdSerializers = { - event: (event: any) => { + event: (event: WebhookEvent | any) => { if (typeof event !== 'object' || !event.payload) { return event } else { - let name = event.event + let name = event.name if (event.payload && event.payload.action) { name = `${name}.${event.payload.action}` } diff --git a/test/serializers.test.js b/test/serializers.test.js index 93797da..428df29 100644 --- a/test/serializers.test.js +++ b/test/serializers.test.js @@ -23,7 +23,7 @@ describe('serializers', () => { describe('event', () => { it('works with a legit event', () => { const event = {id: 1, - event: 'test', + name: 'test', payload: { action: 'test', repository: {full_name: 'probot/test'}, @@ -40,7 +40,7 @@ describe('serializers', () => { it('works a malformed event', () => { const event = {id: 1, - event: 'test', + name: 'test', payload: {}} expect(serializers.event(event)).toEqual({ id: 1, From 565ad9a2460c45ef8fbc5eed23a4dddd1a61a874 Mon Sep 17 00:00:00 2001 From: Brandon Keepers Date: Sun, 15 Jul 2018 11:06:51 -0500 Subject: [PATCH 2/6] Make most webhook payload properties optional for now Until we have a way to generate test fixtures, it's too much of a hassle to require all these properties. --- src/@types/@octokit/webhooks/index.d.ts | 14 +++--- src/application.ts | 23 ++++++--- src/context.ts | 67 +++++-------------------- src/index.ts | 10 ++-- test/index.test.js | 2 +- 5 files changed, 40 insertions(+), 76 deletions(-) diff --git a/src/@types/@octokit/webhooks/index.d.ts b/src/@types/@octokit/webhooks/index.d.ts index beec1dd..7493237 100644 --- a/src/@types/@octokit/webhooks/index.d.ts +++ b/src/@types/@octokit/webhooks/index.d.ts @@ -26,14 +26,14 @@ declare module '@octokit/webhooks' { export interface PayloadRepository { [key: string]: any - full_name: string + full_name?: string name: string owner: { [key: string]: any login: string - name: string + name?: string } - html_url: string + html_url?: string } export interface WebhookPayloadWithRepository { @@ -42,14 +42,14 @@ declare module '@octokit/webhooks' { issue?: { [key: string]: any number: number - html_url: string - body: string + html_url?: string + body?: string } pull_request?: { [key: string]: any number: number - html_url: string - body: string + html_url?: string + body?: string } sender?: { [key: string]: any diff --git a/src/application.ts b/src/application.ts index 858d179..24733e6 100644 --- a/src/application.ts +++ b/src/application.ts @@ -1,15 +1,16 @@ +import { WebhookEvent } from '@octokit/webhooks' import express from 'express' import { EventEmitter } from 'promise-events' import { ApplicationFunction } from '.' -import { Context, WebhookEvent } from './context' +import { Context } from './context' import { GitHubAPI } from './github' import { logger } from './logger' import { LoggerWithTarget, wrapLogger } from './wrap-logger' // Some events can't get an authenticated client (#382): -function isUnauthenticatedEvent (context: Context) { - return !context.payload.installation || - (context.event === 'installation' && context.payload.action === 'deleted') +function isUnauthenticatedEvent (event: WebhookEvent) { + return !event.payload.installation || + (event.name === 'installation' && event.payload.action === 'deleted') } /** @@ -50,10 +51,16 @@ export class Application { } public async receive (event: WebhookEvent) { + if ((event as any).event) { + // tslint:disable-next-line:no-console + console.warn(new Error('Propery `event` is deprecated, use `name`')) + event = { name: (event as any).event, ...event } + } + return Promise.all([ this.events.emit('*', event), - this.events.emit(event.event, event), - this.events.emit(`${event.event}.${event.payload.action}`, event) + this.events.emit(event.name, event), + this.events.emit(`${ event.name }.${ event.payload.action }`, event) ]) } @@ -119,7 +126,7 @@ export class Application { public on (eventName: string | string[], callback: (context: Context) => Promise) { if (typeof eventName === 'string') { - return this.events.on(eventName, async (event: Context) => { + return this.events.on(eventName, async (event: WebhookEvent) => { const log = this.log.child({ name: 'event', id: event.id }) try { @@ -129,7 +136,7 @@ export class Application { github = await this.auth() log.debug('`context.github` is unauthenticated. See https://probot.github.io/docs/github-api/#unauthenticated-events') } else { - github = await this.auth(event.payload.installation.id, log) + github = await this.auth(event.payload.installation!.id, log) } const context = new Context(event, github, log) diff --git a/src/context.ts b/src/context.ts index 0b77105..56da229 100644 --- a/src/context.ts +++ b/src/context.ts @@ -1,3 +1,4 @@ +import { WebhookEvent, WebhookPayloadWithRepository } from '@octokit/webhooks' import yaml from 'js-yaml' import path from 'path' import { GitHubAPI } from './github' @@ -19,28 +20,19 @@ import { LoggerWithTarget } from './wrap-logger' * @property {logger} log - A logger */ -export interface WebhookEvent { - event: string - id: string - payload: WebhookPayloadWithRepository - protocol: 'http' | 'https' - host: string - url: string -} - export class Context implements WebhookEvent { - public event: string + public name: string public id: string public payload: WebhookPayloadWithRepository - public protocol: 'http' | 'https' - public host: string - public url: string + public protocol?: 'http' | 'https' + public host?: string + public url?: string public github: GitHubAPI public log: LoggerWithTarget constructor (event: WebhookEvent, github: GitHubAPI, log: LoggerWithTarget) { - this.event = event.event + this.name = event.name this.id = event.id this.payload = event.payload this.protocol = event.protocol @@ -51,6 +43,11 @@ export class Context implements WebhookEvent { this.log = log } + // Maintain backward compatability + public get event (): string { + return this.name + } + /** * Return the `owner` and `repo` params for making API requests against a * repository. @@ -71,7 +68,7 @@ export class Context implements WebhookEvent { } return Object.assign({ - owner: repo.owner.login || repo.owner.name, + owner: repo.owner.login || repo.owner.name!, repo: repo.name }, object) } @@ -100,7 +97,7 @@ export class Context implements WebhookEvent { * @type {boolean} */ get isBot () { - return this.payload.sender.type === 'Bot' + return this.payload.sender!.type === 'Bot' } /** @@ -161,41 +158,3 @@ export class Context implements WebhookEvent { } } } - -export interface PayloadRepository { - [key: string]: any - full_name: string - name: string - owner: { - [key: string]: any - login: string - name: string - } - html_url: string -} - -export interface WebhookPayloadWithRepository { - [key: string]: any - repository: PayloadRepository - issue: { - [key: string]: any - number: number - html_url: string - body: string - } - pull_request: { - [key: string]: any - number: number - html_url: string - body: string - } - sender: { - [key: string]: any - type: string - } - action: string - installation: { - id: number - [key: string]: any - } -} diff --git a/src/index.ts b/src/index.ts index bcc16f5..55ffd43 100644 --- a/src/index.ts +++ b/src/index.ts @@ -1,8 +1,9 @@ +import { WebhookEvent } from '@octokit/webhooks' import Logger from 'bunyan' import cacheManager from 'cache-manager' import express from 'express' import { Application } from './application' -import { Context, WebhookEvent } from './context' +import { Context } from './context' import { createApp } from './github-app' import { logger } from './logger' import { resolve } from './resolver' @@ -46,11 +47,8 @@ export class Probot { this.server = createServer({ webhook: this.webhook.middleware, logger }) // Log all received webhooks - this.webhook.on('*', (event: any) => { - const webhookEvent = { ...event, event: event.name } - delete webhookEvent.name - - return this.receive(webhookEvent) + this.webhook.on('*', (event: WebhookEvent) => { + return this.receive(event) }) // Log all webhook errors diff --git a/test/index.test.js b/test/index.test.js index 4e63963..d540e55 100644 --- a/test/index.test.js +++ b/test/index.test.js @@ -22,7 +22,7 @@ describe('Probot', () => { const app = probot.load(() => {}) app.receive = jest.fn() await probot.webhook.receive(event) - expect(app.receive).toHaveBeenCalledWith({ event: event.name, payload: event.payload }) + expect(app.receive).toHaveBeenCalledWith(event) }) it('responds with the correct error if webhook secret does not match', async () => { From 25694becc0b4a9842fbefdb6a62367d193bdd21a Mon Sep 17 00:00:00 2001 From: Brandon Keepers Date: Sun, 15 Jul 2018 11:10:02 -0500 Subject: [PATCH 3/6] Fix deprecation warnings --- bin/probot-simulate.js | 2 +- test/application.test.ts | 18 ++++++++++-------- test/context.test.ts | 11 +++++++++-- test/index.test.js | 1 - 4 files changed, 20 insertions(+), 12 deletions(-) diff --git a/bin/probot-simulate.js b/bin/probot-simulate.js index 337eb8f..4c43d65 100755 --- a/bin/probot-simulate.js +++ b/bin/probot-simulate.js @@ -32,4 +32,4 @@ const probot = createProbot({ probot.setup(program.args.slice(2)) probot.logger.debug('Simulating event', eventName) -probot.receive({event: eventName, payload}) +probot.receive({name: eventName, payload}) diff --git a/test/application.test.ts b/test/application.test.ts index f9af149..6b19746 100644 --- a/test/application.test.ts +++ b/test/application.test.ts @@ -1,10 +1,11 @@ + import { WebhookEvent } from '@octokit/webhooks' import { Application } from '../src/application' import { Context } from '../src/context' import { logger } from '../src/logger' describe('Application', () => { let app: Application - let event: any + let event: WebhookEvent let output: any beforeAll(() => { @@ -25,8 +26,8 @@ describe('Application', () => { app.auth = jest.fn().mockReturnValue({}) event = { - event: 'test', id: '123-456', + name: 'test', payload: { action: 'foo', installation: { id: 1 } @@ -71,13 +72,14 @@ describe('Application', () => { }) it('calls callback x amount of times when an array of x actions is passed', async () => { - const event2 = { - event: 'arrayTest', + const event2: WebhookEvent = { + id: '123', + name: 'arrayTest', payload: { action: 'bar', installation: { id: 2 } } - } as any + } const spy = jest.fn() app.on(['test.foo', 'arrayTest.bar'], spy) @@ -105,8 +107,8 @@ describe('Application', () => { it('returns an authenticated client for installation.created', async () => { event = { - event: 'installation', id: '123-456', + name: 'installation', payload: { action: 'created', installation: { id: 1 } @@ -124,8 +126,8 @@ describe('Application', () => { it('returns an unauthenticated client for installation.deleted', async () => { event = { - event: 'installation', id: '123-456', + name: 'installation', payload: { action: 'deleted', installation: { id: 1 } @@ -143,8 +145,8 @@ describe('Application', () => { it('returns an authenticated client for events without an installation', async () => { event = { - event: 'foobar', id: '123-456', + name: 'foobar', payload: { /* no installation */ } } diff --git a/test/context.test.ts b/test/context.test.ts index 783f7d7..2b11751 100644 --- a/test/context.test.ts +++ b/test/context.test.ts @@ -1,16 +1,18 @@ import fs = require('fs') import path = require('path') +import { WebhookEvent } from '@octokit/webhooks' import { Context } from '../src/context' import { GitHubAPI, OctokitError } from '../src/github' describe('Context', () => { - let event: any + let event: WebhookEvent let context: Context beforeEach(() => { event = { - event: 'push', + id: '123', + name: 'push', payload: { issue: { number: 4 }, repository: { @@ -27,6 +29,11 @@ describe('Context', () => { expect(context.payload).toBe(event.payload) }) + it('aliases the event name', () => { + expect(context.name).toEqual('push') + expect(context.event).toEqual('push') + }) + describe('repo', () => { it('returns attributes from repository payload', () => { expect(context.repo()).toEqual({ owner: 'bkeepers', repo: 'probot' }) diff --git a/test/index.test.js b/test/index.test.js index d540e55..ecd5315 100644 --- a/test/index.test.js +++ b/test/index.test.js @@ -12,7 +12,6 @@ describe('Probot', () => { event = { name: 'push', - event: 'push', payload: require('./fixtures/webhook/push') } }) From bdc90cd040b4836b194e48b9bc2806b7c7a5916b Mon Sep 17 00:00:00 2001 From: Brandon Keepers Date: Sun, 15 Jul 2018 11:20:55 -0500 Subject: [PATCH 4/6] Add strong typing for exported webhooks property --- src/index.ts | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/index.ts b/src/index.ts index 55ffd43..6fbff18 100644 --- a/src/index.ts +++ b/src/index.ts @@ -1,4 +1,4 @@ -import { WebhookEvent } from '@octokit/webhooks' +import Webhooks, { WebhookEvent } from '@octokit/webhooks' import Logger from 'bunyan' import cacheManager from 'cache-manager' import express from 'express' @@ -12,7 +12,6 @@ import { createWebhookProxy } from './webhook-proxy' // tslint:disable:no-var-requires // These needs types -const Webhooks = require('@octokit/webhooks') const logRequestErrors = require('./middleware/log-request-errors') const cache = cacheManager.caching({ @@ -29,7 +28,7 @@ const defaultApps: ApplicationFunction[] = [ export class Probot { public server: express.Application - public webhook: any + public webhook: Webhooks public logger: Logger private options: Options From 766d2d175717fe3daf2f2b6ab907f0c9064dec42 Mon Sep 17 00:00:00 2001 From: Brandon Keepers Date: Sun, 15 Jul 2018 11:40:30 -0500 Subject: [PATCH 5/6] Add test for deprecated receive behavior --- test/application.test.ts | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/test/application.test.ts b/test/application.test.ts index 6b19746..74addf2 100644 --- a/test/application.test.ts +++ b/test/application.test.ts @@ -262,4 +262,20 @@ describe('Application', () => { expect(output[0].event.id).toEqual(event.id) }) }) + + describe('deprecations', () => { + test('recieve() accepts param with {event}', async () => { + const spy = jest.fn() + app.events.on('deprecated', spy) + await app.receive({ event: 'deprecated', payload: { action: 'test' } } as any) + expect(spy).toHaveBeenCalled() + }) + + test('recieve() accepts param with {name,event}', async () => { + const spy = jest.fn() + app.events.on('real-event-name', spy) + await app.receive({ name: 'real-event-name', event: 'deprecated', payload: { action: 'test' } } as any) + expect(spy).toHaveBeenCalled() + }) + }) }) From 5085f45a76bbeff25459abeb7c3b673e19a724e3 Mon Sep 17 00:00:00 2001 From: Brandon Keepers Date: Sun, 15 Jul 2018 11:45:33 -0500 Subject: [PATCH 6/6] Test for context.isBot --- test/context.test.ts | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/test/context.test.ts b/test/context.test.ts index 2b11751..1b7b259 100644 --- a/test/context.test.ts +++ b/test/context.test.ts @@ -221,4 +221,20 @@ describe('Context', () => { }) }) }) + + describe('isBot', () => { + test('returns true if sender is a bot', () => { + event.payload.sender = { type: 'Bot' } + context = new Context(event, {} as any, {} as any) + + expect(context.isBot).toBe(true) + }) + + test('returns false if sender is not a bot', () => { + event.payload.sender = { type: 'User' } + context = new Context(event, {} as any, {} as any) + + expect(context.isBot).toBe(false) + }) + }) })