Move more MQTT platforms to config entries #18180
Conversation
ghost
added
the
|
|
||
| async def _async_setup_entity(hass, config, async_add_entities, | ||
| discovery_hash=None): | ||
| """Set up the MQTT JSON Light.""" |
There was a problem hiding this comment.
Have you tested this? I believe the setup entry will never be called because the config entry is forwarded to the platform with the same name as the component (mqtt).
So all light.mqtt_json payloads will be redirected to light.mqtt.
That's a limitation of the config entry system and we should not change the config entry core just to work around this quirk.
There's been some discussion before (can't find the link rn), but the conclusion was that the light.mqtt platform would need to redirect the entity setup to mqtt_json if the platform doesn't match.
There was a problem hiding this comment.
I've only tested the unit tests, I'll do some "real" testing too.
Edit: Fixed now, thanks!
|
@OttoWinter Updated, I hope it's what you intended. |
| from homeassistant.components.light.mqtt_json import ( | ||
| PLATFORM_SCHEMA as PLATFORM_SCHEMA_JSON) | ||
| from homeassistant.components.light.mqtt_json import ( | ||
| _async_setup_entity as _async_setup_entity_json) |
There was a problem hiding this comment.
Maybe consider using the get_platform import instead of this absolute one to support loading custom_components. I don't know how much this is enforced, but it would be best to call:
# in async_discover
if discovery_payload['platform'] == 'mqtt_json':
mqtt_json = get_platform(hass, 'light', 'mqtt_json')
config = mqtt_json.PLATFORM_SCHEMA_JSON(discovery_payload)
await mqtt_json._async_setup_entity_json(...)There was a problem hiding this comment.
Great, thanks!
Edit: Implemented as you suggested.
| @@ -244,15 +245,15 @@ | |||
| hass, component, platform, payload, hass_config) | |||
| return | |||
|
|
|||
| config_entries_key = '{}.{}'.format(component, platform) | |||
| config_entries_key = '{}.{}'.format(component, 'mqtt') | |||
There was a problem hiding this comment.
No, can't be reverted due to the limitations in the config entry system as you pointed out.
| discovery_payload[ATTR_DISCOVERY_HASH]) | ||
|
|
||
| if discovery_payload['platform'] == 'mqtt_json': | ||
| mqtt_json = get_platform(hass, 'light', 'mqtt_json') |
There was a problem hiding this comment.
this is wrong. Platforms should not know about other platforms.
We have two choices:
- don't support
mqtt_jsonwith config entries - move MQTT JSON into the MQTT platform and add a config to switch to "mode". To keep code manageable we could move the files so we have
light/mqtt/__init__.pyandlight/mqtt/json.py
There was a problem hiding this comment.
Hmm, too bad, I'll remove the mqtt_json changes from this PR.
There's a third MQTT light variant. mqtt_template, which has the same issue then.
When you say, "move MQTT JSON into the MQTT platform", does that affect the configuration.yaml entry?
light:
- platform: mqtt_json
command_topic: "home/rgb1/set"light:
- platform: mqtt_template
command_topic: "home/rgb1/set"
command_on_template: "on"
command_off_template: "off"Edit: As explained by @balloob on the chat, yes, that would affect the configuration and be a breaking change.
Description:
#16904, but for this new platform:
Checklist:
tox.