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

Allow programmatic launching of hubot #858

Closed
5 tasks
michaelansel opened this issue Feb 3, 2015 · 6 comments
Closed
5 tasks

Allow programmatic launching of hubot #858

michaelansel opened this issue Feb 3, 2015 · 6 comments
Assignees

Comments

@michaelansel
Copy link
Collaborator

As discussed in several other issues (#514, #768, #801, #815), there is a desire for a programmatic way of launching hubot to control script loading and to combine with existing node applications.

The proposed solution is something along the lines of Rack's config.ru (which I'll reference as the "launch config file").

Primary design constraints:

  • hubot should be able to be launched from an existing node.js application
  • the launch config file should specify which scripts are to be loaded and in what order
  • the launch config file should allow injecting arbitrary middleware/dependencies early in the process (e.g. load an external configuration file before any scripts that would use it)
  • the launch config file should allow arbitrary JS/CS code for detailed configuration of complex extensions (e.g. load an authorization framework and then configure it inside the launch config file)
  • the launch config file should have a mechanism for loading legacy scripts (assuming the script interface changes)

Not really in scope, but could we make it so that a robot can be attached to multiple adapters?

Terminology:

  • script is the current style of scripts (module.exports is a Function that is called with the robot object)
  • module is the new-style of scripts; details are undefined other than it can be more than just a Function and could have a namespace for all listeners it registers

An initial pass at the design:

module.exports = (robot) ->

# Adapter
robot.use require('hubot-irc')

# Middleware
robot.use require('hubot-ldap-user-info') # add extra fields to the msg.user object
robot.use require('hubot-rbac') # apply RBAC authorization rules

# Scripts/Modules
robot.use robot.wrapLegacyScript('mysql', require('hubot-mysql-chatops'))
robot.use require('hubot-duo') # accept two-factor keys and update global user info

Breakdown:

module.exports = (robot) ->

Maybe stick with the current script interface? This could make forward-compatibility possible (which would make moving to the rackup file optional and not require a major version bump).

# Adapter
robot.use require('hubot-irc')

Could we make adapters just another module? Not sure what the implications are for the underlying implementation.

# Middleware
robot.use require('hubot-ldap-user-info') # add extra fields to the msg.user object
robot.use require('hubot-rbac') # apply RBAC authorization rules

Load middleware in whatever order makes sense (since order matters for middleware)

robot.use robot.wrapLegacyScript('mysql', require('hubot-mysql-chatops'))

wrapLegacyScript allows for loading of old-style scripts. It creates a new robot object that extends the real robot so that the legacy script is completely isolated; sandboxes naughty scripts that modify the global robot object. This also implies that new-style scripts will have a namespace that needs to be supplemented ('mysql' in this example).

robot.use require('hubot-duo')

"Standard" module loading method. Just pull in a module and hand it over to the robot. This suggests that a module is more than a function that needs to be called. This could also permit very interesting patterns around generating scripts at load-time (a la #813). If each module has a unique namespace for all listeners, you could even have dynamic unloading of modules.

Some other little tidbits I came up with:

# this could be an easy way of replacing the existing hubot-scripts.json
[
  'ackbar'
].forEach (scriptName) ->
  robot.use robot.wrapLegacyScript('hubot-scripts.'+scriptName, require('hubot-scripts/src/'+scriptName))

# simple patch to existing modules that use legacy scripts
for scriptName in scripts/*
  robot.use robot.wrapLegacyScript(robot.name+'.'+scriptName, require('./scripts/'+scriptName))

Open questions/requirements

  • What, if any, changes need to be made to the generator
  • What part of the interface should be async?
  • Should we change the script interface?
  • Can/Should we sandbox scripts for easier hot-reloading?
  • Where is the decision point between the current load method and the new launch config file?
@michaelansel
Copy link
Collaborator Author

Open question: what, if any, of this needs to be asynchronous? We may be able to keep the launch config file synchronous, but permit a different way of loading that has async calls. Given the nature of JavaScript, I think we always want to provide some mechanism for async extensions.

Example: Generate a module asynchronously (e.g. pull data from JIRA), load it (could this ever be async?), and finally start the robot (connect the adapter)

# part of my custom node application
robot = new Robot(<stuff>)
asyncFunctionToGenerateModules 'my-module', (genModule) ->
  robot.useAsync genModule, () ->
    robot.run

@technicalpickles
Copy link
Member

This a great writeup, thanks for putting it together!

hubot should be able to be launched from an existing node.js application

I'm not so sure about this one. I think some might interpret this as being say, launchable from express application cc #815. That is a bit more complicated because of hubot's builtin express router cc #767

the launch config file should have a mechanism for loading legacy scripts (assuming the script interface changes)

How are we defining legacy scripts exactly? Preferably, we'd have backwards compatability with the hubot-scripts package, as well as npm script packages.

Not really in scope, but could we make it so that a robot can be attached to multiple adapters?

Definitely out of scope. The problem with multiple adapters is more about user identify and persistence.

Could we make adapters just another module? Not sure what the implications are for the underlying implementation.

That is going to be more complicated, because of the builtin adapters. That is probably an argument they should be made external to hubot though. I also tend to think of the adapter used as more of a run-time decision. That is, I might run in development with shell, test with the mock test adapter, and run in production with campfire, so having that be programatic is less interesting I think.

wrapLegacyScript allows for loading of old-style scripts. It creates a new robot object that extends the real robot so that the legacy script is completely isolated; sandboxes naughty scripts that modify the global robot object.

I'm not sure about this bit either. If you are making a new robot instance, you'd also have to manage all the lifecycle events. I'm also not sure what the difference between a legacy and non-legacy script is at this time. Preferably, robot.use would be smart enough to take whatever it's given. It can be typeofd, and if it's a function, you can check the arrity, if that helps.

what, if any, of this needs to be asynchronous? We may be able to keep the launch config file synchronous, but permit a different way of loading that has async calls.

That's a good question. I'd prefer sync for the initialization, because then you can know when you have a bootable hubot. I guess you'd be able to async some things, like polling an external service to dynamically create listeners, but practically speaking you'd want all the core things (like rbac) to be loaded up front before firing of those.


Aside from what you've brought up, two other points I'd consider:

  • how does this effect the current bin/hubot? Would that check for a 'launch config file' in a well known location?
  • how does this affect the generator?

Kinda tied together, but I'd suggest generating a index.coffee in new hubots, and then in the generated bin/hubot shell script (not to be confused with github/hubot's bin/hubot), tell it to use that 'launch config file'.

@michaelansel
Copy link
Collaborator Author

Thanks for the feedback! A few quick overarching thoughts:

  • my plan is to maintain backwards compatibility with all existing hubots/scripts. Defining a new interface is hard enough; moving an entire community overnight is near impossible. Eventually, I imagine we would deprecate the old interfaces, but that can wait for another day (and a clear migration path).
  • I'm starting on the design now (I expect it to take a while), but I want to get a full battery of automated tests in place before I begin making changes.
  • I've tried to read through most of the vthree-rethink branch, but I may have missed stuff, so let me know if there are points in there I should look at.
  • This will probably spin off some dependent work (like automated tests), but I'll try to keep those pretty contained and easy to merge without the "big changes".

hubot should be able to be launched from an existing node.js application
That is a bit more complicated because of hubot's builtin express router

As I understand it, the long-term goal is to pull Express out of the core. I would be okay assuming #767 is a dependency for embedding hubot in an existing Express server (but we could start working on the new structure without it).

How are we defining legacy scripts exactly? Preferably, we'd have backwards compatibility with the hubot-scripts package, as well as npm script packages.

Legacy scripts are scripts that follow the current model of module.exports = (robot) ->. I'm planning for the possibility of changing that interface (though I don't know what it would look like yet). I expect to maintain backwards-compatibility no matter what.

Could we make adapters just another module?

In particular, I'd like to move module resolution out of hubot core. loadFile doing path resolution, extension checking, and module loading just feels really dirty (and has caused me pain in a couple of places). I'd like for all requires to happen before we launch the robot instead of dynamically requiring a new module as part of robot.run (pretty sure dynamic loading is a JavaScript anti-pattern). I agree that this will need a lot more detailed thought.

If you are making a new robot instance, you'd also have to manage all the lifecycle events. I'm also not sure what the difference between a legacy and non-legacy script is at this time. Preferably, robot.use would be smart enough to take whatever it's given. It can be typeofd, and if it's a function, you can check the arrity, if that helps.

My (untested) thought was to rely on protypical inheritance extend the Robot instance. This way, a script that tries to set up robot.myModulesThing = herpDerp doesn't impact any other scripts. It also allows you to easily contain all the listeners/middleware that a script creates and associate them with the name of the script that created them (imagine having help sorted by script instead of just a giant list of commands a la #862 ). This would need a pretty good sized test bed to make sure we aren't breaking functionality (automated tests are a must for all of this).

I like the idea of making robot.use more intelligent. wrapLegacyScript was leftover from an earlier iteration of the design and can probably be dropped.

how does this effect the current bin/hubot? Would that check for a 'launch config file' in a well known location?

I haven't thought about it too much, but I think so. Eventually, I'd like to strip down pretty much everything that is in bin/hubot and move it into the core. I think we can maintain the exact same interface that currently exists.

how does this affect the generator?

Erm.. not sure, but I would assume it would get updated if the script interface changes.

Kinda tied together, but I'd suggest generating a index.coffee in new hubots, and then in the generated bin/hubot shell script (not to be confused with github/hubot's bin/hubot), tell it to use that 'launch config file'.

I'm going to need to dig into that some more. I don't (yet) fully understand the boot process, but I expect to before I start making changes.

@technicalpickles
Copy link
Member

cc #847 as well, as that'd let users require newrelic up at the top of their launch config.

@michaelansel
Copy link
Collaborator Author

Another thing to think about: if we start sharing the Express server (or even colocating multiple bots), how do URL paths look? cc: #622 Not sure this is even something that we want to support, but should be considered.

@technicalpickles
Copy link
Member

Another thing to think about: if we start sharing the Express server (or even colocating multiple bots), how do URL paths look?

That's a good question. I'm not sure it's something this feature needs to be concerned about yet. I think having express be extracted out will be a bit easier with this feature implemented.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants