Added liquid templating and migrated first agents

[dsander]

Like discussed in #255 I added liquid templating. The two main benefits are that it is now possible to emit customized events which HTML formatting and that the EventFormatterAgent is not needed anymore for simple use cases (for example send Basecamp updates to a Hipchat channel).

At the moment just Hipchat and EventFormatter Agents are migrated because I am using them actively and thus was able to verify the migration worked.
I am not sure if I covered every use case of JSONPaths in the LiquidMigrator so it would be great if you could check if it would work with your agent configurations.

Update: I had a quick look at every available agent and checked if the need to be migrated or not. Agents marked with an asterisk could benefit from using Liquid to gain flexibility.

Agent which do not need an update:

AdiosoAgent
BasecampAgent
DigestEmailAgent
EmailAgent
FtpsiteAgent
GrowlAgent *
JavaScriptAgent
ManualEventAgent
PostAgent
PublicTransportAgent
PushoverAgent
SentimentAgent
ShellCommandAgent
StubhubAgent
TwilioAgent
TwitterStreamAgent
TwitterUserAgent
UserLocationAgent
WeatherAgent
WebhookAgent
WebsiteAgent
WeiboPublishAgent *
WeiboUserAgent

Agent which need to be migrated:

• DataOutputAgent
• HumanTaskAgent
• JabberAgent
• PeakDetectorAgent
• TranslationAgent
• TriggerAgent ?
• TwitterPublishAgent
• EventFormattingAgent
• HipchatAgent
• PushbulletAgent

[cantino]

Slick! I'll check this out tonight.

[cantino]

You're correct that some people might be using more complex JSONPath expressions than basic Liquid can support. Since we can't automatically translate them, should we make upgrading be a manual process? Alternatively, we could detect in LiquidMigrator if a JSONPath expression is more than just \A(\$\.)?(\w+\.)*(\w+)\Z and the migration could fail.

As an aside, this also brings me back to wondering what the best way to version a shared Rails app is...

[cantino]

Style: We try to follow the GitHub style guide for Ruby, using parentheses around method arguments.

[cantino]

Is it weird to use liquid templates for some options keys, and JSONPath for others (e.g., those used for configuration and those ending with _path)? I wonder if there is some way to combine the two, or make Liquid navigate using JSONPath?

[dsander]

You are right about the complex JSONPaths, when I wrote the migration I did not fully understand the usage of those those complex paths in huginn yet (and I am still learning). So i will check if the JSONPath is simple enough and otherwise raise an exception mentioning the migration needs to be fine tuned.

About descending into nested hashes I am still not sure if it is needed, but I thought it might be useful in some cases (it would make the migrations a bit simpler, but could also cause trouble).

Well, at first I was thinking Liquid could replace JSONPaths everywhere but that clearly is not possible when using complex selectors.
How about we use Liquid when strings are interpolated (really just use it for templating) and JSONPaths as selectors only (for example in the WebsiteAgent)
Following this 'rule' I would also update the matches section of the EventFormattingAgent to use Liquid.
I have one question though, in the EventFormattingAgent spec this <$.content.text.*> is used, but in the payload just content.text is a string, so the asterisk unnecessary? (That is why I just removed trailing asterisks in the LiquidMigrator)

After the Liquid migration the _path option keys should be gone and the JsonPathOptionsOverwritable should not be needed anymore. If someone wants to provide a default option it is still possible using {{data | default:'default'}}

[cantino]

Okay, I think this makes sense. I actually don't know the details of complex JSONPath. I've only ever used $.foo.bar, to be honest. I don't know if people use more complex selectors.

[dsander]

@cantino I think we are good to merge once I have added some documentation about Liquid in the wiki, what is your opinion?

[snicker]

I do know that the ruby implementation of JSONPath is... broken? Evaluation did not work properly when I tested it previously (see commentary in #175 and #177), so I, for one, welcome our new Liquid overlords

[cantino]

I will try to deploy it on my setup tomorrow and sanity check everything.

[cantino]

Also, which Agents still are left to migrate? (Maybe I can help with them!)

[dsander]

@cantino for now just the Hipchat, Pushbullet and EventFormattingAgent are being migrated. I did not look at all the other agents yet. Do you want to migrate them all before merging?

@snicker I read about the JSONPath evaluation, where do you want to use it? Because with liquid we can use filters and conditions, but i do not think it can replace JSONPath when selecting parts of a hash.

[snicker]

@dsander the use cases I had in mind were limited and have other workarounds... so I didn't get too hung up on it.

[albertsun]

Is this the right link to the liquid documentation? http://docs.shopify.com/themes/liquid-basics

Should add that to the descriptions of each agent that will be using it.

[dsander]

@albertsun yeah i just started writing the documentation a few hours ago :)

[dsander]

All the "simple" agents are migrated to liquid. I could work on the other three, but would just rely on the specs and I doubt thats a good idea 😉

[cantino]

I think this will need to be recursive to handle Agents that use nested structures, like the HumanTaskAgent, or the DataOutputAgent.

[dsander]

You are right, for the DataOutputAgent I just used the item hash, but we probably want support for liquid in the hole template hash.

[cantino]

I'm falling a bit behind on reviewing this and the bootstrap upgrade PR, but I'll get to them within the next few days. Sorry for the delay!

(I'm going to be flying to Berlin on Saturday. I was invited to give a talk about Huginn at Data Science Day next week!)

[dsander]

No worries, I know this is a big one to review 😄

This is cool, are you going to be in berlin the whole week? I will see if I can come too!

[cantino]

Running the Migrator, I got:

JSONPath '$first_title' is too complex, please check your migration./Users/andrew/personal/huginn/lib/liquid_migrator.rb:60:in `check_path'

I think we should support $foo as an alias of $.foo, as the JSONPath gem supports them both. (I don't know if they have a differing interpretation.)

Also, I'd suggest adding the Agent class and id to the error message, if it's easy. E.g., "JSONPath '$first_title' in HipchatAgent#2 is too complex, please ...".

Edit: looks like making the period optional fixes it:

if string !~ /\A(\$\.?)?(\w+\.)*(\w+)\Z/

[dsander]

Ah, i did not know about the $first_title syntax. I will change it later today.

[cantino]

For TriggerAgents, I think we just need to convert the message field to Liquid interpolation. The exact same is true of the PeakDetectorAgent.

For the HumanTaskAgent, I think you should recursively apply Liquid interpolation to everything in the hit option. It can be multiple levels deep. If you wouldn't mind dropping it in, I'll run it on my production setup and let you know if everything works. It looks like it should.

[cantino]

Also, these migrations should all probably look like this:

class MigrateStufftToLiquid < ActiveRecord::Migration
  def up
    # calls to LiquidMigrator ...
  end

  def down
    raise ActiveRecord::IrreversibleMigration, "Cannot revert migration to Liquid templating"
  end
end

Unless you want to write a LiquidReverseMigrator, which might be annoying (although not a bad idea).

[mfclarke]

Apologies if I'm missing something, but I don't see any plans to get liquid into the email agents. It would be great to have liquid variables for email subjects and format the body in html+liquid. As well as be able to use liquid for loops and conditionals in the digest agent's body.

Might be a bit much for a migration to handle (speaking for myself - if anyone thinks otherwise then by all means!). I'd be willing to do a separate liquid email agent...?

[dsander]

@mfclarke you are right the email agents could gain a lot of flexibility with liquid, but in this PR I am just planning to convert the existing JSONPath interpolation to liquid.
I am not sure if it is possible to change the existing email agents to (optionally) support liquid without breaking the current installations.

[dsander]

@cantino what do you think about merging the migrations into one file to keep the directory a bit cleaner?

[cantino]

That makes sense to me, @dsander.

[dsander]

Those were the last missing migrations ✌️. I just tested them with the default options, so I hope this does not break in production. Since all migrations a in one file now, we should be covert by the wrapping transaction.

[cantino]

I just deployed it to my server. I'll let it run 2-3 days, and merge if everything has been running well. So far, it's working great!

[cantino]

Okay @dsander, this has been working for me. Merging it in!

[cantino]

Awesome work!!

[dsander]

Great! Thank you for reviewing and testing it.