Migrate Mailgun to use the webhook component#17464
Migrate Mailgun to use the webhook component#17464balloob merged 8 commits intohome-assistant:devfrom
Conversation
ghost
added
the
There was a problem hiding this comment.
line too long (111 > 79 characters)
There was a problem hiding this comment.
'urllib.parse.parse_qsl' imported but unused
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
force-pushed
the
mailgun-webhooks
branch
from
1103aed to
2326bf2
Compare
rohankapoorcom
force-pushed
the
mailgun-webhooks
branch
from
2326bf2 to
a9a0a79
Compare
rohankapoorcom
force-pushed
the
mailgun-webhooks
branch
from
25a0d32 to
1ad5dad
Compare
rohankapoorcom
changed the title
| @@ -0,0 +1,61 @@ | |||
| """Helpers for data entry flows for config entries that use webhooks.""" | |||
There was a problem hiding this comment.
Can you merge this file with helpers/config_entry_flow.py
|
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 |
|
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). |
|
Awesome work. Yes you're right we can delete that test in IFTTT now. Added that in a commit to your branch. |
6 participants
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:
tox. Your PR cannot be merged unless tests passIf user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
REQUIREMENTSvariable (example).requirements_all.txtby runningscript/gen_requirements_all.py..coveragerc.If the code does not interact with devices: