Use device-reported repository for upgrades#2078
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds device-reported firmware repository support: Info gains ChangesFirmware Repository Configuration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRsSuggested labelsbugfix Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/wled/wled.py`:
- Around line 686-687: The current fallback only checks for None when assigning
repo from self._device.info.repo, which allows empty or whitespace strings to be
used and later produces invalid GitHub URLs; update the assignment logic in the
code path that sets repo (the repo variable and usage of self._device.info.repo)
to normalize/trim the device-provided value and treat any falsy or blank (e.g.,
"", whitespace-only) value as missing, falling back to DEFAULT_REPO instead;
ensure you explicitly strip whitespace from self._device.info.repo before
testing and then assign DEFAULT_REPO when the stripped value is empty.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 91682953-0cb0-4e27-be48-2b1800e1c759
📒 Files selected for processing (5)
src/wled/models.pysrc/wled/wled.pytests/__snapshots__/test_models.ambrtests/test_models.pytests/test_wled.py
|
Hey. I think we should put together some documentation that describes how the release file is created and how you can start distributing new releases to your own devices, because we're really the only ones providing the automatic update mechanism, so we're the ones setting the standard. For now, this is just a guess based on how files were created in WLED/WLED, but it was never explicitly written as a rule, so another person would have to reverse engineer our code and release structure in the WLED/WLED repo to be able to create a new repository with custom releases. For example, I'm wondering if GLEDOPTO releases its files correctly? If not, how should they do it? Is there documentation anywhere that describes this? |
|
While working on this, I found out what the scheme is, and it seems really sensible (although it is at present 100% dependent on Github specifically). Its built in two parts: a utility to detect a latest release (which already supports a supplied repo), and then an upgrader (which we're fixing) which will, given a version and a device, find the relevant release file. WLEDReleases is a utility to fetch releases from github, for the repo in question. It will scan for releases and find the latest release, the latest prerelease and so on. Independently.
The The I'm sure you knew this, but this is the consolidated info that is really needed to build docs. |
|
Sound good to me. Can you add that information to README or other docs file? I would like to have documentation that I can send to someone if they want to provide automatic updates. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2078 +/- ##
===========================================
+ Coverage 58.61% 97.52% +38.91%
===========================================
Files 6 8 +2
Lines 662 1133 +471
Branches 143 113 -30
===========================================
+ Hits 388 1105 +717
+ Misses 270 18 -252
- Partials 4 10 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
mik-laj
added
enhancement
|
@mik-laj I gave it a shot. I used the projects Readme.md to host it - if it should go elsewhere, let me know. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@README.md`:
- Around line 70-84: Update the firmware asset naming docs to reflect the full
upgrade contract: replace the pattern literal "WLED_{version}_{release}.bin"
with a generic "{brand}_{version}_{release}.bin[.gz]" and update the explanatory
text to state that "brand" and "release" come from device.info.brand and
device.info.release while "version" is the release tag without the leading "v";
also adjust the example sentence to include device.info.brand (e.g.,
`brand="WLED"`) so the example URL and explanation match the new pattern.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_wled.py`:
- Around line 1270-1271: The test case labeled "missing_device_repo" is
incorrectly using a whitespace string like the "blank_device_repo" case; update
the pytest.param for id "missing_device_repo" to simulate a truly missing repo
(e.g., pass None or omit the repo key) instead of " " so the info.repo fallback
path is exercised—change the first argument in that pytest.param (currently " ")
to None (or adjust the input dict to not include repo) while keeping
DEFAULT_REPO and the test id intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Co-authored-by: Kamil Breguła <mik-laj@users.noreply.github.com>
|
@mik-laj that seems like all of it - besides the CI job failing due to auth, right? |
|
@LordMike The code and documentation look good now, but I'll want to test it in practice. Do you have any favorite repositories that follow our convention? How did you test if this code works? |
|
I did it on my own repo: https://github.com/LordMike/WLED/releases I'm still waiting on this repo and the HA integration to fully see it in action end to end - but at least for my repo, the correct url is produced. I have not tested actually calling upgrade on my device through this, because I know that part works once the file has been downloaded. So just arriving at the correct url was enough for me. I basically just ran the WLED's CI workflows, slightly manipulated. Goal being being able to at some point just retarget.. Then I found this work already in progress, so I jumped the bandwagon :) |
3 participants
Proposed Changes
Adds support for using the repository reported by a WLED device's /json info response when downloading firmware during WLED.upgrade().
The upgrade API still accepts an explicit repo override. If no repo is passed, it uses device.info.repo, which defaults to wled/WLED for firmware that does not report a repository.
Validation performed:
This code was authored by OpenAI Codex.
Related Issues
Related to #2049
Thanks to @netmindz for the original implementation and discussion that shaped this change.
Summary by CodeRabbit
New Features
Tests
Documentation