-
-
Notifications
You must be signed in to change notification settings - Fork 38.3k
[codex] WLED: use repo-aware release checks for upgrades #172481
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
Open
LordMike
wants to merge
5
commits into
home-assistant:dev
Choose a base branch
from
LordMike:update-wled-custom-fw
base: dev
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
f6e3337
WLED: use repo-aware release checks for upgrades
LordMike c470f57
WLED: normalize repo keys for release cache
LordMike df068c2
Merge remote-tracking branch 'origin/dev' into update-wled-custom-fw
LordMike 1b2e9f3
WLED: track release repositories in one coordinator
LordMike 848e5c4
Merge branch 'dev' into update-wled-custom-fw
LordMike File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| """Tests for WLED release coordination.""" | ||
|
|
||
| from unittest.mock import AsyncMock, MagicMock, call, patch | ||
|
|
||
| from homeassistant.components.wled import WLED_KEY, async_get_releases_coordinator | ||
| from homeassistant.components.wled.coordinator import normalize_repo | ||
| from homeassistant.core import HomeAssistant | ||
|
|
||
|
|
||
| def test_normalize_repo() -> None: | ||
| """Test repo normalization.""" | ||
| assert normalize_repo(None) == "wled/WLED" | ||
| assert normalize_repo("") == "wled/WLED" | ||
| assert normalize_repo(" LordMike/Wled ") == "LordMike/Wled" | ||
|
|
||
|
|
||
| async def test_release_coordinator_is_cached_per_repo(hass: HomeAssistant) -> None: | ||
| """Test release coordinators are deduplicated by repository.""" | ||
| hass.data[WLED_KEY] = {} | ||
|
|
||
| created: list[MagicMock] = [] | ||
|
|
||
| def _create_coordinator(*args: object, **kwargs: object) -> MagicMock: | ||
| coordinator = MagicMock() | ||
| coordinator.async_request_refresh = AsyncMock() | ||
| created.append(coordinator) | ||
| return coordinator | ||
|
|
||
| with patch( | ||
| "homeassistant.components.wled.WLEDReleasesDataUpdateCoordinator", | ||
| autospec=True, | ||
| side_effect=_create_coordinator, | ||
| ) as mock_coordinator: | ||
| first = await async_get_releases_coordinator(hass, "LordMike/Wled") | ||
| second = await async_get_releases_coordinator(hass, "LordMike/Wled") | ||
| default = await async_get_releases_coordinator(hass, None) | ||
|
|
||
| assert first is second | ||
| assert first is not default | ||
| assert created == [first, default] | ||
| assert mock_coordinator.call_args_list == [ | ||
| call(hass, repo="LordMike/Wled"), | ||
| call(hass, repo="wled/WLED"), | ||
| ] | ||
| assert first.async_request_refresh.await_count == 1 | ||
| assert default.async_request_refresh.await_count == 1 |
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
Oops, something went wrong.
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 problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit problematic because this coordinator is never stopped when a user stops using the fork.
I'm wondering if it would be possible to update the coordinator to check which repositories the user is using and download data for all the repositories the user is currently using.
What do you think about this?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another solution is to register the repository in the release coordinator in
async_setup_entryand then unregister it in theasync_unload_entryfunction.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I see it though, the release coordinator remains in memory, yes (and we could definitely prune that) - but we're not actively polling the repo there. So if you add a device A, with repo A - and remove device A. Then repo A remains, but is not polled. The DataUpdateCoordinator base operates with listeners (each update entity listens), and when the last listener disappears, which it hopefully does, DataUpdateCoordinator stops polling.
I'll see if we can't clean out the dict regardless.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you on to making WLEDReleasesDataUpdateCoordinator able to manage multiple repositories in one instance?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I looked into blindly going for entry setup/teardown, but it would introduce extra bookkeeping. On entry_setup ( a single device), we create the coordinator if not existing. But on teardown, we'd have to start reviewing if the coordinator is still in use.
It seems unlikely to be a problem, leaving hte coordinator around after the device has disappeared. It only lasts till next restart of the HA instance. And it really is only 1 instance per unique repository - so in practice its like.. at most 2..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, a drawback here is that:
I'm partial to the one instance per repo instead, keep it lean.. but then try to solve for cleanup - either the temporary bit like disabling the update entity, or the more permanent bit that is removing a WLED device (entry) entirely. It could be solved by a set of entry_ids in each release coordinator, and when that set is empty, we remove it.
The release coordinator already tracks subscriptions, which each update entity uses. We could in our WLED release coordinator, expose the count of subscriptions, and if after removal this becomes 0, we clean up the instance entirely. Hopefully we can then tweak disabling of update entities to unsubscribe, which in turn will make stuff auto-cleanup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is something on .. what if we have 2 wled devices (same repo), and we do a force update on all their update entities at the same time. We will then do two forceful data updates, which means two queries for releases, parsing of those, paginating those, and so on..
If you had 10 or 100 wled devices, we might hit GH rate limits, even.
... something like a time limit on rechecks? "minimum 60s since last check" -style.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can add better error handling similarly to what is done in the system monitor integration.
core/homeassistant/components/systemmonitor/coordinator.py
Lines 189 to 200 in 840243d
I think this is a minor issue because at most the user will have more up-to-date data.
I think that the coordinator debouncer will work here, it will prevent the same data from being downloaded multiple times in a short period of time.
I'm not sure what that would look like, but we could try to implement it. I proposed my solution because it sounded relatively simple to me. It requires the least amount of code changes to get it working now. I will open a PR with my proposal so that we can see what it looks like in full and then we can compare.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I created a draft PR: mik-laj#1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I'll move closer to a single instance of update coordinator, and then tracking repos in it.