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

Log errors and exit on authentication issues on Hubot plugin initialization #168

Closed
wants to merge 52 commits into from

Conversation

jinpingh
Copy link
Contributor

@jinpingh jinpingh commented May 21, 2019

Fixes StackStorm/st2chatops#124

  1. Capture all authentication related exceptions and exit from st2chatops.
  2. Create new callback function for robot.error to capture uncaught error from hubot.
  3. Add unit tests for authentication error.

Closes StackStorm/st2tests#170

Copy link
Member

@arm4b arm4b left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find the fact that we need introducing ENV variable like EXIT_ON_FAILURES a bad sign.

It's an expected practice for an app to exit if there is a critical error we can't recover from.
Otherwise what value brings the daemon which prints single Failed to authenticate: error message and going into a waiting behavior?

If we talk about the failure which could be potentially recovered by connection retries/or retries with backoff, - we can consider using that instead. But if nothing helped, - always exit with error message and non-zero exit code.

@blag
Copy link
Contributor

blag commented May 28, 2019

If we talk about the failure which could be potentially recovered by connection retries/or retries with backoff, - we can consider using that instead.

We have this behavior between other microservices of ST2, so it would be nice to have the same behavior in st2chatops. But code-wise, it would be frustrating to keep the semantics of "retry with backoff" in st2chatops up-to-date with how it works in the rest of ST2.

But if nothing helped, - always exit with error message and non-zero exit code.

This is the easiest thing for us to do. At this point, I think this behavior is a good compromise between trying to behave like the other ST2 services and increased complexity of this codebase/PR.

@jinpingh jinpingh force-pushed the issue-124/exit_on_failures branch from 1f43a26 to 8f6dcad Compare May 28, 2019 20:00
Fixes StackStorm/st2chatops#124
For the case if st2chatops encounters authenticate or unresolved application URL issues with Stackstorm, instead it's being unresponsive/hanging/waiting in a running state, st2chatops will be exit.
@jinpingh jinpingh force-pushed the issue-124/exit_on_failures branch from 8f6dcad to 8f7c74e Compare May 28, 2019 20:05
@blag blag mentioned this pull request Jun 4, 2019
@jinpingh jinpingh force-pushed the issue-124/exit_on_failures branch 2 times, most recently from 81ecd17 to 6165f1e Compare June 11, 2019 02:49
jinpingh added 4 commits June 11, 2019 08:48
Log error inside logErrorAndExit other than depend on caller.
Check one more error message for token and API key test cases.
@blag blag requested a review from arm4b June 18, 2019 08:58
@blag
Copy link
Contributor

blag commented Jun 18, 2019

@armab Sorry to keep pulling you back to this, but I figured out how to test this all, and I think it's finally in a mergeable state!

@blag
Copy link
Contributor

blag commented Jun 18, 2019

Coverage from current master:

------------------------|----------|----------|----------|----------|-------------------|
File                    |  % Stmts | % Branch |  % Funcs |  % Lines | Uncovered Line #s |
------------------------|----------|----------|----------|----------|-------------------|
All files               |    82.73 |    79.59 |    83.76 |    82.24 |                   |
 lib                    |    96.88 |    96.68 |    97.75 |    96.75 |                   |
  command_factory.js    |      100 |      100 |      100 |      100 |                   |
  format_command.js     |      100 |      100 |      100 |      100 |                   |
  format_data.js        |      100 |      100 |      100 |      100 |                   |
  post_data.js          |      100 |      100 |      100 |      100 |                   |
  slack-messages.js     |    88.89 |    81.58 |    91.67 |    87.64 |... 34,35,37,38,43 |
  slack_monkey_patch.js |    28.57 |       80 |       50 |    28.57 |    17,18,19,28,29 |
  utils.js              |      100 |      100 |      100 |      100 |                   |
 scripts                |    41.81 |    37.11 |    39.29 |    41.81 |                   |
  stackstorm.js         |    41.81 |    37.11 |    39.29 |    41.81 |... 04,405,406,431 |
------------------------|----------|----------|----------|----------|-------------------|

Coverage from this branch:

------------------------|----------|----------|----------|----------|-------------------|
File                    |  % Stmts | % Branch |  % Funcs |  % Lines | Uncovered Line #s |
------------------------|----------|----------|----------|----------|-------------------|
All files               |     87.9 |    86.34 |    86.67 |    87.56 |                   |
 lib                    |       99 |    99.57 |    98.86 |    98.96 |                   |
  command_factory.js    |      100 |      100 |      100 |      100 |                   |
  format_command.js     |      100 |      100 |      100 |      100 |                   |
  format_data.js        |      100 |      100 |      100 |      100 |                   |
  post_data.js          |      100 |      100 |      100 |      100 |                   |
  slack-messages.js     |      100 |      100 |      100 |      100 |                   |
  slack_monkey_patch.js |    28.57 |       80 |       50 |    28.57 |    17,18,19,40,41 |
  utils.js              |      100 |      100 |      100 |      100 |                   |
 scripts                |    59.28 |    58.56 |    53.13 |    59.28 |                   |
  stackstorm.js         |    59.28 |    58.56 |    53.13 |    59.28 |... 96,425,461,462 |
------------------------|----------|----------|----------|----------|-------------------|

👍

@blag blag changed the title Exit st2chatop from st2client failures. Log errors and exit on authentication issues on Hubot plugin initialization Jun 18, 2019
@jinpingh jinpingh closed this Jun 26, 2019
@jinpingh jinpingh deleted the issue-124/exit_on_failures branch June 26, 2019 16:10
@arm4b arm4b mentioned this pull request Jun 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

st2chatops doesn't exit on ERROR, keeps waiting/unresponsive when AUTH/API unresolved
3 participants