firmware: resolve firmware artifacts from files[]#2915
Open
rahmonov wants to merge 1 commit into
Open
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (13)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (10)
Summary by CodeRabbit
WalkthroughFirmware metadata now supports optional artifact URLs, downloads verify SHA-256, cache filenames derive from URLs, and machine-controller artifact handling uses a configured cache directory. One observability metrics row was added. ChangesFirmware artifact resolution and SHA-256 migration
Observability metrics documentation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
Author
|
@coderabbitai ptal |
Contributor
|
✅ Action performedReview finished.
|
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@crates/api-model/src/firmware.rs`:
- Around line 210-214: The firmware artifact model currently allows entries with
only sha256, but these are invalid for downstream resolution. Add validation to
the firmware config/model in firmware.rs so `files[]` entries must provide at
least one of `filename` or `url`, and reject the shape during
load/deserialization rather than later in `firmware_artifact` handling. Use the
existing artifact struct fields `filename`, `url`, and `sha256` to enforce this
boundary check and keep bad metadata out of machine-controller.
In `@crates/firmware/src/downloader.rs`:
- Around line 61-71: `available()` and `available_actual()` currently
short-circuit on an existing `filename`, which skips the new `sha256` check and
can return stale cached bytes as valid. Update the cache-hit path in
`Downloader::available_actual` so existing files are revalidated against the
requested digest before returning `true`, using the same checksum verification
logic already used for downloaded artifacts. Keep the behavior consistent for
both direct calls through `available()` and the async download flow, and add a
regression test that pre-creates `filename` with mismatched contents and asserts
the cache hit is rejected.
In `@crates/machine-controller/src/handler/firmware_artifact.rs`:
- Around line 60-137: Give url precedence in both resolver functions: in
resolve_firmware_artifact and resolve_scout_file_artifact, branch on
artifact.url before artifact.filename so a populated URL is treated as a remote
artifact. Update the local-path/source selection in resolve_firmware_artifact to
use the URL path first and only fall back to filename when URL is absent, and
adjust resolve_scout_file_artifact so it preserves the remote URL instead of
rewriting it through PXE-local handling when url is provided.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f09737c5-fd80-4f27-97da-20a7391280f5
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
Cargo.tomlcrates/api-model/src/firmware.rscrates/firmware/Cargo.tomlcrates/firmware/src/artifact_cache.rscrates/firmware/src/downloader.rscrates/firmware/src/lib.rscrates/firmware/src/tests/config.rscrates/firmware/src/tests/downloader.rscrates/machine-controller/src/config/firmware_global.rscrates/machine-controller/src/handler.rscrates/machine-controller/src/handler/firmware_artifact.rs
💤 Files with no reviewable changes (1)
- Cargo.toml
rahmonov
force-pushed
the
migration-to-files
branch
from
5e760dd to
3395c31
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
crates/machine-controller/src/handler/firmware_artifact.rs (1)
173-496: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse table-driven resolver tests.
These cases are repeated input/output variants for the same resolver functions. Consolidating them with
value_scenarios!,scenarios!, or explicitcheck_values/check_caseswill keep coverage easier to extend. As per coding guidelines, “Use table-driven test style when writing tests in Rust.”🤖 Prompt for 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. In `@crates/machine-controller/src/handler/firmware_artifact.rs` around lines 173 - 496, The resolver tests are written as many nearly identical case-by-case unit tests instead of a table-driven style. Refactor the repeated scenarios around resolve_firmware_artifact and resolve_scout_file_artifact into parameterized/table-based tests using the existing test helpers or macros like value_scenarios!, scenarios!, check_values, or check_cases. Keep the same assertions and coverage, but group the input/output variants into compact case lists so new cases can be added without duplicating test bodies.Sources: Coding guidelines, Path instructions
🤖 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 `@crates/api-model/src/firmware.rs`:
- Around line 324-334: The artifact_count method currently treats legacy entries
with only url set as empty, so update the single-artifact branch in
artifact_count to also return 1 when self.url is present. Make the change
alongside the existing checks for self.files, self.filenames, and self.filename,
and add a regression test covering a url-only Firmware entry to verify
backward-compatible behavior.
In `@crates/machine-controller/src/handler/firmware_artifact.rs`:
- Around line 157-164: Percent-encode the PXE URL path segments before building
the return value in firmware_artifact.rs, because using the `relative` string
from `Component::Normal` directly in the `Ok(format!(...))` path can produce
invalid URLs or alter the fragment/query. Update the `relative.to_str()` /
`Ok(format!(...))` flow so each path segment is encoded before concatenation,
while keeping the existing `StateHandlerError::GenericError` handling for
invalid UTF-8 intact.
- Around line 72-79: `resolve_firmware_artifact()` currently rejects remote
artifacts when `firmware_cache_filename()` cannot derive a basename from the
URL, even if `files[].filename` is present. Update the `local_path` handling in
`firmware_artifact.rs` so the `Remote` case first tries
`firmware_cache_filename(...)` and, if that returns `None`, falls back to a
cache path built from `files[].filename` while keeping `source` as `Remote`. Use
the existing `firmware.version`, `pos`, and `files[].filename` context to keep
the error path only for cases where neither source is usable.
---
Nitpick comments:
In `@crates/machine-controller/src/handler/firmware_artifact.rs`:
- Around line 173-496: The resolver tests are written as many nearly identical
case-by-case unit tests instead of a table-driven style. Refactor the repeated
scenarios around resolve_firmware_artifact and resolve_scout_file_artifact into
parameterized/table-based tests using the existing test helpers or macros like
value_scenarios!, scenarios!, check_values, or check_cases. Keep the same
assertions and coverage, but group the input/output variants into compact case
lists so new cases can be added without duplicating test bodies.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 2b35efd5-f1f9-4666-8d70-e4d9fca9d2a3
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
Cargo.tomlcrates/api-model/src/firmware.rscrates/firmware/Cargo.tomlcrates/firmware/src/artifact_cache.rscrates/firmware/src/downloader.rscrates/firmware/src/lib.rscrates/firmware/src/tests/config.rscrates/firmware/src/tests/downloader.rscrates/machine-controller/src/config/firmware_global.rscrates/machine-controller/src/handler.rscrates/machine-controller/src/handler/firmware_artifact.rs
💤 Files with no reviewable changes (1)
- Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (8)
- crates/firmware/src/lib.rs
- crates/firmware/Cargo.toml
- crates/firmware/src/tests/config.rs
- crates/firmware/src/artifact_cache.rs
- crates/firmware/src/tests/downloader.rs
- crates/machine-controller/src/config/firmware_global.rs
- crates/firmware/src/downloader.rs
- crates/machine-controller/src/handler.rs
rahmonov
force-pushed
the
migration-to-files
branch
from
3395c31 to
dff3559
Compare
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
Support the new files[] firmware artifact shape for host firmware upgrades. Artifacts with urls are resolved as remote downloads into the firmware cache; artifacts without urls, and legacy entries without files[], are treated as local files. Move Scout file artifact URL resolution into the firmware artifact module and fail firmware upgrades when a required local artifact is missing instead of deferring indefinitely.
rahmonov
force-pushed
the
migration-to-files
branch
from
dff3559 to
be081f1
Compare
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
1 participant
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.
We are moving towards being able to manage firmware configuration at runtime.
One of the requirements for this is to download the firmware files at runtime rather than packaging files into the filesystem at deployment (which means updates require re-deployment).
But we need to be careful with regards to backwards-compatibility. That's why I am introducing a resolving logic in this PR.
If the URL is present in the config, it means it needs to be download. If not, the code needs to look at the local filesystem.
Related issues
Type of Change
Testing
Additional Notes
Suggestion to the reviewer: