Rework MQTT config merging and adding defaults #90529
Conversation
home-assistant
bot
added
cla-signed
core
integration: mqtt
small-pr
|
Hey there @emontnemery, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
| entry = hass.config_entries.async_entries(mqtt.DOMAIN)[0] | ||
| entry.data = {"broker": "test-broker", "port": 1234} | ||
| await mqtt_mock_entry_no_yaml_config() | ||
| await hass.async_block_till_done() | ||
|
|
||
| # Config entry data should now be updated |
There was a problem hiding this comment.
We don't update the entry data anymore to mix missing items. Defaults are added to the loaded entry date using the schema. For an invalid config entry the user can correct the settings in the UI.
| @@ -183,15 +183,14 @@ async def test_user_connection_works( | |||
| assert result["type"] == "form" | |||
|
|
|||
| result = await hass.config_entries.flow.async_configure( | |||
| result["flow_id"], {"broker": "127.0.0.1", "advanced_options": False} | |||
There was a problem hiding this comment.
"advanced_options" was filtered out, that was why tests pass, it should not be a setting in the entry, and in real live this setting was never added to the MQTT config entry data.
| ) | ||
|
|
||
| assert result["type"] == "create_entry" | ||
| assert result["result"].data == { | ||
| "broker": "127.0.0.1", | ||
| "port": 1883, | ||
| "discovery": True, | ||
| "discovery_prefix": "homeassistant", |
There was a problem hiding this comment.
Default settings are not added to the entry by default, they are added during entry load by applying the schema.
| @@ -169,9 +158,11 @@ | |||
| CONFIG_SCHEMA_ENTRY = vol.Schema( | |||
There was a problem hiding this comment.
In a follow-up, I think we can drop the schema? There's no reason to validate config entry options / data?
There was a problem hiding this comment.
The schema adds default values. If we want to drop the schema we need to change the config flow to save basic broker settings with broker CONFIG_SCHEMA_ENTRY. So I don't see an option to drop it yet.
There was a problem hiding this comment.
I understand, but there's no need to add default values via a schema, just use dict.get instead and pass the wanted default value.
There was a problem hiding this comment.
So let's implement this in a follow up PR
| """Return typed MqttData from hass.data[DATA_MQTT].""" | ||
| mqtt_data: MqttData | ||
| if ensure_exists: |
There was a problem hiding this comment.
Instead of just removing the check, maybe raise instead:
if DATA_MQTT not in hass.data:
raise HomeAssistantErrorOr maybe the KeyError is good enough?
There was a problem hiding this comment.
KeyError should be enough I think
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
Proposed change
Rework the MQTT config processing and remove not needed check since we do not import broker settings from
configuration.yamlanymore.Further changes:
CONFIG_SCHEMA_ENTRYnow takes care of adding the defaults as they are missing and validates the entry data at entry startup.ConfigEntryError, this will set the config entry in error allowing the user to correct the entry settings.CONFIG_SCHEMA_BASEschema used for yaml based settings was a left over and is removed now.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.To help with the load of incoming pull requests: