Skip to content

Migrate twilio webhooks to the webhook component#17715

Merged
balloob merged 5 commits intohome-assistant:devfrom
rohankapoorcom:twilio-webhooks
Oct 25, 2018
Merged

Migrate twilio webhooks to the webhook component#17715
balloob merged 5 commits intohome-assistant:devfrom
rohankapoorcom:twilio-webhooks

Conversation

@rohankapoorcom
Copy link
Copy Markdown
Member

@rohankapoorcom rohankapoorcom commented Oct 23, 2018

Description:

Migrating Twilio over to the webhook API so that it doesn't need an api password.

This is built on top of #17464 (which needs to be merged before this one). Tests will not pass until after that is merged.

Breaking Change: Each instance of Home Assistant will now generate it's own unique webhook url for Twilio to use. One will need to be generated and provided to Twilio for incoming calls/messages from Twilio to Home Assistant to continue to flow.

I followed the approach used in #16817 to implement a new config flow to register the webhook. Existing configuration (account_sid, auth_token) still occurs via the configuration.yaml file.

I do not actually use twilio myself so I cannot test the behavior with Twilio, but the unit tests look okay.

Related issue (if applicable): Related to #15376

Pull request in home-assistant.io with documentation (if applicable): home-assistant/home-assistant.io#7074

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (example).
  • New or updated dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

blank line at end of file

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

blank line at end of file

@ghost ghost assigned balloob Oct 23, 2018
@balloob
Copy link
Copy Markdown
Member

balloob commented Oct 23, 2018

I rebased but it didn't pass tests.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Now that I think about it, we probably don't want to support the webhook id being set by the yaml config...

@rohankapoorcom
Copy link
Copy Markdown
Member Author

rohankapoorcom commented Oct 23, 2018

Very strange, I'm able to reproduce locally, the error it's showing is ModuleNotFoundError: No module named 'twilio'. This is provided within the REQUIREMENTS for twilio as well as within requirements_all.txt

Any idea why it wouldn't be finding the module?

What's even more confusing is that it works fine when running Home Assistant and does in fact install the twilio module and the component loads correctly.

@balloob
Copy link
Copy Markdown
Member

balloob commented Oct 23, 2018

twilio needs to be part of the test requirements. Open gen_requirements.py and add twilio to the list.

@balloob
Copy link
Copy Markdown
Member

balloob commented Oct 23, 2018

Or better, don't add it to the list and mock it using MockDependency

@@ -0,0 +1,41 @@
"""Test the init file of Twilio."""
from unittest.mock import patch, Mock
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

'unittest.mock.Mock' imported but unused

@balloob balloob merged commit 5024a80 into home-assistant:dev Oct 25, 2018
@ghost ghost removed the in progress label Oct 25, 2018
@rohankapoorcom rohankapoorcom deleted the twilio-webhooks branch October 25, 2018 07:51
@rohankapoorcom
Copy link
Copy Markdown
Member Author

rohankapoorcom commented Oct 26, 2018

@balloob @MartinHjelmare does this need a breaking change label? The breaking change paragraph is in the description.

@balloob
Copy link
Copy Markdown
Member

balloob commented Oct 26, 2018

Added!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants