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

Merge Slack and Websocket connectors to the core project #530

Merged
merged 20 commits into from
Jun 15, 2018

Conversation

jacobtomlinson
Copy link
Member

@jacobtomlinson jacobtomlinson commented Apr 20, 2018

Description

This PR is the start of the work on #185 by adding the Slack and Websocket connectors into the core project.

The loader now checks for connector modules within the opsdroid core library before looking to load an external plugin. This makes it fully backward compatible.

Status

UNDER DEVELOPMENT

Type of change

  • Core maintenance

How Has This Been Tested?

New unit tests have been added to fully test the code within the connectors, however the third party APIs have been mocked.

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
  • Thoroughly test both connectors.
  • Improve the connectors documentation to include tutorial like instructions on how to set it up.
  • Add documentation about the fact there are now builtin connectors.

@codecov
Copy link

codecov bot commented Apr 20, 2018

Codecov Report

Merging #530 into master will increase coverage by 0.08%.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #530      +/-   ##
========================================
+ Coverage   99.91%   100%   +0.08%     
========================================
  Files          18     20       +2     
  Lines        1135   1312     +177     
========================================
+ Hits         1134   1312     +178     
+ Misses          1      0       -1
Impacted Files Coverage Δ
opsdroid/connector/__init__.py 100% <ø> (ø)
opsdroid/connector/websocket/__init__.py 100% <100%> (ø)
opsdroid/loader.py 100% <100%> (ø) ⬆️
opsdroid/connector/slack/__init__.py 100% <100%> (ø)
opsdroid/core.py 100% <0%> (+0.54%) ⬆️

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 df22850...0e338af. Read the comment docs.

@FabioRosado
Copy link
Member

I looked over at the code and I know you are still working on it. The idea is to move all the other connectors into opsdroid core right? Or slack will be the only one moved at the moment? (I guess people would use slack more than any of the others)

@jacobtomlinson
Copy link
Member Author

jacobtomlinson commented Apr 21, 2018

Yeah the idea is to eventually move all the connectors and database modules into the core project. I just wanted to take it in small chunks as it is quite a lot of work.

So this PR updated the loader to look for modules in the core project first and adds the first two connectors. Once it's finished and merged I'll start looking into the other modules. I was hoping for some of them like the matrix connector which was written by someone else that I could get them to help merge it in.

@FabioRosado
Copy link
Member

I'm not sure if you are still working on this Jacob but I assume it's pretty much done, I got some time to go through it today and this looks good let me know if you want it merged now 😄 👍

@jacobtomlinson
Copy link
Member Author

This still needs documenting. Sorry I've been busy and this slipped my mind.

@FabioRosado
Copy link
Member

It’s okay it happens :D I’ve been busy learning js as well haha
Do you need any help with documenting?

@jacobtomlinson
Copy link
Member Author

If you're happy to help then yes please!

@FabioRosado
Copy link
Member

Hey Jacob I just wanted to check with you a few things. For me to submit commits into your PR do I just need to add you as a remote on my end, create a branch and then push these changes into your own branch?

The tutorial do you want me to write just how to set it up (like config options and whatnot?)

The builtin connectors documentation should I just update the connector documentation and mention this fact, or would you prefer to have a new file for this?

Thanks

@jacobtomlinson
Copy link
Member Author

Yes you should be able you add my fork as a remote.

I think we need to update the connector docs to say there are builtin connectors and you can use plugins. We should also have a documentation page for each built-in, like we have for parsers.

@FabioRosado
Copy link
Member

FabioRosado commented Jun 8, 2018

I've changed just a bit of the documentation since you started with it anyway. I didn't create a tutorial on how to set it up because slack.md seems to be pretty self-explanatory, but if you would like to add an in-depth one I will be more than happy to create one.

I have also tested to see if opsdroid would connect to slack and respond to hello and everything worked flawlessly 👍

--EDIT--
I have noticed that the build seems to fail from time to time mostly due to timeout issues. I think the reason might be that some tests call sleep and that makes them run longer than 30s. Do you think this might be the case?

@jacobtomlinson
Copy link
Member Author

Sorry I haven't had a chance to look at this, been super busy this last week. Do you mean test failures of readthedocs failures?

@FabioRosado
Copy link
Member

It's okay sometimes we get extremely busy with work and life 👍
I was updating all the dependencies the other day and it seems that some tests either erroed (like the docker failed to connect to aws) or some python test (like pythong 3.5) failed because of timeout, not sure if the issue was within travis (it seems that week they had some issues) or if our tests don't help much haha

@jacobtomlinson
Copy link
Member Author

Ah interesting! The tests seem to be passing here so perhaps you could raise it as a separate issue?

Thanks again for adding the documentation, huge help!

@jacobtomlinson jacobtomlinson merged commit 5b4c5df into opsdroid:master Jun 15, 2018
@jacobtomlinson jacobtomlinson deleted the merge-connectors branch June 15, 2018 07:01
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.

None yet

2 participants