Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add multiple support #22

Closed
wants to merge 1 commit into from

Conversation

excaliburhan
Copy link
Contributor

@excaliburhan excaliburhan commented Jan 18, 2017

multiple handlers

I need multiple handlers for different repos and I hope those handlers have different secret for each repo. And those handlers only take one port.

So I make options.secret to support both string and object. If options.secret is an object, the key of the object will be appId, and the value of the object will be appSecret. At last, the EmitData will return the appId for use.


Here's the example for multiple handlers:

// handler settings
var handler = createHandler({
  path: '/app',
  secret: {
    app1: 'secret1',
    app2: 'secret2'
  }
})
// server
http.createServer(function (req, res) {
  handler(req, res, function (err) {
    res.statusCode = 404
    res.end('no such location')
  })
}).listen(7777)
// events
handler.on('push', function (event) {
  var appId = event.appId
  switch (appId) {
    case 'app1':
      // do sth about app1
      break
    case 'app2':
      // do sth about app2
      break
    default:
      break
  }
})

And in github Webhooks Payload URL:

app1

http://example.com/app?id=app1

app2

http://example.com/app?id=app2

If options.secret is a string, everything remains the same.

@rjz rjz self-assigned this Jan 18, 2017
@rjz
Copy link
Collaborator

rjz commented Jan 18, 2017

Thanks, @excaliburhan! This PR covers a fairly specific scenario as written, but I wonder if we could expand it to accommodate the scenarios in #16 ("add additional fields to emitData") and #21 ("enable multiple handlers").

If we allowed consumers to provide their own validation/parsing logic--or even separated webhook processing from the HTTP server itself--we could preserve the existing (simple) default but open the door for more customization downstream.

Thoughts?

@excaliburhan
Copy link
Contributor Author

excaliburhan commented Jan 19, 2017

OK, I realize this PR is actually 'One handler deals with multiple webhooks'.

Solution A: expand
Support multiple path && secret to enable real multiple handlers, like this:

var createHandler = require('github-webhook-handler')
var handler = createHandler([
{ path: '/app1', secret: 'secret1' },
{ path: '/app2', secret: 'secret2' }
])

this solution means we have to rewrite the handler function in github-webhook-handler.js


Solution B: process over the HTTP server

var handlerA = createHandler({ path: '/app1', secret: 'secret1' })
var handlerB = createHandler({ path: '/app2', secret: 'secret2' })

http.createServer(function (req, res) {
  // process logic
  switch (req.url) {
    case '/app1':
      handlerA(req, res, function (err) {
        res.statusCode = 404
        res.end('no such location')
      })
      break
    case '/app2':
      handlerB(req, res, function (err) {
        res.statusCode = 404
        res.end('no such location')
      })
      break
    default:
      break
  }
}).listen(7777)

// events
handlerA.on('issues', function(event) {
    // do sth
})
handlerB.on('issues', function(event) {
    // do sth
})

this solution contains consumers' own process logic.


I prefer the Solution A, it may make the github-webhook-handler.js a little complicated, but for consumers, it's more convenient.

For Solution B, what makes me unhappy is I have to write many nearly same code to keep different handlers work.

I'd like to help with Solution A.

@rjz what do you think about this?

@rjz
Copy link
Collaborator

rjz commented Jan 19, 2017

It seems like we could write A in terms of B, something like:

function multiHook(handlerOpts) { // it's a silly name :^)
  var handlers = handlerOpts.reduce(function (hs, opts) {
    hs[opts.path] = createHandler(opts);
    return hs;
  }, {});

  return http.createServer(function (req, res) {
    var handler = handlers[req.url];
    handler(req, res, function (err) {
      res.statusCode = 404
      res.end('no such location')
    });
  });
}

If that's reasonable, maybe it's appropriate to leave this handler focused on a single hook but either:

  • add an example for the multi-app case here
  • create a new package (ala github-webhook) tailored to the multi-app case

@excaliburhan
Copy link
Contributor Author

Cool.
I think we could close this PR now:)

@excaliburhan
Copy link
Contributor Author

One more thing, in github webhooks settings, the content-type must be application/json rather than application/x-www-form-urlencoded.

I think this tip should be written in the READEME.md.

Just a suggestion.

@rjz
Copy link
Collaborator

rjz commented Jan 19, 2017

Done, and big thanks for your suggestions on this, @excaliburhan!

  • README.md: great point, will add (or be happy to merge a PR)
  • Also, if you're up to add an example to this repo or to link up an external package, it would be awesome to have a reference for other folks with this issue

@rjz rjz closed this Jan 19, 2017
@excaliburhan
Copy link
Contributor Author

excaliburhan commented Jan 22, 2017

ok, here's the multi handlers complete example(have tested on my server), and an external package node-github-webhook.

example for github-webhook-handler

function generaterHandler(handlerOpts) {
  var handlers = handlerOpts.reduce(function(hs, opts) {
    hs[opts.path] = createHandler(opts)
    return hs
  }, {})

  return http.createServer(function(req, res) {
    var handler = handlers[req.url]
    handler(req, res, function(err) {
      res.statusCode = 404
      res.end('no such location')
    })
  }).listen(7777)
}

var http = require('http')
var createHandler = require('github-webhook-handler')
var handlerOpts = [{
  path: '/app1',
  secret: 'secret1'
}, {
  path: '/app2',
  secret: 'secret2'
}]
var handler = generaterHandler(handlerOpts)

handler.on('error', function(err) {
  console.error('Error:', err.message)
})
handler.on('push', function (event) {
  var url = event.url
  switch (url) {
    // be careful if you use query to distinguish your app, url contains the querys, otherwise, it is equal to the path
    case '/app1':
      // do sth for app1
      break
    case '/app2':
      // do sth for app2
      break
    default:
      break
})

example for node-github-webhook

var http = require('http')
var createHandler = require('node-github-webhook')
var handler = createHandler([{
    path: '/app1',
    secret: 'secret1'
  },
  {
    path: '/app2',
    secret: 'secret2'
  }
])

http.createServer(function (req, res) {
  handler(req, res, function (err) {
    res.statusCode = 404
    res.end('no such location')
  })
}).listen(7777)

// handler
handler.on('error', function (err) {
  console.error('Error:', err.message)
})
handler.on('push', function (event) {
  var path = event.path
  switch (path) {
    case '/app1':
      // do sth for app1
      break
    case '/app2':
      // do sth for app2
      break
    default:
      break
  }
})

@rjz rjz mentioned this pull request Feb 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants