hubot-stackstorm
hubot-stackstorm copied to clipboard
Code Refactor
This refactor includes (resolves #128 and #114):
- pulling all the chatops specific code into a
messaging_handlerwhich is then pulled in once and also eachmessaging_handleris separated out into its own file for better organization. - changing it to support a listener per action-alias so that you can support hubot middleware plugins such as hubot-auth-middleware-ext (this was a pretty substantial change)
- adding an env variable called ST2_ALIAS_PACK_RESTRICTION which is a comma separated list that restricts it to importing only from the packs in that list (this was easier than disabling the alises I didn't want which got reenabled I believe at one point)
- if you define an extra/hubot_auth element in your action alias, it will take that data and add listener opts that the hubot-middleware-auth-ext plugin can pick up and use for authentication
- used event emitters to better decouple code
- you can hide chatops commands from help by setting a representation with no display
if you set something like this in your env file:
export ST2_ALIAS_PACK_RESTRICTION=ems
export HUBOT_AUTH_ADMIN=U111111,U222222 # chatops id, in this case slack id
export HUBOT_AUTH_MIDDLEWARE_ENVIRONMENT=production # if you have multiple stackstorm instances in various envs you can use this to control which aliases will run in which envs
then configure your action-aliases to look like the following, it will add role/room authentication to the action-alias. (hubot-auth-middleware-ext also support different environments as well)
---
name: some_action_alias
pack: some_pack
description: "this alias does some work"
action_ref: "some_pack.hello_world"
formats:
- "hello wolrd {{ name }}"
extra:
hubot_auth:
roles:
- devops
rooms:
- general
roles and/or rooms are optional although it might be pointless if you don't have either of them and still define the hubot_auth part. (it won't do auth if you leave it all out)
https://github.com/HelloFax/hubot-auth-middleware (should note that the npm package has ext on the end of it, the directions are not accurate on the package name)
I renamed the npm package to hubot-stackstorm-auth for now so it's easier to use.
If you want to persist the roles, you'll need to install redis.
cd /opt/stackstorm/chatops
sudo npm install hubot-stackstorm-auth
sudo npm install hubot-auth
sudo npm install hubot-auth-middleware-ext
sudo npm install hubot-redis-brain
the external-scripts.json would then look like this:
[
"hubot-redis-brain",
"hubot-stackstorm-auth",
"hubot-auth",
"hubot-auth-middleware-ext",
"hubot-help"
]
obviously installing hubot-stackstorm from this repo instead of npm.
Remaining things to do:
- Add Duo Auth back in
- Resolve merge conflicts which I believe is the Mattermost addition
- Support reloading of the action-aliases without restarting the st2chatops service
- Get Tests working
Incredible! Awesome work.
I'll try to find the time to review this asap, and figure out a way to include it in the st2chatops package as well.
@emedvedev I noticed on the original code there was support for sending a signal to reload the commands and then there's also a timer that reloads them by default every 120 seconds, does something in the st2 stack trigger the chatops service to reload commands and/or do we need support for reloading commands both ways?
@silverbp Both ways of (re) loading commands will need to be supported in this rewrite.
I like this, but can you rebase on current master?
I have refactored the chat provider adapters in #186 using part of this PR as a blueprint. Hopefully this makes it easier for the community to add chat provider adapters for other platforms.
Hopefully we can refactor scripts/stackstorm.js (which is now src/stackstorm.js) soon.
@silverbp Thank you for taking the time to write this PR, I have learned a great deal about Javascript coding practices from reading your code.
Although we won't be using this PR directly when refactoring src/stackstorm.js, I will keep this PR open until we do.
The scripts/stackstorm.js module was moved, split up, and partially refactored in #187.
Further Work
Now that StackStormApi has been turned into a JS class, it should be easier to write unit tests for each of its public methods, and to update it to use new style JS classes with the class and extends keywords.
- [ ] Change the
stop()method to shutdown the previously openedst2streaminstead of opening a new one and immediately shutting it down - requires manual testing to make sure it doesn't break anything, or add a comment thatst2client.jscaches the connection and reuses it, so opening the same connection simply reuses the old connection and correctly shuts it down - [ ] Add unit tests for
StackStormApipublic methods - [ ] Investigate moving things into the
stackstorm.jswrapper- [ ] the
process.onunhandled rejection handler registration - [ ] the
robot.errorhandler registration - [ ] the
robot.respondhandler registration - [ ] the
robot.router.postendpoint (seestackstorm.js:51from this PR) - [ ] any other endpoints (I expect at least one to be added for ChatOps authentication as part of ChatOps RBAC)
- [ ] the
- [ ] Decouple
stackstorm_api.jsfromstackstorm.jsa bit, similar to how it is implemented here- [ ] have
StackStormApiinherit fromEventEmitter - [ ] emit
st2.chatops_announcementandst2.execution_errorevents (consumed instackstorm.js:68andstackstorm.js:72, respectively)- [ ] also possibly emit/handler an event for two-factor authorization requests (see
HUBOT_2FA)
- [ ] also possibly emit/handler an event for two-factor authorization requests (see
- [ ] have
- [ ] Return the promises in
loadCommandsandstart
I'm leaving this PR open to track refactoring progress. I'll close it once the refactoring inspired by this PR is complete.
Marking as stale only so Pull Reminder doesn't keep pinging me about this.
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.