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

Move more MQTT platforms to config entries #18180

Merged
merged 5 commits into from Nov 6, 2018

Conversation

Projects
None yet
4 participants
@emontnemery
Contributor

emontnemery commented Nov 4, 2018

Description:

#16904, but for this new platform:

  • lock

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox.
async def _async_setup_entity(hass, config, async_add_entities,
discovery_hash=None):
"""Set up the MQTT JSON Light."""

This comment has been minimized.

@OttoWinter

OttoWinter Nov 4, 2018

Contributor

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.

This comment has been minimized.

@emontnemery

emontnemery Nov 4, 2018

Contributor

I've only tested the unit tests, I'll do some "real" testing too.

Edit: Fixed now, thanks!

@emontnemery

This comment has been minimized.

Contributor

emontnemery commented Nov 4, 2018

@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)

This comment has been minimized.

@OttoWinter

OttoWinter Nov 4, 2018

Contributor

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(...)

See

If you want to retrieve a platform that is part of a component, you should
call get_component(hass, 'switch.your_platform'). In both cases the config
directory is checked to see if it contains a user provided version. If not
available it will check the built-in components and platforms.

This comment has been minimized.

@emontnemery

emontnemery Nov 4, 2018

Contributor

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')

This comment has been minimized.

@OttoWinter

OttoWinter Nov 4, 2018

Contributor

Revert this to platform?

This comment has been minimized.

@emontnemery

emontnemery Nov 4, 2018

Contributor

No, can't be reverted due to the limitations in the config entry system as you pointed out.

Show resolved Hide resolved homeassistant/setup.py Outdated
Show resolved Hide resolved tests/components/light/test_mqtt_json.py Outdated
discovery_payload[ATTR_DISCOVERY_HASH])
if discovery_payload['platform'] == 'mqtt_json':
mqtt_json = get_platform(hass, 'light', 'mqtt_json')

This comment has been minimized.

@balloob

balloob Nov 4, 2018

Member

this is wrong. Platforms should not know about other platforms.

We have two choices:

  • don't support mqtt_json with 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__.py and light/mqtt/json.py

This comment has been minimized.

@emontnemery

emontnemery Nov 4, 2018

Contributor

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.

@balloob

balloob approved these changes Nov 4, 2018

@emontnemery emontnemery referenced this pull request Nov 5, 2018

Merged

Mqtt light refactor #18227

3 of 3 tasks complete

@cgarwood cgarwood referenced this pull request Nov 6, 2018

Merged

Z-Wave Lock Config Entry Support #18209

2 of 2 tasks complete

@balloob balloob merged commit 5897649 into home-assistant:dev Nov 6, 2018

5 checks passed

Hound No violations found. Woof!
WIP Legacy commit status override — see details
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.0009%) to 93.062%
Details

@wafflebot wafflebot bot removed the in progress label Nov 6, 2018

zxdavb added a commit to zxdavb/home-assistant that referenced this pull request Nov 13, 2018

Move more MQTT platforms to config entries (home-assistant#18180)
* Move Lock MQTT platform to config entries

* Move MQTT JSON Light platform to config entries

* Review comments

* Review comments

* Revert mqtt_json changes

@balloob balloob referenced this pull request Nov 29, 2018

Merged

0.83 #18776

@emontnemery emontnemery deleted the emontnemery:mqtt_discovery_info branch Dec 2, 2018

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