Use device-reported repo for firmware releases and upgrades#2049
Use device-reported repo for firmware releases and upgrades#2049netmindz wants to merge 7 commits intofrenck:mainfrom
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 51 minutes and 43 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds a device-reported Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/wled/wled.py (1)
607-659:⚠️ Potential issue | 🟠 MajorCaller-supplied
repoargument is silently ignored.Line 659 unconditionally overwrites the local
repowithself._device.info.repo, which discards whatever the caller passed via therepo=keyword argument. SinceInfo.repodefaults toDEFAULT_REPO, any explicit override (e.g.await wled.upgrade(version="...", repo="myfork/WLED")) now has zero effect — a silent behavioral regression of the public API.Either remove the
repoparameter fromupgrade()(breaking change, document it) or give the explicit argument precedence and fall back to the device-reported value only when the caller didn't customize it. The latter preserves backward compatibility:🛠️ Suggested fix
- # Prefer the repo reported by the device itself; older firmware that does - # not include the field will have fallen back to DEFAULT_REPO already. - repo = self._device.info.repo + # Prefer an explicitly supplied repo; otherwise use the repo reported by + # the device itself. Older firmware without the field will have fallen + # back to DEFAULT_REPO during deserialization. + if repo == DEFAULT_REPO: + repo = self._device.info.repoAlso consider updating the
repoargument docstring (Line 618) to reflect the new precedence rules.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/wled/wled.py` around lines 607 - 659, The upgrade() method currently unconditionally overwrites the caller-supplied repo with self._device.info.repo; change repo handling so an explicit caller argument wins: update the signature repo: str | None = None (instead of DEFAULT_REPO), then set local repo = repo if repo is not None else (self._device.info.repo or DEFAULT_REPO) inside upgrade(), and update the repo docstring to state that an explicit repo parameter takes precedence over the device-reported repo. Refer to function upgrade, parameter repo, self._device.info.repo, and DEFAULT_REPO when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/wled/wled.py`:
- Around line 607-659: The upgrade() method currently unconditionally overwrites
the caller-supplied repo with self._device.info.repo; change repo handling so an
explicit caller argument wins: update the signature repo: str | None = None
(instead of DEFAULT_REPO), then set local repo = repo if repo is not None else
(self._device.info.repo or DEFAULT_REPO) inside upgrade(), and update the repo
docstring to state that an explicit repo parameter takes precedence over the
device-reported repo. Refer to function upgrade, parameter repo,
self._device.info.repo, and DEFAULT_REPO when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 453a78d8-c076-401a-ad5a-fc2038917300
📒 Files selected for processing (2)
src/wled/models.pysrc/wled/wled.py
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2049 +/- ##
===========================================
+ Coverage 58.61% 97.12% +38.51%
===========================================
Files 6 8 +2
Lines 662 1079 +417
Branches 143 113 -30
===========================================
+ Hits 388 1048 +660
+ Misses 270 20 -250
- Partials 4 11 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@coderabitai review |
There was a problem hiding this comment.
Pull request overview
Adds a device-reported GitHub repository field to the WLED Info model and updates firmware upgrade behavior to prefer that repository, enabling upgrades for custom-firmware devices without relying solely on the library default.
Changes:
- Add
Info.repo(defaulting toDEFAULT_REPO) to store the device-reportedorg/repo. - Update
WLED.upgrade()to use the device-reported repo when constructing the GitHub download URL. - Add tests covering
Info.repodefaulting/device override and upgrade behavior using a device-reported repo.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/wled/models.py |
Adds Info.repo with DEFAULT_REPO fallback and documentation for the new field. |
src/wled/wled.py |
Changes upgrade URL construction to prefer the device-reported repository. |
tests/test_models.py |
Adds unit tests for Info.repo default and device-provided values. |
tests/test_wled.py |
Adds an upgrade test verifying device-reported repo is used for firmware download. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
frenck
left a comment
frenck
left a comment
There was a problem hiding this comment.
There are comments from copilot, and above all... the CI is failing.
../Frenck
Blogging my personal ramblings at frenck.dev
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/test_wled.py (1)
1335-1368: Optional: reuse_get_wled_with_device_for_upgradehelper and add an explicit URL assertion.The setup duplicates
_get_wled_with_device_for_upgrade(Lines 1206-1233). Consider extending the helper to accept arepokwarg and using it here, then explicitly assert the request was made by inspectingmocked.requestsso the intent is clearer than relying on aioresponses raising on unmatched URLs.♻️ Sketch
async def _get_wled_with_device_for_upgrade( mocked: aioresponses, session: aiohttp.ClientSession, arch: str = "esp32", version: str = "0.14.0", wifi_bssid: str = "AA:BB:CC:DD:EE:FF", + repo: str | None = None, ) -> WLED: wled_data = load_fixture_json("wled") wled_data["info"]["arch"] = arch wled_data["info"]["ver"] = version + if repo is not None: + wled_data["info"]["repo"] = repo if wifi_bssid is not None: wled_data["info"]["wifi"]["bssid"] = wifi_bssid ...Then in the test, after
await wled.upgrade(...), additionally assert that the MoonModules URL was actually hit (e.g., viamocked.requests) rather than relying solely on aioresponses raising on an unmatched URL.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_wled.py` around lines 1335 - 1368, Refactor the duplicated setup by extending the existing helper _get_wled_with_device_for_upgrade to accept a repo kwarg (defaulting to current behavior), then call that helper inside test_upgrade_uses_device_repo to create the WLED instance instead of repeating the session/mock setup; after awaiting wled.upgrade(version="0.15.0") assert explicitly that the GitHub release URL (https://github.com/MoonModules/WLED/releases/download/v0.15.0/WLED_0.15.0_ESP32.bin) was requested by inspecting mocked.requests so the test verifies the device-reported repo was actually used by WLED.upgrade.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/wled/models.py`:
- Around line 493-497: The repo field can contain malformed or malicious values
used by WLED.upgrade() to build GitHub URLs; add a lightweight
post-deserialization validation on the model (implement a __post_deserialize__
or equivalent hook in the class that defines repo) that checks repo against a
strict regex like ^[A-Za-z0-9._-]+/[A-Za-z0-9._-]+$ and, on mismatch, resets the
attribute to DEFAULT_REPO and optionally logs a warning; reference the repo
field name, DEFAULT_REPO constant, and the WLED.upgrade() call-site so reviewers
can verify the fallback and ensure the upgrade URL construction uses the
validated value.
In `@tests/test_models.py`:
- Around line 318-323: The test test_info_repo_defaults_to_default_repo is
importing DEFAULT_REPO inside the test body which causes lint errors
(PLC0415/C0415); move the DEFAULT_REPO import to the module-level import block
at the top of tests/test_models.py (alongside other wled.const imports) and
remove the inline from wled.const import DEFAULT_REPO line from inside the test
so the test simply references DEFAULT_REPO.
---
Nitpick comments:
In `@tests/test_wled.py`:
- Around line 1335-1368: Refactor the duplicated setup by extending the existing
helper _get_wled_with_device_for_upgrade to accept a repo kwarg (defaulting to
current behavior), then call that helper inside test_upgrade_uses_device_repo to
create the WLED instance instead of repeating the session/mock setup; after
awaiting wled.upgrade(version="0.15.0") assert explicitly that the GitHub
release URL
(https://github.com/MoonModules/WLED/releases/download/v0.15.0/WLED_0.15.0_ESP32.bin)
was requested by inspecting mocked.requests so the test verifies the
device-reported repo was actually used by WLED.upgrade.
🪄 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: ef44f2aa-d46b-4688-9fe8-9c7d996ff5ed
📒 Files selected for processing (4)
src/wled/models.pytests/__snapshots__/test_models.ambrtests/test_models.pytests/test_wled.py
|
Fixed @frenck |
| # Prefer the repo reported by the device itself only when the caller | ||
| # did not override the default repository. Older firmware that does not | ||
| # include the field will already have fallen back to DEFAULT_REPO. | ||
| if repo == DEFAULT_REPO: |
There was a problem hiding this comment.
This has a side effect, if the default repo was actually meant to be set, it wouldn't work. We probably need to use a sentinel to make this more robust.
|
|
||
| from wled.const import DEFAULT_REPO | ||
|
|
||
| __all__ = ["DEFAULT_REPO"] |
There was a problem hiding this comment.
This does not make much sense. Instead just import directly
This pull request adds support for devices to specify their own GitHub repository for firmware upgrades, allowing the system to prefer a device-reported repository over the default. This improves flexibility when fetching releases or performing upgrades, especially for devices with custom firmware.
Device-reported repository support:
repofield to theInfomodel insrc/wled/models.py, which stores the GitHub repository in'org/repo'format as reported by the device firmware. This field is used in preference to the default repository when fetching releases or upgrading.DEFAULT_REPOinsrc/wled/models.pyto provide a fallback value for the newrepofield.src/wled/wled.pyto prefer thereporeported by the device itself, improving compatibility with devices running custom firmware.# Proposed ChangesRelated Issues
Summary by CodeRabbit
New Features
Tests