Let settings be changed without tearing the integration out - #36
Merged
Merged
Conversation
There was a setup dialog and nothing else, so changing the port, the upstream, the nameserver, the flow rate, the firmware policy or the check interval meant deleting the integration and adding it back. That is fine for an install configured once and left alone, and it is exactly the wrong answer for someone who has just typed one field wrong. Adds a reconfigure step that reopens the same form filled in with what is configured, then saves and reloads the entry so the new values are actually in force. One dialog for all six rather than the conventional split between a reconfigure step for the connection and an options menu for the rest: every setting here is read once at setup and needs the same reload, so the split would only make someone guess which of two menus holds the field they got wrong. The port is checked before anything is written, because saving first and finding out at reload leaves the entry dead and the listener gone. It is checked only when it changed: our own listener is holding the configured port, so probing it every time would report every port taken and quietly make the whole dialog useless. The probe binds the wildcard address the way the listener does, which also happens to be the only way it is portable -- macOS lets a wildcard bind sit alongside a loopback one, so a loopback-only squatter is not a clash at all. Fixes a leak the same dialog would have made easy to hit. A port that could not be bound threw a raw OSError out of async_setup_entry, and because nothing was registered with Home Assistant yet, unloading never ran: the half-started runner, the upstream session and the entry's slot in hass.data were all left behind, and every retry added another set. Now it cleans up and raises ConfigEntryNotReady, so a port that frees up is picked up on a retry instead of needing a restart. Tests cover the form arriving filled in, a change taking effect in the running integration rather than only on disk, a busy port refused with the old listener still serving, an unchanged port not mistaken for a clash, a reload genuinely releasing and rebinding the socket, and setup after a failed bind leaving nothing behind. The reconfigure form is held to the same serialisation check as the setup form, which is the only thing standing between a schema change and a dialog that 500s. Refs #24, which keeps the capture-to-disk toggle and the staleness window.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
There was a setup dialog and nothing else, so changing the port, the upstream,
the nameserver, the flow rate, the firmware policy or the check interval meant
deleting the integration and adding it back. That is fine for an install
configured once and left alone. It is exactly the wrong answer for someone who
has just typed one field wrong, which is the person most likely to need it.
Adds a reconfigure step that reopens the same form filled in with what is
configured, then saves and reloads the entry so the new values are actually in
force. One dialog for all six settings rather than the conventional split
between a reconfigure step for the connection and an options menu for the rest:
every setting here is read once at setup and needs the same reload, so the
split would only make someone guess which of two menus holds the field they got
wrong.
The port is checked before anything is written, because saving first and
finding out at reload leaves the entry dead and the listener gone. It is
checked only when it changed: our own listener is holding the configured port,
so probing it every time would report every port taken and quietly make the
whole dialog useless. The probe binds the wildcard address the way the listener
does, which is also the only way it is portable. macOS lets a wildcard bind sit
alongside a loopback one, so a loopback-only squatter is not a clash at all,
and a probe bound to loopback would disagree with the listener on which ports
are free.
The leak this would have made easy to hit
A port that could not be bound threw a raw
OSErrorout ofasync_setup_entry. Nothing was registered with Home Assistant at that point,so unloading never ran and could not clean up: the half-started runner, the
upstream session and the entry's slot in
hass.datawere all left behind, andevery retry added another set. It now cleans up and raises
ConfigEntryNotReady, so a port that frees up is picked up on a retry ratherthan needing a restart of Home Assistant.
Only reachable at first install before this change. Once a dialog can change
the port, any typo reaches it.
Verified on a real instance, not only in tests
Against a throwaway Home Assistant container running this branch:
with the live values. No 500, and every label, help string and radio option
renders from
translations/en.json.inside that container, is refused inline with the error text. The field keeps
what was typed rather than silently reverting, and nothing is written.
through to the upstream. The stored entry carries the new value.
Tests
223 passing, up from 213. New coverage: the form arriving filled in, a change
taking effect in the running integration rather than only on disk, a busy port
refused with the old listener still serving, an unchanged port not mistaken for
a clash, a reload genuinely releasing and rebinding the socket, and setup after
a failed bind leaving nothing behind. The reconfigure form is held to the same
serialisation check as the setup form, which is the only thing standing between
a schema change and a dialog that 500s.
Refs #24, which keeps the capture-to-disk toggle and the staleness window. The
staleness window is worth a look before anyone builds it: the 7 minute
threshold lives only in the dashboard's Jinja, and nothing in the integration
reads a staleness setting at all, so an options field for it would presently
wire to nothing.