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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle slack bad token issue #844

Merged
merged 4 commits into from
Mar 2, 2019
Merged

Conversation

FabioRosado
Copy link
Member

@FabioRosado FabioRosado commented Feb 14, 2019

Description

I tried to take a stab at the bad slack token issue. At the moment I made some changes so try and handle the flood of exceptions. I tried to start with slacker.Error but that didn't work, every little bit of the code was breaking because slack.disconnect() is called when a broad exception is passed and that made things break because there is nothing to disconnect.

Also slack.listen() is running on a loop so I tried to break out of the loop if the exception happens (which it will).

Finally, I had to take the raise statement from the connect because it was breaking opsdroid. I think opsdroid shouldn't crash bad if one connector doesn't work. I think most people would use more than one connector with opsdroid so it might be better to let opsdroid running.

I didn't do anything to the core yet because like I said before adding anything to it would make the if/else statement longer. I guess we could just set a return value and if it returns False opsdroid would just skip that connector?

This PR is just an idea and to be honest I am not sure if any of these changes are anything worthwhile but here it is. Let me know what do you think and then I will add tests if accepted 馃槃 馃憤

p.s.: should we add slacker to the requirements? It's not currently on them.

Fixes #764

Status

READY |UNDER DEVELOPMENT | ON HOLD

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration.

  • Tox - all green
  • Tested opsdroid with slacker connector and a random token - logged message and opsdroid didn't crash.

Checklist:

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation (if applicable)
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@codecov
Copy link

codecov bot commented Feb 24, 2019

Codecov Report

Merging #844 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #844   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          34     34           
  Lines        2077   2082    +5     
=====================================
+ Hits         2077   2082    +5
Impacted Files Coverage 螖
opsdroid/connector/slack/__init__.py 100% <100%> (酶) 猬嗭笍

Continue to review full report at Codecov.

Legend - Click here to learn more
螖 = absolute <relative> (impact), 酶 = not affected, ? = missing data
Powered by Codecov. Last update 6360912...3cd40cc. Read the comment docs.

@FabioRosado
Copy link
Member Author

This PR is now ready for a review. You were right when you mentioned that we were checking for errors elsewhere. Today I did a bit of digging and the fix was easier than I expected.

We just had to handle slacker.Error and log an error message to the console. We will still need to handle that AttributeError in connector.listen otherwise, the connector will crash badly with the log that I have mentioned in the issue.

I have also reverted the raise when a broad Exception happens within the connector mostly because this will be some edge case stuff and perhaps we need to make opsdroid crash so we get reports of it and fix whatever went wrong haha

@jacobtomlinson jacobtomlinson merged commit b4db89d into opsdroid:master Mar 2, 2019
@FabioRosado FabioRosado deleted the slack-auth branch April 12, 2019 09:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handle slacker connector invalid token exception
2 participants