home-assistant / architecture

Repo to discuss Home Assistant architecture
317 stars 99 forks source link

Ability to edit/update config entries #377

Closed jjlawren closed 1 year ago

jjlawren commented 4 years ago

Context

With the move away from YAML configuration for integrations, there's a clear need for the ability to allow the user to edit/update config entries without touching .storage/ by hand. Config entries can be updated from within the integrations today, but the exposed workflow for this process needs polish.

Besides the user-initiated decisions to edit an existing integration's config, there's also no good way for the integration to mark itself as failed (e.g., auth credentials invalidated). The integration can begin a new config flow to update the existing by reusing the same entry_id, but it will show up as a duplicated "Discovered" integration in the UI.

Summary of known problems:

  1. There's no obvious interface to start a config entry "edit".
  2. No clean way to complete an interactive update of a config entry. Neither async_abort nor async_create_entry seem applicable to this use case. ConfigFlow_abort_if_unique_id_configured can be used as a convenience, but it will tell the user the setup has been aborted which is confusing.
  3. "Update" flows for failed config entries that are initiated by the backend are marked as "Discovered".
  4. Related to point 3, if a user begins and cancels an integration-initiated "update" flow then it will disappear. This can leave the config entry in a broken state with no obvious indication.

Proposal

Some point-by-point suggestions for the problems above:

  1. Add a new standard step to enter the config flow (SOURCE_EDIT) which can be called either by the UI or from the integration during runtime.
  2. Add a new helper (i.e., ConfigFlow._update_if_unique_id_configured) which will update an existing config entry. Question: should this also unload/reload a failed entry?
  3. Add a new context parameter to the config entry which indicates that it requires attention. This could allow the entry to be shown at the top of the integration page with a "Needs Attention" label and highlighted in red. This could also change the "Configure" button to redirect to the SOURCE_EDIT step mentioned in point 1.
  4. If we mark the config entry instead of creating a new flow, then it will be:
    • Persistent in case the user aborts during reconfiguration
    • Won't show a duplicate in the Integrations page
    • Allows us to keep the delete/options/etc buttons exposed for the already-configured integration

Consequences

Editing/updating existing config entries should become more usable to the user and more obvious on how to implement to the maintainers. The benefit of easy editability of YAML with the benefits of runtime validation for config flows can both be realized when using the config entry model.

SeanPM5 commented 4 years ago

Was asked to post the mockup I shared on Discord a few days ago. Integrations in a fail state could be clearly marked as such with a red background and shown first above anything else. There'd be a button to fix it, and an overflow menu (3 dots) that would let you delete if necessary.

mockup

cgarwood commented 4 years ago

Agreed. Something like this is definitely needed.

Jc2k commented 4 years ago

I think something like @SeanPM5 posted would be great (👍 for that), but thats probably seperate to a discussion about editing config entries as yaml?

bdraco commented 4 years ago

I'm thinking this could be a bit more generic call to action. Hopefully the integration could set the action string and button title.

For HomeKit instead of "Credentials expired [Reauthorize]"

"Pairing Required [Pair]"

balloob commented 4 years ago

I think that we can solve this using the UI and existing infrastructure.

bdraco commented 4 years ago

Since not all entries have a unique id, I think passing in the config entry id would make sense to start updating a flow. I expect every integration would have the config entry id since the failure they are trapping or seeing would be inside the setup which knows it.

jjlawren commented 4 years ago

I think that we can solve this using the UI and existing infrastructure.

  • Whenever a discovered config flow has the same unique ID as an existing config entry, we know it's an update flow and can call it out (color it differently).

What about for point 4? If the user aborts an update flow for a failed config entry without completing, the flow would be removed from the UI and the integration would remain in a broken state without an obvious indication. Having a persistent card for a config entry that needs attention would be good for usability. It would stay there until either the update flow is successfully completed or if the user deletes the existing broken config entry.

balloob commented 4 years ago

I think that we should require a unique ID for updating a config entry. How else will we be able to refer to it?

I've added step_id to the flows in progress endpoint (https://github.com/home-assistant/core/pull/35272). This will allow us to load translations based on the step ID.

For aborting the flow, let's make sure that's not allowed :) Maybe add something to context? allow_abort = boolean

bdraco commented 4 years ago

I think that we should require a unique ID for updating a config entry. How else will we be able to refer to it?

I'm probably missing the big picture on this, but is there a case where you want to update the config entry from outside of the integration where you won't know the entry.entry_id?

balloob commented 4 years ago

Very good point 😆

teharris1 commented 4 years ago

Couldn't we use the OptionsFlow class for this? If OptionsFlow has an async_update_entry method it seems to me most of what is discussed in this thread becomes possible. Not all integrations make sense for reconfiguration so the reconfigure option would only be there if it is needed.

bdraco commented 4 years ago

Maybe a new step in the config flow async_step_reconfig that re-runs async_step_user with the existing data filled in which would lead to an update instead of create.

bdraco commented 4 years ago

It might be nice to raise ConfigEntryNeedsReconfig and start the config flow automatically.

balloob commented 4 years ago

I think that the approach that MQTT is taking is fine as a solution for now: it uses an options flow and it will update data in the config entry before finishing the options flow with a create entry: https://github.com/home-assistant/core/pull/36537

jameshilliard commented 4 years ago

Supporting reconfiguration of devices without unique ID's would be helpful for certain types of devices such as the hlk-sw16 which is a 16 switch device that does not have hardware backed unique ID's at all. Being able to move the device to a different IP/port without reconfiguration of the 16 switch entities would be quite helpful.

I think that the approach that MQTT is taking is fine as a solution for now: it uses an options flow and it will update data in the config entry before finishing the options flow with a create entry: home-assistant/core#36537

Would this approach be useable for hardware like the hlk-sw16?

bdraco commented 4 years ago

Here is another approach https://github.com/home-assistant/core/pull/38341/files

jameshilliard commented 4 years ago

Here is another approach https://github.com/home-assistant/core/pull/38341/files

I think the main issue with that approach is that it's not really possible to trigger the reauth flow manually. For devices like the hlk-sw16 that don't use authentication the reconfigure would have to be manually triggered from my understanding.

frenck commented 1 year ago

This architecture issue is old, stale, and possibly obsolete. Things changed a lot over the years. Additionally, we have been moving to discussions for these architectural discussions.

For that reason, I'm going to close this issue.

../Frenck