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

Migrate twilio webhooks to the webhook component #17715

Merged
merged 5 commits into from Oct 25, 2018

Conversation

@rohankapoorcom
Contributor

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.

@rohankapoorcom rohankapoorcom requested a review from home-assistant/core as a code owner Oct 23, 2018

@wafflebot wafflebot bot added the in progress label Oct 23, 2018

@@ -0,0 +1 @@
"""Tests for the Twilio component."""

This comment has been minimized.

@houndci-bot

houndci-bot Oct 23, 2018

blank line at end of file

'https://www.twilio.com/docs/glossary/what-is-a-webhook',
'docs_url': 'https://www.home-assistant.io/components/twilio/'
}
)

This comment has been minimized.

@houndci-bot

houndci-bot Oct 23, 2018

blank line at end of file

@rohankapoorcom rohankapoorcom force-pushed the rohankapoorcom:twilio-webhooks branch from d6fadeb to f9c445f Oct 23, 2018

@balloob balloob force-pushed the rohankapoorcom:twilio-webhooks branch from f9c445f to 2675e2c Oct 23, 2018

@balloob

This comment has been minimized.

Member

balloob commented Oct 23, 2018

I rebased but it didn't pass tests.

vol.Optional(DOMAIN): vol.Schema({
vol.Required(CONF_ACCOUNT_SID): cv.string,
vol.Required(CONF_AUTH_TOKEN): cv.string,
vol.Optional(CONF_WEBHOOK_ID): cv.string,

This comment has been minimized.

@rohankapoorcom

rohankapoorcom Oct 23, 2018

Contributor

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

@rohankapoorcom rohankapoorcom force-pushed the rohankapoorcom:twilio-webhooks branch from 2675e2c to 5b7f3f5 Oct 23, 2018

@rohankapoorcom

This comment has been minimized.

Contributor

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

@houndci-bot

houndci-bot Oct 25, 2018

'unittest.mock.Mock' imported but unused

rohankapoorcom added some commits Oct 25, 2018

@rohankapoorcom rohankapoorcom referenced this pull request Oct 25, 2018

Merged

Add documentation for Twilio webhook changes #7074

2 of 2 tasks complete

@balloob balloob merged commit 5024a80 into home-assistant:dev Oct 25, 2018

5 checks passed

Hound No violations found. Woof!
WIP ready for review
Details
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.09%) to 93.502%
Details

@wafflebot wafflebot bot removed the in progress label Oct 25, 2018

@rohankapoorcom rohankapoorcom deleted the rohankapoorcom:twilio-webhooks branch Oct 25, 2018

@rohankapoorcom

This comment has been minimized.

Contributor

rohankapoorcom commented Oct 26, 2018

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

@balloob

This comment has been minimized.

Member

balloob commented Oct 26, 2018

Added!

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