Conversation
probot-home-assistant
Bot
added
core
integration: config
small-pr
|
Hey there @home-assistant/core, mind taking a look at this pull request as its been labeled with a integration ( |
frenck
changed the title
frenck
changed the title
|
I think that offering a second type of ignoring might lead to a confusing user experience. I much rather enforce config entry unique ID for things that are discoverable and upgrade existing flows that don't leverage unique ID. Quick search: (note, discovery flow based config entries don't set unique ID right now but working on a PR) Config flow with discovery that sets unique ID
Config flow with discovery that do not set unique ID
Detailsfrom pathlib import Path
import json
INT_DIR = Path("homeassistant/components")
DISCOVERY_KEYS = {"ssdp", "zeroconf"}
def main():
wd_has_id = []
wd_has_no_id = []
has_id = []
has_no_id = []
for fil in INT_DIR.glob("*/manifest.json"):
manifest = json.loads(fil.read_text())
if not manifest.get("config_flow"):
continue
has_discovery = any(key in manifest for key in DISCOVERY_KEYS)
config_flow = (fil.parent / "config_flow.py").read_text()
name = f"{manifest['name']} ({manifest['domain']})"
sets_unique = (
"self.async_set_unique_id" in config_flow
or "config_entry_flow.register_discovery_flow" in config_flow
)
if has_discovery and sets_unique:
wd_has_id.append(name)
elif has_discovery:
wd_has_no_id.append(name)
elif sets_unique:
has_id.append(name)
else:
has_no_id.append(name)
print("Config flow with discovery that sets unique ID")
for name in wd_has_id:
print(f" - {name}")
print()
print("Config flow with discovery that does not set unique ID")
for name in wd_has_no_id:
print(f" - {name}")
print()
print("Config flow that sets unique ID")
for name in has_id:
print(f" - {name}")
print()
print("Config flow that does not set unique ID")
for name in has_no_id:
print(f" - {name}")
print()
main() |
|
Unique ID for fritzbox is also on the way: #34716 |
|
Awesome @balloob! Let's move that route, saw the discovery flow based config entries fix for it. So maybe indeed, we can consider requiring a unique_id for config flows with discovery after all. Seems like the little script you posted there, is already half the Hassfest check 🎉 Closing this 🕺 |
@frenck is there some additional info to read about why this is not a good idea? I'd expect it works with hostname instead of mac for esphome, as esp devices can be repurposed by a firmware upgrade (not all esp devices are plugs or lights all their life) It'd be nice for a discovery to show up if a new node name comes on, so long you can ignore it once for all, I don't see a big bummer. |
|
Never mind just found out you're going to use node name as unique id 👍 can't get used to that discord thing, I'm so lonely here |
5 participants
Proposed change
Context
More integrations move towards configuration to the UI. In a lot of cases discovery using HomeKit, SSDP or ZeroConf is added. Which is awesome!
However, a lot of devices don't provide a
unique_id, which removed the ability to ignore such a discovered integration, such as IKEA Tradfri or ESPHome.This is annoying towards our users. Some recent example issues raised: #34683 #34561.
Goal
This PR aims to provide a method to disable discovery for integration altogether if the found discovered item doesn't provide a unique identifier.
Reasoning
We could implement a unique ID in all config flow... however, that is not a reality as not all devices provide such means for a unique identifier (e.g., ESPHome & IKEA Trafri).
Some suggestions done: Use the MAC address based on IP. However, that would cause issues when using different subnets/VLAN's since you won't get the correct mac.
Other suggestions: Use the hostname. While this might work for the IKEA example, for the ESPHome example, this is configurable/changeable by the user.
Furthermore, we don't require a unique ID to be set. This makes sense, if it a device won't have it... it would exempt those from using a configuration flow.
So that leaves the user, with a discovery he can't ignore. Which we should solve.
Current status
This PR implements a simple change that, when a discovery is ignored, will use the unique ID to create a config entry with the ignored source, but set the unique ID to None when the unique ID is missing.
I also have the matching frontend change ready for this.
Basic frontend change:
(some more changes here, to make the confirmation dialogs distinctive and probably change the "ignore" button to "ignore all" in these cases).
Missing in this implementation
Currently missing in the part where inside the core we detect this and trigger the abort for the configuration flow. As we should implement this in our core flow context and not be dependent on the integration flow itself. I've been struggling to find a good location for this.
Basically we need to abort the config flow from the core, if:
Type of change
Example entry for
configuration.yaml:# Example configuration.yamlAdditional 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: