Modernize and refactor stackstorm.js

[blag]

This PR breaks up src/stackstorm.js into src/stackstorm_api.js and src/stackstorm.js (which is now just a wrapper).

Additionally, the stackstorm_api.js module is refactored into a StackStormApi JS "class", and the closed over global functions in the original anonymous closure have been broken out into their own constituent public methods.

Notes

The stop function was reverted to the previous functionality to ensure that this refactoring doesn't break anything. That change will land in a future release.

The stop function was changed slightly. Instead of opening a new connection to ST2's event stream and then immediately closing it:

    api_client.stream.listen().then(function (st2stream) {
      st2stream.removeAllListeners();
      st2stream.close();
    });

it now saves the event stream handler and uses that to close and stop the event stream handler:

  if (self.st2stream) {
    self.st2stream.removeAllListeners();
    self.st2stream.close();
  }

Before, authenticate() was run conditionally:

  if (env.ST2_API_KEY || env.ST2_AUTH_TOKEN || env.ST2_AUTH_USERNAME || env.ST2_AUTH_PASSWORD) {
    // If using username and password then all are required.
    if ((env.ST2_AUTH_USERNAME || env.ST2_AUTH_PASSWORD) &&
        !(env.ST2_AUTH_USERNAME && env.ST2_AUTH_PASSWORD && env.ST2_AUTH_URL)) {
      throw new Error('Env variables ST2_AUTH_USERNAME, ST2_AUTH_PASSWORD and ST2_AUTH_URL should only be used together.');
    }
    authenticated = authenticate();
  }

And now that check is separated from the authenticate() call:

if (env.ST2_API_KEY || env.ST2_AUTH_TOKEN || env.ST2_AUTH_USERNAME || env.ST2_AUTH_PASSWORD) {
    // If using username and password then all are required.
    if ((env.ST2_AUTH_USERNAME || env.ST2_AUTH_PASSWORD) &&
      !(env.ST2_AUTH_USERNAME && env.ST2_AUTH_PASSWORD && env.ST2_AUTH_URL)) {
      throw new Error('Env variables ST2_AUTH_USERNAME, ST2_AUTH_PASSWORD and ST2_AUTH_URL should only be used together.');
    }
  }

I compensate for this in stackstorm.js almost perfectly:

module.exports = function (robot) {
  var stackstormApi = new StackStormApi(robot);

  return stackstormApi.authenticate().then(function () {

But, that isn’t quite good enough for some of the tests. So in order to test things like the environment variables ST2_API and HUBOT_2FA being handled, you now have to specify proper values for ST2_AUTH_URL, ST2_AUTH_USERNAME, and ST2_AUTH_PASSWORD:

test-st2bot-envvars.js:69

    restore_env = mockedEnv({
      ST2_API: 'https://nonexistent-st2-auth-url:9101',
      ST2_AUTH_URL: 'localhost:8000',
      ST2_AUTH_USERNAME: 'user',
      ST2_AUTH_PASSWORD: 'pass'
    });

test-st2bot-envvars.js:102

    restore_env = mockedEnv({
      ST2_AUTH_URL: 'localhost:8000',
      ST2_AUTH_USERNAME: 'user',
      ST2_AUTH_PASSWORD: 'pass',
      HUBOT_2FA: 'true'
    });

Coverage Differential

Test coverage went from:

---------------------|----------|----------|----------|----------|-------------------|
File                 |  % Stmts | % Branch |  % Funcs |  % Lines | Uncovered Line #s |
---------------------|----------|----------|----------|----------|-------------------|
All files            |    89.35 |    87.13 |    87.76 |    89.05 |                   |
 src                 |    62.16 |    60.55 |    63.33 |    62.16 |                   |
  stackstorm.js      |    62.16 |    60.55 |    63.33 |    62.16 |... 02,403,404,437 |
...
---------------------|----------|----------|----------|----------|-------------------|

to:

---------------------|----------|----------|----------|----------|-------------------|
File                 |  % Stmts | % Branch |  % Funcs |  % Lines | Uncovered Line #s |
---------------------|----------|----------|----------|----------|-------------------|
All files            |    89.58 |    87.28 |    87.76 |    89.29 |                   |
 src                 |       65 |    61.95 |    63.33 |       65 |                   |
  stackstorm.js      |      100 |      100 |      100 |      100 |                   |
  stackstorm_api.js  |    63.73 |    61.95 |    60.71 |    63.73 |... 52,454,455,457 |
...
---------------------|----------|----------|----------|----------|-------------------|

(unchanged coverage results omitted)

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 opened st2stream instead 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 that st2client.js caches the connection and reuses it, so opening the same connection simply reuses the old connection and correctly shuts it down
• Add unit tests for StackStormApi public methods
• Move things into the stackstorm.js wrapper
  • the process.on unhandled rejection handler registration
  • the robot.error handler registration
  • the robot.respond handler registration
  • the robot.router.post endpoint (see Code Refactor #133's stackstorm.js:51)
  • any other endpoints (I expect at least one to be added for ChatOps authentication as part of ChatOps RBAC)
• Decouple stackstorm_api.js from stackstorm.js a bit, similar to how it is implemented in Code Refactor #133
  • have StackStormApi inherit from EventEmitter
  • emit st2.chatops_announcement and st2.execution_error events (consumed in stackstorm.js:68 and stackstorm.js:72, respectively)
    • also possibly emit/handler an event for two-factor authorization requests (see HUBOT_2FA)
• Return the promises in loadCommands and start

Note that I disagree with how commandFactory was turned into an event emitter in #133 (stackstorm.js:84) so I'm not including it in this checklist.

Edit: Tracking overall refactoring progress in #133.

[jinpingh]

LGTM!

[blag]

GitHub and Travis aren't talking to each other properly right now.

The Travis CI results are here: https://travis-ci.org/StackStorm/hubot-stackstorm/builds/560748297. The build is passing. ✅

[arm4b]

Nice refactoring here!

👍