Add Gotify component #53050
Add Gotify component #53050
Conversation
|
Hi @benjmarshall, It seems you haven't yet signed a CLA. Please do so here. Once you do that we will be able to review and accept this pull request. Thanks! |
Co-authored-by: Milan Meulemans <milan.meulemans@live.be>
Simplify the ConfigFlow
Migrate component to Config Flow
|
@milanmeu you mentioned previously that I should change the way that the platform setup is done. Could you open a PR or provide some guidance on how I should be doing it? Thanks. |
…ific notify service. Add intiial support for multiple entries.
|
I've updated this so that it should now correctly load the Notification platform and expose a "notify.your_gotify_entry" service. The major outstanding issue with this integration is that multiple entries will not work correctly. The is due to the underlying library only exposing a bunch of functions, and using global variables for host/application ID etc, rather than defining a class which can hold a proper configuration for a specific host/application id. I might be able to hack around this by always re-configuring the host/application id with each message to be sent but this is messy. As a better alternative I have opened a PR on the underlying library which converts it to using a class. That way each specific GotifyNotificationService would be able to properly hold it's own configuration. |
…tion to use client token to allow the creationg of a new gotify application if required. Re-work to allow multiple config entires to exist. Generate entry name from gotify application name instead of user supplied name.
|
Hi All, I've re-worked this component a little today on the basis of comments from @bdraco. First up the initial configuration has been re-worked to use a Gotify client token, instead of an app token. This allows access to more API endpoints which allows the flexibility to get application details and create new applications if required. An application token can still be optionally supplied in the config. If supplied then a config entry will be created for the Gotify application that corresponds to this token. If no application token is supplied then a new Gotify application is created on the Gotify server. The name of the config entry is now generated based on the name of the Gotify application (either created or determined by the supplied app token). Because the name of the config entry is used to create a notify service this needs to be unique. I have added a check to make sure that a unique config entry name is used. A postscript integer (starting with 2) will be appended if the Gotify application would not create a unique config entry name. These changes should address all of the comments from @bdraco. Additionally these changes allow for multiple config entries to co-exist gracefully. Due to the fact I have changed more than strictly the suggested changes by @bdraco it would be beneficial to have a further review by the previous comments @milanmeu @TheZoker. These changes will also require an update to the documentation which I will push soon. |
|
There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days. |
|
Bump for bot (because AFAICT this is again waiting on the Home Assistant team) |
|
I'll bump this as well... Even though because its taken so long I had to jump over to matrix as my notifier i would still like to see this implemented. |
|
If it helps at all, I've been using this as a custom component for a few months now and it has been rock solid. |
|
Any chance of a quick primer on how to do that, @jackwilsdon? Would love to run this. I was hoping it would be on HACS if not here yet. |
| raw_name = DOMAIN + "_" + name.replace(" ", "_").lower() | ||
|
|
||
| if DOMAIN in hass.data: | ||
| entry_names = [] |
There was a problem hiding this comment.
I think this needs moving outside of this if statement (or the while loop needs moving inside of it), as otherwise this function errors out if the gotify domain doesn't exist.
There was a problem hiding this comment.
Yea, this is a bug for sure. Since the lower half of the function doesn't do anything really if entry_names is empty, I'd recommend a short circuit exit.
if DOMAIN not in hass.data:
return name
|
@Hukuma1 sure:
If you have any issues, feel free to message me on the Home Assistant Community (either using this direct link or via my profile) as I don't wish to start any unrelated discussions on this PR. |
| discovery_info = {CONF_NAME: entry.data[CONF_NAME], "entry_id": entry.entry_id} | ||
| await hass.async_create_task( | ||
| discovery.async_load_platform( | ||
| hass, "notify", DOMAIN, discovery_info, hass.data[DOMAIN] |
There was a problem hiding this comment.
The string, notify, should be imported from homeassistant.const.Platform.
Eg.
from homeassistant.const import Platform
Platform.NOTIFY| async def async_unload_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool: | ||
| """Unload a config entry.""" | ||
| hass.services.async_remove("notify", entry.data[CONF_NAME]) | ||
| hass.data[DOMAIN].pop(entry.entry_id) | ||
| return True |
There was a problem hiding this comment.
Now the standard pattern here is
async def async_unload_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool:
"""Unload a config entry."""
if unload_ok := await hass.config_entries.async_unload_platforms(entry, PLATFORMS):
hass.data[DOMAIN].pop(entry.entry_id)
return unload_okAgain, what you have probably works, but I'm not sure if updating to the new methods is required for merging.
There was a problem hiding this comment.
Same as above the notify platform doesn't support config entries.
| raw_name = DOMAIN + "_" + name.replace(" ", "_").lower() | ||
|
|
||
| if DOMAIN in hass.data: | ||
| entry_names = [] |
There was a problem hiding this comment.
Yea, this is a bug for sure. Since the lower half of the function doesn't do anything really if entry_names is empty, I'd recommend a short circuit exit.
if DOMAIN not in hass.data:
return name| for entry in hass.data[DOMAIN]: | ||
| entry_names.append(hass.data[DOMAIN][entry].data[CONF_NAME]) |
There was a problem hiding this comment.
As this is a dictionary and you only seem to care about the values, you can instead iterate over hass.data[DOMAIN].values() directly.
You can also simplify this using list comprehension and even avoid empty list initialization.
entry_names = [entry.data[CONF_NAME] for entry in hass.data[DOMAIN].values()]| name = raw_name | ||
| i = 2 | ||
| while name in entry_names: | ||
| name = raw_name + str(i) | ||
| return name |
There was a problem hiding this comment.
This will cause an infinite loop if the same name exists since you never remove the name from the entry_names list.
You're probably better off filtering by name in the list above and then using it's length here.
entry_names = [entry.data[CONF_NAME] for entry in hass.data[DOMAIN].values() if entry.data[CONF_NAME] == name]
name = raw_name + str(len(entry_names))
That said, we're not using the actual name values at all, so it could be simplified further.
def sanitise_name(hass: HomeAssistant, name: str) -> str:
"""Make sure we are not going to create and entry with a duplicate name as this would produce duplicate services."""
num_existing_names = 1
for entry in hass.data.get(DOMAIN, {}).values():
if entry.data[CONF_NAME] == name:
num_existing_names += 1
name = DOMAIN + "_" + name.replace(" ", "_").lower()
if num_existing_names == 1:
return name
return f"{name}_{num_existing_names}"| return_name = "" | ||
|
|
||
| try: | ||
| current_applications = await hass.async_add_executor_job( |
There was a problem hiding this comment.
It looks like this is only used in one block below. It can go within the if CONF_TOKEN in user_input: block.
| if user_input[CONF_TOKEN] == app.get("token"): | ||
| return_token = user_input[CONF_TOKEN] | ||
| return_name = app.get("name") | ||
| if not (CONF_TOKEN in user_input) or not return_token: |
There was a problem hiding this comment.
You could drop not (CONF_TOKEN in user_input) and only check not return_token here as return_token is initialized as a falsy value.
| ) as mock_setup_entry: | ||
| result2 = await hass.config_entries.flow.async_configure( | ||
| result["flow_id"], | ||
| {"host": "https://1.1.1.1", "token": "test-token", "name": "test"}, |
There was a problem hiding this comment.
Minor nit: 1.1.1.1 is a real public IP address. example.com is reserved for these purposes.
|
|
||
| result2 = await hass.config_entries.flow.async_configure( | ||
| result["flow_id"], | ||
| {"host": "1.1.1.1", "token": "test-token", "name": "test"}, |
There was a problem hiding this comment.
This is a semantic thing, but technically this is a host address, though it is not a URI. https://www.sistrix.com/ask-sistrix/technical-seo/site-structure/what-is-the-difference-between-a-url-domain-subdomain-hostname-etc
|
There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days. |
|
Bump |
|
As anyone still working on this PR (especially @benjmarshall)? |
|
I don't think anyone is actively working on this. Whilst I am using it on my install as a custom component I am not actively developing it as it provides the functionality I need already. I have pretty much given up trying to get it merged as no one from the core team with permissions to do so seemed interested. I am sympathetic as I imagine they would rather focus on their integrated mobile app notifications.
The component still works for me so if you have the knowledge it can be installed manually as a custom component. I should however say that my install is behind the latest Home Assistant by a bit now so no guarantees on compatibility with the latest release(s).
13 May 2023 08:31:31 Fabian Winter ***@***.***>:
… As anyone still working on this PR (especially @benjmarshall[https://github.com/benjmarshall])?
—
Reply to this email directly, view it on GitHub[#53050 (comment)], or unsubscribe[https://github.com/notifications/unsubscribe-auth/ABQEWSFUEKUT2ON4XZGVVH3XF42E5ANCNFSM5ANAOVHA].
You are receiving this because you were mentioned.[Tracking image][https://github.com/notifications/beacon/ABQEWSC2TZWCJ3QCGC36A2TXF42E5A5CNFSM5ANAOVHKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOLQXQOXY.gif]
|
|
I've marked this PR as draft, as changes are requested that need to be processed first before we can do another review. Thanks! 👍 |
|
Because there hasn't been any activity on this PR for quite some time now, I've decided to close it for being stale. Feel free to re-open this PR when you are ready to pick up work on it again 👍 ../Frenck |
Proposed change
Add a new platform for the notify component. Gotify is an open source, self-hosted push notification server.
Type of change
Additional information
Checklist
black --fast homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all..coveragerc.The integration reached or maintains the following Integration Quality Scale:
To help with the load of incoming pull requests: