1324807 - templating system for injecting arbitrary webhook payload

Status: RESOLVED FIXED

(Taskcluster :: Services, defect)

Description

[HAMMAD AKHTAR]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_1) AppleWebKit/602.2.14 (KHTML, like Gecko) Version/10.0.1 Safari/602.2.14

Comment 1

[HAMMAD AKHTAR]

taskcluster::hooks may get triggered by users using different type of services. So we need a tempelating system for converting the request payload into a task definition.

Let's say payload looks like following

>{
>  name: "Jim",
>  id: "123",
>  token: "abc",
>  tasks: ["enter", "exit"]
>}

A parameterized task defintion may look like following

>{
>  username: "{{payload.name}}",
>  clientId: "{{payload.id}}",
>  accessToken: "{{payload.token}}",
>  task: "payload.tasks.0"
>}

both the payload and parameterized task-definition will be passed into the tempelating engine and it will spit out following output for the above case

>{
>  username: "Jim",
>  clientId: "123",
>  accessToken: "abc",
>  task: "enter"
>}


Potential services which may provide the payload: 
http://docs.aws.amazon.com/sns/latest/dg/SendMessageToHttp.html
https://webhooks.pbworks.com/w/page/13385124/FrontPage
http://help.papertrailapp.com/kb/how-it-works/web-hooks/
https://zapier.com/zapbook/webhook/
http://etherpad.mozilla.org/

Task-definition has following structure
https://docs.taskcluster.net/reference/platform/queue/task

Comment 2

[Dustin J. Mitchell [:dustin] (he/him)]

This is a good start, but the task definitions in the previous comment don't match the structure you link to on the last line.

Comment 3

[HAMMAD AKHTAR]

Ohh, silly mistake. I just wanted to provide an example for, what a tempelating engine will do. Example is totally unrelated to task-definition structure for which I have provided link in the last line of comment1.

Comment 4

[HAMMAD AKHTAR]

@Dustin

I have created a basic template engine for starters. Have a look at it here https://github.com/taskcluster/taskcluster-hooks/pull/29

Comment 5

[HAMMAD AKHTAR]

@Dustin

I have implemented property access, array index access, function evaluation for parametrization. Work is inspired from jonas's json-parametrization project.

Examples
1) array index access https://github.com/hammad13060/json-e/blob/master/tests.js#L37
2) function evaluation https://github.com/hammad13060/json-e/blob/master/tests.js#L22
3) propert access https://github.com/hammad13060/json-e/blob/master/tests.js#L37 (hint: name property)

I will add support for $if, $switch and $eval.

Comment 6

[Dustin J. Mitchell [:dustin] (he/him)]

Very cool!  A few minor things:

 * Please write a README explaining how to use the library
 * `npm test` should run the tests
 * Is there a reason to make this a class?  Why not just a function?
 * I think you will need more in package.json to make babel work.  Consider using https://www.npmjs.com/package/babel-compile?

I'd like to get Jonas's feedback on the details of the implementation.  He's away for a bit for the holidays though.

Comment 7

[HAMMAD AKHTAR]

@dustin and @jonas

check this out now https://github.com/hammad13060/json-e

Comment 8

[Dustin J. Mitchell [:dustin] (he/him)]

This looks great!  I think Jonas isn't back yet, so I'll still rely on him to review the actual parameterization here.

A few notes, though, as a potential user of this tool: 

var par = Parameterize(template1, context1); //constructor 
par.setNewTemplate(template2); //replaces template1 with template2 
par.setNewContext(context2); //replaces context1 with context2 
par.getTemplate(); //returns current template 
par.getContext(); //returns current context 
par.render(); //renders template using context 

that's a lot of work and methods for a class that really does one thing (combine a JSON template and a context to produce a JSON object).  In particular, I think it's very confusing that the result is available from `par.getContext()` after `par.render()` is called.  Why not just

result = parameterize(template, context)?

with no class involved?

Also, could array indexing use `[..]` instead of `(..)`?  That's not too important if it's hard, but I bet I'll forget it's `(..)` every time I try to use it :)

Comment 9

[Jonas Finnemann Jensen (:jonasfj)]

@hammad,
I had lots of comments, but they died with my browser :(
Please file a PR against an empty branch, you might have to rewrite history to do that, but then it's a lot
easier to discuss each case on it's own.

Notably:
  I don't think we need both {{...}} and ${...}.
  I would stick to ${...} and say that if the expression covers the full string, then the value of the
  expression replaces the string, otherwise it's string interpolation as expected. Granted making it
  always string interpolation might be more sane and predictable.

  As for {$eval: ''}, $if and $switch, I see no need to require the ${...} wrapper as those strings always
  have to be expressions, it would never make sense to do string interpolation for those.

Also:
Rather than babel-compile consider using neutrino with taskcluster preset for node. Ask Eli on irc, if
you have any problems with it. I don't think we're using it for many libs yet, but it gets you everything
pretty easily.

Comment 10

[Dustin J. Mitchell [:dustin] (he/him)]

https://github.com/taskcluster/json-e/issues/116

Comment 11

[Dustin J. Mitchell [:dustin] (he/him)]

We have decided on something like the following:

context = {
  triggeredBy: 'triggerHook',
  context: <supplied in API call>
}

then use jsone.render(hook.task, context) as the task definition.

and plan to add additionally

context = {
  triggeredBy: 'schedule',
}

context = {
  triggeredBy: 'webhook', // for webhooks with tokens
  body: <json body of webhook>
}

context = {
  triggeredBy: 'pulseMessage',
  routingKey: ..,
  message: ..,
}

Alex, can you file a bug to implement the 'schedule' and 'webhook' contexts?  After that, I think we can close this bug -- it was about deciding how to do all of this, and ^^ that is what we've decided :)