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 Mailgun to use the webhook component #17464

Merged
merged 8 commits into from Oct 23, 2018

Conversation

@rohankapoorcom
Contributor

rohankapoorcom commented Oct 15, 2018

Description:

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

This is built on top of #17460 (which is approved and needs to be merged before this one).

Breaking Change: Each instance of Home Assistant will now generate it's own unique webhook url for Mailgun to use. One will need to be generated and provided to Mailgun for incoming messages from Mailgun 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 (api key, domain) still occurs via the configuration.yaml file.

I will update the documentation and unit tests once this gets an initial look. I want to validate that the approach makes sense first.

Related issue (if applicable): Related to #15376

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

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.
},
description_placeholders={
'mailgun_url':
'https://www.mailgun.com/blog/a-guide-to-using-mailguns-webhooks', # pylint: disable=c0301

This comment has been minimized.

@houndci-bot

houndci-bot Oct 15, 2018

line too long (111 > 79 characters)

For more details about this component, please refer to the documentation at
https://home-assistant.io/components/mailgun/
"""
from urllib.parse import urlparse, parse_qsl

This comment has been minimized.

@houndci-bot

houndci-bot Oct 15, 2018

'urllib.parse.parse_qsl' imported but unused

async def async_step_user(self, user_input=None):
"""Handle a user initiated set up flow."""
if self._async_current_entries():

This comment has been minimized.

@rohankapoorcom

rohankapoorcom Oct 15, 2018

Contributor

A lot of this code is directly copied from the IFTTT setup. I would be willing to create a superclass for use for both mailgun and ifttt config flow but I'm not sure if that's the best way of handling this duplication.

Is there any way to share some of the strings? Any value in doing so?

This comment has been minimized.

@balloob

balloob Oct 19, 2018

Member

Good call on generalizing!

We should follow the same strategy as the generalized discovery flow: https://github.com/home-assistant/home-assistant/blob/dev/homeassistant/helpers/config_entry_flow.py#L7

Strings will still be done on a per component basis, which is good as for webhooks it can be all over the place.

This comment has been minimized.

@rohankapoorcom

rohankapoorcom Oct 21, 2018

Contributor

Sounds good - I've done a pass over this and built a new webhook_config_entry_flow. Let me know if I went about this the right way before I write up tests for it.

@rohankapoorcom rohankapoorcom force-pushed the rohankapoorcom:mailgun-webhooks branch from 1103aed to 2326bf2 Oct 16, 2018

@rohankapoorcom rohankapoorcom force-pushed the rohankapoorcom:mailgun-webhooks branch from 2326bf2 to a9a0a79 Oct 16, 2018

@frenck frenck added the docs-missing label Oct 21, 2018

@rohankapoorcom rohankapoorcom force-pushed the rohankapoorcom:mailgun-webhooks branch from 25a0d32 to 1ad5dad Oct 21, 2018

rohankapoorcom added some commits Oct 22, 2018

@rohankapoorcom rohankapoorcom changed the title from WIP: Migrate Mailgun to use the webhook component to Migrate Mailgun to use the webhook component Oct 22, 2018

@@ -0,0 +1,61 @@
"""Helpers for data entry flows for config entries that use webhooks."""

This comment has been minimized.

@balloob

balloob Oct 22, 2018

Member

Can you merge this file with helpers/config_entry_flow.py

@rohankapoorcom

This comment has been minimized.

Contributor

rohankapoorcom commented Oct 23, 2018

I'm thinking we can also delete this unit test: https://github.com/home-assistant/home-assistant/blob/dev/tests/components/ifttt/test_init.py#L41 since that logic no longer exists in the component itself and is tested by the helper tests here: https://github.com/home-assistant/home-assistant/pull/17464/files#diff-f51b5c565fd30e4d50c767f79eda1988R186

@rohankapoorcom

This comment has been minimized.

Contributor

rohankapoorcom commented Oct 23, 2018

Documentation updated and I added a section on this being a breaking change.

Once this is merged in, there should be a follow up PR where we implement the Making It Secure section from the Mailgun guide (this was never previously implemented in Home Assistant).

@rohankapoorcom rohankapoorcom referenced this pull request Oct 23, 2018

Merged

Migrate twilio webhooks to the webhook component #17715

7 of 8 tasks complete
@balloob

This comment has been minimized.

Member

balloob commented Oct 23, 2018

Awesome work.

Yes you're right we can delete that test in IFTTT now. Added that in a commit to your branch.

@balloob balloob merged commit d5a5695 into home-assistant:dev Oct 23, 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.01%) to 93.589%
Details

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

@rohankapoorcom rohankapoorcom deleted the rohankapoorcom:mailgun-webhooks branch Oct 23, 2018

@rohankapoorcom rohankapoorcom referenced this pull request Oct 23, 2018

Merged

Remove webhook_id from yaml config for mailgun #17732

3 of 3 tasks complete

@frenck frenck removed the docs-missing label Oct 25, 2018

@balloob balloob referenced this pull request Nov 9, 2018

Merged

0.82 #18335

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